Skip to content

net: fix net.Server.prototype.listen error message - #11693

Closed
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:fix-net-listen-error
Closed

net: fix net.Server.prototype.listen error message#11693
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:fix-net-listen-error

Conversation

@joyeecheung

@joyeecheungjoyeecheung commented Mar 5, 2017

Copy link
Copy Markdown
Member

The first commit uses util.inspect to make the error messages thrown by server.listen more useful.

Before:

net.createServer().listen(null)// Error: Invalid listen argument: [object Object]

After:

net.createServer().listen(null)// Error: Invalid listen argument: { port: null }

The second commit refactors the listen option test, adding precise error message validation and a few more test cases.

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)

net, test

@nodejs-github-botnodejs-github-bot added the net Issues and PRs related to the net subsystem. label Mar 5, 2017
@joyeecheungjoyeecheung added the test Issues and PRs related to the tests. label Mar 5, 2017
@mscdexmscdex added semver-major PRs that contain breaking changes and should be released in the next major version. test Issues and PRs related to the tests. and removed test Issues and PRs related to the tests. labels Mar 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate?

Previously the error messages are mostly `[object Object]`
after the options get normalized. Use util.inspect to make
it more useful.
Refactor the listen option test, add precise
error message validation and a few more test cases.
@joyeecheung
joyeecheungforce-pushed the fix-net-listen-error branch from 8ae4ffc to dbcba0dCompareMarch 8, 2017 07:45
@joyeecheung

Copy link
Copy Markdown
MemberAuthor

Got a conflict after #11667 landed. Rebased.

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

@fhinkel

Copy link
Copy Markdown
Member

Thanks. Squashed and landed in 4775942

@fhinkelfhinkel closed this Mar 9, 2017
fhinkel pushed a commit that referenced this pull request Mar 9, 2017
Previously the error messages are mostly `[object Object]`
after the options get normalized. Use util.inspect to make
it more useful.
Refactor the listen option test, add precise
error message validation and a few more test cases.
PR-URL: #11693
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
Previously the error messages are mostly `[object Object]`
after the options get normalized. Use util.inspect to make
it more useful.
Refactor the listen option test, add precise
error message validation and a few more test cases.
PR-URL: nodejs#11693
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@jasnelljasnell mentioned this pull request Apr 4, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

netIssues and PRs related to the net subsystem.semver-majorPRs that contain breaking changes and should be released in the next major version.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@joyeecheung@fhinkel@jasnell@lpinca@mscdex@nodejs-github-bot