test: fix flaky test-dgram-empty-packet & friends #9724

Trott · 2016-11-21T19:10:21Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test dgram

Description of change

Liberal use of common.mustCall()
Rename test-dgram-empty-packet -> test-dgram-send-empty-packet
Remove use of timers to avoid CI failures like seen in the Ref below:

not ok 237 parallel/test-dgram-empty-packet
  ---
  duration_ms: 0.717
  severity: fail
  stack: |-
    /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-dgram-empty-packet.js:38
        throw new Error('Timeout');
        ^

    Error: Timeout
        at Timeout._onTimeout
(/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-dgram-empty-packet.js:38:11)
        at ontimeout (timers.js:365:14)
        at tryOnTimeout (timers.js:237:5)
        at Timer.listOnTimeout (timers.js:207:5)

Refs: https://ci.nodejs.org/job/node-test-commit-freebsd/5341/nodes=freebsd11-x64/console:

Trott · 2016-11-21T19:11:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4921/

cjihrig · 2016-11-21T19:26:17Z

Looks like the CI had a hiccup on OS X, which is pretty relevant to this test.

Trott · 2016-11-21T19:27:50Z

@cjihrig Yeah, looks like the bug on OS X is still there on the verison of OS X in CI. Putting the skip-code back in...

Trott · 2016-11-21T19:29:25Z

CI again: https://ci.nodejs.org/job/node-test-pull-request/4923/

Trott · 2016-11-21T21:02:43Z

CI looks good. /cc @nodejs/testing @indutny

santigimeno · 2016-11-22T22:20:58Z

The changes look good. I only see an issue with relying that every UDP message is going to be received by the server. This, though highly unlikely, could be a source of flakiness.

Trott · 2016-11-23T06:56:00Z

@santigimeno I've wrapped the client.send() call in setInterval() so that it will try again if the UDP message is not received.

CI: https://ci.nodejs.org/job/node-test-pull-request/4958/

santigimeno

LGTM with a couple of suggestions. Thanks!

santigimeno · 2016-11-23T09:08:24Z

test/parallel/test-dgram-send-empty-buffer.js

  const port = this.address().port;

  client.on('message', common.mustCall(function onMessage(buffer, bytes) {
-    clearTimeout(timer);
+    clearInterval(interval);
    client.close();
  }));


maybe check that buffer is empty. Also s/bytes/info or even removing the 2nd argument if we're doing nothing with it

santigimeno · 2016-11-23T09:09:17Z

test/parallel/test-dgram-send-empty-packet.js

+    if (firstArg instanceof Buffer) {
+      clearInterval(interval);
+      client.close();
+    }


check firstArg is empty too?

* Liberal use of common.mustCall() * Rename test-dgram-empty-packet -> test-dgram-send-empty-packet * Remove use of timers to avoid CI failures like seen in the Ref below: ``` not ok 237 parallel/test-dgram-empty-packet --- duration_ms: 0.717 severity: fail stack: |- ... throw new Error('Timeout'); ^ Error: Timeout at Timeout._onTimeout ... at ontimeout (timers.js:365:14) at tryOnTimeout (timers.js:237:5) at Timer.listOnTimeout (timers.js:207:5) ``` Refs: https://ci.nodejs.org/job/node-test-commit-freebsd/5341/nodes=freebsd11-x64/console:

Trott · 2016-11-24T04:55:16Z

Excellent suggestions from @santigimeno incorporated. CI again: https://ci.nodejs.org/job/node-test-pull-request/4978/

Trott · 2016-11-24T22:38:40Z

Raspberry Pi failure in CI is a build failure and unrelated.

* Liberal use of common.mustCall() * Rename test-dgram-empty-packet -> test-dgram-send-empty-packet * Remove use of timers to avoid CI failures like seen in the Ref below: ``` not ok 237 parallel/test-dgram-empty-packet --- duration_ms: 0.717 severity: fail stack: |- ... throw new Error('Timeout'); ^ Error: Timeout at Timeout._onTimeout ... at ontimeout (timers.js:365:14) at tryOnTimeout (timers.js:237:5) at Timer.listOnTimeout (timers.js:207:5) ``` Refs: https://ci.nodejs.org/job/node-test-commit-freebsd/5341/nodes=freebsd11-x64/console: PR-URL: nodejs#9724 Reviewed-By: Santiago Gimeno <[email protected]>

Trott · 2016-11-24T22:40:34Z

Landed in 9b0f53d

* Liberal use of common.mustCall() * Rename test-dgram-empty-packet -> test-dgram-send-empty-packet * Remove use of timers to avoid CI failures like seen in the Ref below: ``` not ok 237 parallel/test-dgram-empty-packet --- duration_ms: 0.717 severity: fail stack: |- ... throw new Error('Timeout'); ^ Error: Timeout at Timeout._onTimeout ... at ontimeout (timers.js:365:14) at tryOnTimeout (timers.js:237:5) at Timer.listOnTimeout (timers.js:207:5) ``` Refs: https://ci.nodejs.org/job/node-test-commit-freebsd/5341/nodes=freebsd11-x64/console: PR-URL: #9724 Reviewed-By: Santiago Gimeno <[email protected]>

* Liberal use of common.mustCall() * Rename test-dgram-empty-packet -> test-dgram-send-empty-packet * Remove use of timers to avoid CI failures like seen in the Ref below: ``` not ok 237 parallel/test-dgram-empty-packet --- duration_ms: 0.717 severity: fail stack: |- ... throw new Error('Timeout'); ^ Error: Timeout at Timeout._onTimeout ... at ontimeout (timers.js:365:14) at tryOnTimeout (timers.js:237:5) at Timer.listOnTimeout (timers.js:207:5) ``` Refs: https://ci.nodejs.org/job/node-test-commit-freebsd/5341/nodes=freebsd11-x64/console: PR-URL: nodejs#9724 Reviewed-By: Santiago Gimeno <[email protected]>

* Liberal use of common.mustCall() * Rename test-dgram-empty-packet -> test-dgram-send-empty-packet * Remove use of timers to avoid CI failures like seen in the Ref below: ``` not ok 237 parallel/test-dgram-empty-packet --- duration_ms: 0.717 severity: fail stack: |- ... throw new Error('Timeout'); ^ Error: Timeout at Timeout._onTimeout ... at ontimeout (timers.js:365:14) at tryOnTimeout (timers.js:237:5) at Timer.listOnTimeout (timers.js:207:5) ``` Refs: https://ci.nodejs.org/job/node-test-commit-freebsd/5341/nodes=freebsd11-x64/console: PR-URL: #9724 Reviewed-By: Santiago Gimeno <[email protected]>

Trott added dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests. labels Nov 21, 2016

Trott force-pushed the de-timer-ize branch 2 times, most recently from f60c297 to 06c26b6 Compare November 21, 2016 19:25

Trott force-pushed the de-timer-ize branch from 06c26b6 to 1df9e64 Compare November 21, 2016 19:27

Trott force-pushed the de-timer-ize branch 3 times, most recently from b33bfca to c1c40fd Compare November 23, 2016 06:55

santigimeno approved these changes Nov 23, 2016

View reviewed changes

Trott added 4 commits November 23, 2016 20:42

squash: intervals

84abe59

squash:nits

e5a0a86

squash: nits

2f73690

Trott force-pushed the de-timer-ize branch from c1c40fd to 2f73690 Compare November 24, 2016 04:47

santigimeno approved these changes Nov 24, 2016

View reviewed changes

Trott closed this Nov 24, 2016

Fishrock123 mentioned this pull request Dec 5, 2016

v7.2.1 proposal #10127

Merged

2 tasks

addaleax added the land-on-v6.x label Dec 8, 2016

MylesBorins added the dont-land-on-v4.x label Dec 20, 2016

MylesBorins mentioned this pull request Dec 21, 2016

V6.9.3 proposal #10394

Merged

Trott deleted the de-timer-ize branch January 13, 2022 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix flaky test-dgram-empty-packet & friends #9724

test: fix flaky test-dgram-empty-packet & friends #9724

Trott commented Nov 21, 2016 •

edited

Loading

Trott commented Nov 21, 2016

cjihrig commented Nov 21, 2016

Trott commented Nov 21, 2016

Trott commented Nov 21, 2016

Trott commented Nov 21, 2016

santigimeno commented Nov 22, 2016

Trott commented Nov 23, 2016

santigimeno left a comment

santigimeno Nov 23, 2016

santigimeno Nov 23, 2016

Trott commented Nov 24, 2016

Trott commented Nov 24, 2016

Trott commented Nov 24, 2016

test: fix flaky test-dgram-empty-packet & friends #9724

test: fix flaky test-dgram-empty-packet & friends #9724

Conversation

Trott commented Nov 21, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Trott commented Nov 21, 2016

cjihrig commented Nov 21, 2016

Trott commented Nov 21, 2016

Trott commented Nov 21, 2016

Trott commented Nov 21, 2016

santigimeno commented Nov 22, 2016

Trott commented Nov 23, 2016

santigimeno left a comment

Choose a reason for hiding this comment

santigimeno Nov 23, 2016

Choose a reason for hiding this comment

santigimeno Nov 23, 2016

Choose a reason for hiding this comment

Trott commented Nov 24, 2016

Trott commented Nov 24, 2016

Trott commented Nov 24, 2016

Trott commented Nov 21, 2016 •

edited

Loading