Skip to content

test: refactor test-http-abort-before-end#18508

Merged
cjihrig merged 1 commit intonodejs:masterfrom
cjihrig:test
Feb 7, 2018
Merged

test: refactor test-http-abort-before-end#18508
cjihrig merged 1 commit intonodejs:masterfrom
cjihrig:test

Conversation

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Feb 1, 2018 •
edited
Loading

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-bot nodejs-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 •
edited
Loading

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
Contributor Author

cjihrig commented Feb 1, 2018

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
Contributor Author

cjihrig commented Feb 1, 2018 •
edited
Loading

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

@BridgeAR BridgeAR 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

BridgeAR commented Feb 6, 2018

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

@cjihrig
test: refactor test-http-abort-before-end
ffb385b 
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 <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Feb 7, 2018

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

@cjihrig cjihrig merged commit ffb385b into nodejs:master Feb 7, 2018
@cjihrig cjihrig deleted the test branch February 7, 2018 18:45
@apapirovski apapirovski mentioned this pull request Feb 8, 2018
Closed
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@cjihrig @MylesBorins
test: refactor test-http-abort-before-end
f422b82 
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 <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@cjihrig @MylesBorins
test: refactor test-http-abort-before-end
4deffa2 
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 <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@cjihrig @MylesBorins
test: refactor test-http-abort-before-end
f3e6c76 
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 <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
gibfahn pushed a commit that referenced this pull request Apr 13, 2018
@cjihrig @gibfahn
test: refactor test-http-abort-before-end
bc883fb 
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 <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Apr 13, 2018
@cjihrig @gibfahn
test: refactor test-http-abort-before-end
cbd698a 
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 <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 13, 2018
Merged
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@cjihrig @MayaLekova
test: refactor test-http-abort-before-end
575b7f8 
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 <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

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