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.

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

test: wrap assert.fail when passed to callback #3453

Closed
wants to merge 1 commit into from
Closed

test: wrap assert.fail when passed to callback #3453

wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

Currently there are many instances where assert.fail is directly passed
to a callback for error handling. Unfortunately this will swallow the error
as it is the third argument of assert.fail that sets the message not the first.

This commit adds a new function to test/common.js that simply wraps assert.fail
and calls it with the provided message.

Tip of the hat to @Trott for pointing me in the direction of this.

@MylesBorins
Copy link
Contributor Author

/cc @jasnell

@jasnell
Copy link
Member

jasnell commented Oct 20, 2015

LGTM

@mscdex mscdex added the test Issues and PRs related to the tests. label Oct 20, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Oct 20, 2015

LGTM pending CI

@@ -25,7 +25,7 @@ process.on('exit', function() {
console.log('ok');
});

// make sure that flush/write doesn't trigger an assert failure
// make sure that flush/write doesn't trigger an common.failure
Copy link
Member

Choose a reason for hiding this comment

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

find/replace error in this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Trott
Copy link
Member

Trott commented Oct 20, 2015

LGTM pending CI

Speaking of which: https://ci.nodejs.org/job/node-test-pull-request/547/

@Trott
Copy link
Member

Trott commented Oct 20, 2015

CI results breakdown:

freebsd101-32 failure is a known flaky test that #3430 intends to fix.

Three tests on ppcbe-fedora20 and one test on ppcle-ubuntu1404 (or did I put that backwards?) are all known fail-every-time tests on those platforms. (Probably should mark them as flaky for those platforms in test/parallel/parallel.status.)

win2008r2 vs2015 is build step explosion. ¯\_(ツ)_/¯

win10 vs2015 is one test failure that was untouched by this PR and then a build step explosion.

ubuntu1404-32 is a failure in a test untouched by this PR.

In short: LGTM

@brendanashworth
Copy link
Contributor

I don't have a specific concern with either this or #3378, but my thought regarding this particular pattern is that it is already in use in both core and user land. Would it be worth implementing this as an official API?

It would be, I guess, assert.fail(message) and assert.fail(val1, val2, operator). Currently the signature is assert.fail(val1, val2, message, operator) which is weird because message and { val1, val2, operator } are exclusive arguments, i.e. if message is falsy the rest are used, but if it is truthy the rest are ignored. Which leads to, logically, we should have two signatures instead of one.

Here is the illogical output for what I'd imagine are common paradigms:

> assert.fail()
AssertionError: undefined undefined undefined
    at repl:1:8
    at ...
> assert.fail('uh oh!')
AssertionError: 'uh oh!' undefined undefined
    at repl:1:8
    at ...
> assert.fail(1e2, 1e3, 'not equal', '!==')
AssertionError: not equal
    at repl:1:8
    at ...

Yes, I'm aware people want to lock this as a module but this I think is a good reason why it shouldn't be done. This is already expected for the most part.

This shouldn't hold this up though, just tangential.

@MylesBorins
Copy link
Contributor Author

@jasnell is this ready to land?

@Trott
Copy link
Member

Trott commented Oct 23, 2015

@thealphanerd if you could amend the commit message so that the body wraps at 72 chars as specified in CONTRIBUTING.md, that will save James or whoever lands this that little step. Might as well rebase against current master while you're at it just to suss out any conflicts that may have arisen with commits from the last 2 days.

Currently there are many instances where assert.fail is directly passed
to a callback for error handling. Unfortunately this will swallow the
error as it is the third argument of assert.fail that sets the message
not the first.

This commit adds a new function to test/common.js that simply wraps
assert.fail and calls it with the provided message.

Tip of the hat to @Trott for pointing me in the direction of this.
@Trott
Copy link
Member

Trott commented Oct 24, 2015

Landed in 28e9a02

@Trott Trott closed this Oct 24, 2015
Trott pushed a commit that referenced this pull request Oct 24, 2015
Currently there are many instances where assert.fail is directly passed
to a callback for error handling. Unfortunately this will swallow the
error as it is the third argument of assert.fail that sets the message
not the first.

This commit adds a new function to test/common.js that simply wraps
assert.fail and calls it with the provided message.

Tip of the hat to @Trott for pointing me in the direction of this.

PR-URL: #3453
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 26, 2015
Currently there are many instances where assert.fail is directly passed
to a callback for error handling. Unfortunately this will swallow the
error as it is the third argument of assert.fail that sets the message
not the first.

This commit adds a new function to test/common.js that simply wraps
assert.fail and calls it with the provided message.

Tip of the hat to @Trott for pointing me in the direction of this.

PR-URL: #3453
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

Landed in v4.x-staging in 635f743

rvagg pushed a commit that referenced this pull request Oct 26, 2015
Currently there are many instances where assert.fail is directly passed
to a callback for error handling. Unfortunately this will swallow the
error as it is the third argument of assert.fail that sets the message
not the first.

This commit adds a new function to test/common.js that simply wraps
assert.fail and calls it with the provided message.

Tip of the hat to @Trott for pointing me in the direction of this.

PR-URL: #3453
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 29, 2015
Currently there are many instances where assert.fail is directly passed
to a callback for error handling. Unfortunately this will swallow the
error as it is the third argument of assert.fail that sets the message
not the first.

This commit adds a new function to test/common.js that simply wraps
assert.fail and calls it with the provided message.

Tip of the hat to @Trott for pointing me in the direction of this.

PR-URL: #3453
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants