Skip to content

Conversation

@cjihrig
Copy link
Contributor

@cjihrigcjihrig commented Feb 1, 2018

This test was added over six years ago, and the behavior seems to have changed since then. When the test was originally written, common.mustCall() did not exist. The only assertion in this test was not actually executing. Instead of an 'error' event being emitted as expected, an 'abort' event was emitted.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Feb 1, 2018
@lpinca
Copy link
Member

lpinca commented Feb 1, 2018

Change lgtm but why does it fixes the connect EADDRNOTAVAIL error?
Edit with answer: because 'abort' is emitted in the next tick.

@cjihrig
Copy link
ContributorAuthor

My guess is some race between req.end() and server.close(). This change waits to call server.close() until we've received the event we're looking for.

@cjihrig
Copy link
ContributorAuthor

cjihrig commented Feb 1, 2018

Actually, now that I say that, I don't think that would cause EADDRNOTAVAIL. I still think the change is preferable though.

@BridgeARBridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 6, 2018
@BridgeAR
Copy link
Member

@cjihrig please always trigger a CI after opening a new PR :-)

This test was added over six years ago, and the behavior seems to have changed since then. When the test was originally written, common.mustCall() did not exist. The only assertion in this test was not actually executing. Instead of an 'error' event being emitted as expected, an 'abort' event was emitted. PR-URL: nodejs#18508 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
@cjihrig
Copy link
ContributorAuthor

@cjihrigcjihrig merged commit ffb385b into nodejs:masterFeb 7, 2018
@cjihrigcjihrig deleted the test branch February 7, 2018 18:45
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
This test was added over six years ago, and the behavior seems to have changed since then. When the test was originally written, common.mustCall() did not exist. The only assertion in this test was not actually executing. Instead of an 'error' event being emitted as expected, an 'abort' event was emitted. PR-URL: #18508 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
This test was added over six years ago, and the behavior seems to have changed since then. When the test was originally written, common.mustCall() did not exist. The only assertion in this test was not actually executing. Instead of an 'error' event being emitted as expected, an 'abort' event was emitted. PR-URL: #18508 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
This test was added over six years ago, and the behavior seems to have changed since then. When the test was originally written, common.mustCall() did not exist. The only assertion in this test was not actually executing. Instead of an 'error' event being emitted as expected, an 'abort' event was emitted. PR-URL: #18508 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 21, 2018
gibfahn pushed a commit that referenced this pull request Apr 13, 2018
This test was added over six years ago, and the behavior seems to have changed since then. When the test was originally written, common.mustCall() did not exist. The only assertion in this test was not actually executing. Instead of an 'error' event being emitted as expected, an 'abort' event was emitted. PR-URL: #18508 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this pull request Apr 13, 2018
This test was added over six years ago, and the behavior seems to have changed since then. When the test was originally written, common.mustCall() did not exist. The only assertion in this test was not actually executing. Instead of an 'error' event being emitted as expected, an 'abort' event was emitted. PR-URL: #18508 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Apr 13, 2018
@MylesBorinsMylesBorins mentioned this pull request May 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
This test was added over six years ago, and the behavior seems to have changed since then. When the test was originally written, common.mustCall() did not exist. The only assertion in this test was not actually executing. Instead of an 'error' event being emitted as expected, an 'abort' event was emitted. PR-URL: nodejs#18508 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@cjihrig@lpinca@BridgeAR@jasnell@gibfahn@nodejs-github-bot