Skip to content

Conversation

@santigimeno
Copy link
Member

This test was sometimes timing out in OS X. Remove the timeout and
clean up the code.

This test has failed in my OS X box with this error:

=== release test-dgram-udp4 ===                                                
Path: parallel/test-dgram-udp4
server is listening on 0.0.0.0:12446
/Users/sgimeno/node/node/test/parallel/test-dgram-udp4.js:59
  throw new Error('Timeout');
  ^

Error: Timeout
    at null._onTimeout (/Users/sgimeno/node/node/test/parallel/test-dgram-udp4.js:59:9)
    at Timer.listOnTimeout (timers.js:92:15)
Command: out/Release/node /Users/sgimeno/node/node/test/parallel/test-dgram-udp4.js

This test was sometimes timing out in `OS X`. Remove the timeout and
clean up the code.
@r-52 r-52 added dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests. labels Feb 20, 2016
@jasnell
Copy link
Member

jasnell commented Feb 21, 2016

LGTM

@r-52
Copy link
Contributor

r-52 commented Feb 22, 2016

@mcollina
Copy link
Member

I'm not sure removing the timeout is a great call, otherwise the test will get stuck instead of failing (as far as I understand it).
However, we should be consistent within all dgram tests.

@santigimeno
Copy link
Member Author

@mcollina the idea was that the test runner timeout handled it. Sometimes the use of timeouts has been a source of test flakiness. Do you see it as a problem?

@mcollina
Copy link
Member

No, I'm happy with this.
There are more that needs the timeout removal.

@mcollina
Copy link
Member

LGTM

@santigimeno
Copy link
Member Author

There are more that needs the timeout removal.

Agreed. See: nodejs/testing#19

@mcollina
Copy link
Member

are you a contributor here, or should I land this?

@santigimeno
Copy link
Member Author

I am not, please land. Thanks

mcollina pushed a commit to mcollina/node that referenced this pull request Feb 26, 2016
This test was sometimes timing out in `OS X`. Remove the timeout and
clean up the code.

Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#5339
@mcollina
Copy link
Member

Landed in dff01d1

@mcollina mcollina closed this Feb 26, 2016
rvagg pushed a commit that referenced this pull request Feb 27, 2016
This test was sometimes timing out in `OS X`. Remove the timeout and
clean up the code.

Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #5339
rvagg pushed a commit that referenced this pull request Feb 27, 2016
This test was sometimes timing out in `OS X`. Remove the timeout and
clean up the code.

Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #5339
@Fishrock123 Fishrock123 mentioned this pull request Mar 1, 2016
5 tasks
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
This test was sometimes timing out in `OS X`. Remove the timeout and
clean up the code.

Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #5339
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
This test was sometimes timing out in `OS X`. Remove the timeout and
clean up the code.

Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #5339
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants