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: use common.fail() instead of assert(false) #10899

Merged
merged 1 commit into from
Jan 23, 2017
Merged

test: use common.fail() instead of assert(false) #10899

merged 1 commit into from
Jan 23, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 19, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Jan 19, 2017
@italoacasas
Copy link
Contributor

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

lpinca
lpinca reviewed Jan 21, 2017
View reviewed changes
test/parallel/test-net-reconnect-error.js
@@ -10,8 +10,7 @@ let disconnect_count = 0;
const c = net.createConnection(common.PORT);

c.on('connect', function() {
console.error('CLIENT connected');
assert.ok(false);
common.fail('client should not have connected');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: any reason for not using c.on('connect', common.fail);?

Copy link
Member

Choose a reason for hiding this comment

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

common.fail() expects a message for the first argument. If you do c.on('connect', common.fail);, you end up with AssertionError: null undefined null which is not nearly as useful as AssertionError: client should not have connected.

Copy link
Member

@lpinca lpinca Jan 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, but there are other cases where this is already done, for example https://github.com/nodejs/node/pull/10899/files#diff-74c7cfcd74dc6766c9877fac9daf0721R19.

I'm also not a fan of using common.fail() when the argument is not a string for example when it used as a 'request' listener or similar as you end up with AssertionError: [object Object].

@cjihrig
test: use common.fail() instead of assert(false)
aa0e4f3 
PR-URL: nodejs#10899
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@cjihrig cjihrig merged commit aa0e4f3 into nodejs:master Jan 23, 2017
@cjihrig cjihrig deleted the asserts branch January 23, 2017 14:52
targos pushed a commit that referenced this pull request Jan 28, 2017
@cjihrig @targos
test: use common.fail() instead of assert(false)
ea80934 
PR-URL: #10899
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@cjihrig @italoacasas
test: use common.fail() instead of assert(false)
b7bf43a 
PR-URL: nodejs#10899
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@cjihrig @italoacasas
test: use common.fail() instead of assert(false)
2fd4ee6 
PR-URL: nodejs#10899
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

This will need backport PRs to land on v6 or v4

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

@Trott Trott Trott left review comments

@italoacasas italoacasas italoacasas approved these changes

@targos targos targos 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.
8 participants
@cjihrig @italoacasas @jasnell @Trott @lpinca @targos @mscdex @nodejs-github-bot