Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Feb 7, 2018
Merged

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

merged 1 commit into from
Feb 7, 2018

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

@lpinca lpinca lpinca approved these changes

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