test: refactor test for net listen on fd0#10025
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
julianduque added the code-and-learn 
imyller removed the code-and-learn julianduque added the code-and-learn 
test/parallel/test-net-listen-fd0.js Outdated
Member
There was a problem hiding this comment.
I think you could get rid of the exit listener if you assert in the error callback that e.code can only be EINVAL or ENOTSOCK
ContributorAuthor
There was a problem hiding this comment.
@santigimeno do you suggest to do something like:
.on('error', common.mustCall(function(e){switch (e.code){case 'EINVAL': case 'ENOTSOCK': assert(e instanceof Error); break} 
cjihrig reviewed Dec 2, 2016
test/parallel/test-net-listen-fd0.js Outdated
Contributor
There was a problem hiding this comment.
Can you capitalize and punctuate the comment please.
julianduqueforce-pushed the test-net-listen-fd0 branch from a3d0886 to dab7b0cCompare
ContributorAuthor
julianduque commented Dec 2, 2016
Rebased with suggestions from @cjihrig and @santigimeno |
gibfahn approved these changes Dec 2, 2016
Contributor
Fishrock123 commented Dec 2, 2016
cjihrig reviewed Dec 3, 2016
test/parallel/test-net-listen-fd0.js Outdated
Contributor
There was a problem hiding this comment.
This seems incorrect to me. The previous behavior ran the assertion no matter what. The new behavior guarantees that an error is emitted, but not that it is the correct type. What if you just did:
assert(einstanceofError);assert(['EINVAL','ENOTSOCK'].includes(e.code));ContributorAuthor
There was a problem hiding this comment.
@cjihrig way better, I like it, just rebased with your suggestions
Replace var to const/let, use common.mustCall on callbacks and move process.on('exit') to the .on('error') handlerjulianduqueforce-pushed the test-net-listen-fd0 branch from dab7b0c to 7255098Compare
Member
gibfahn commented Dec 5, 2016
jasnell approved these changes Dec 5, 2016
jasnell pushed a commit that referenced this pull request Dec 5, 2016
Replace var to const/let, use common.mustCall on callbacks and move process.on('exit') to the .on('error') handler PR-URL: #10025 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Member
jasnell commented Dec 5, 2016
Landed in 1d766b8. Thank you for the PR and for participating in the code-and-learn! |
Fishrock123 pushed a commit that referenced this pull request Dec 6, 2016
Replace var to const/let, use common.mustCall on callbacks and move process.on('exit') to the .on('error') handler PR-URL: #10025 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
Replace var to const/let, use common.mustCall on callbacks and move process.on('exit') to the .on('error') handler PR-URL: nodejs#10025 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
Replace var to const/let, use common.mustCall on callbacks and move process.on('exit') to the .on('error') handler PR-URL: nodejs#10025 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Replace var to const/let, use common.mustCall on callbacks and move process.on('exit') to the .on('error') handler PR-URL: #10025 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test, net
Description of change
Replace var to const/let, use common.mustCall on callbacks and move
process.on('exit') to the .on('error') handler