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: apply correct assert.fail() arguments #3378

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 15, 2015

The assert.fail function signature has the message as the third argument
but, understandably, it is often assumed that it is the first argument
(or at least the first argument if no other arguments are passed).

This corrects the assert.fail() invocations in the Node.js tests.

Before:
assert.fail('message');
// result: AssertionError: 'message' undefined undefined

After:
assert.fail(null, null, 'message');
// result: AssertionError: message

The assert.fail function signature has the message as the third argument
but, understandably, it is often assumed that it is the first argument
(or at least the first argument if no other arguments are passed).

This corrects the assert.fail() invocations in the Node.js tests.

Before:
assert.fail('message');
// result: AssertionError: 'message' undefined undefined

After:
assert.fail(null, null, 'message');
// result: AssertionError: message
@Trott Trott added the test Issues and PRs related to the tests. label Oct 15, 2015
@Trott
Copy link
Member Author

Trott commented Oct 15, 2015

@Trott
Copy link
Member Author

Trott commented Oct 15, 2015

CI looks like it was having issues building some hosts. Trying again: https://ci.nodejs.org/job/node-test-commit/847/

@cjihrig
Copy link
Contributor

cjihrig commented Oct 15, 2015

LGTM. Aren't there a number of occurrences where assert.fail() is used as a callback or event handler like foo.on('event', assert.fail)? Just curious if that turned up in your search at all.

@Trott
Copy link
Member Author

Trott commented Oct 15, 2015

@cjihrig Yes, that's a thing too.

Trott added a commit that referenced this pull request Oct 16, 2015
The assert.fail function signature has the message as the third argument
but, understandably, it is often assumed that it is the first argument
(or at least the first argument if no other arguments are passed).

This corrects the assert.fail() invocations in the Node.js tests.

Before:
assert.fail('message');
// result: AssertionError: 'message' undefined undefined

After:
assert.fail(null, null, 'message');
// result: AssertionError: message

PR-URL: #3378
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott
Copy link
Member Author

Trott commented Oct 16, 2015

Landed in 676e618

@Trott Trott closed this Oct 16, 2015
@MylesBorins
Copy link
Contributor

@Trott assert.fail didn't change after 4.2.x right?

This should probably be backported to LTS

/cc @jasnell

@rvagg rvagg mentioned this pull request Oct 21, 2015
Trott added a commit that referenced this pull request Oct 26, 2015
The assert.fail function signature has the message as the third argument
but, understandably, it is often assumed that it is the first argument
(or at least the first argument if no other arguments are passed).

This corrects the assert.fail() invocations in the Node.js tests.

Before:
assert.fail('message');
// result: AssertionError: 'message' undefined undefined

After:
assert.fail(null, null, 'message');
// result: AssertionError: message

PR-URL: #3378
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

Landed on v4.x-staging on d540666

Trott added a commit that referenced this pull request Oct 29, 2015
The assert.fail function signature has the message as the third argument
but, understandably, it is often assumed that it is the first argument
(or at least the first argument if no other arguments are passed).

This corrects the assert.fail() invocations in the Node.js tests.

Before:
assert.fail('message');
// result: AssertionError: 'message' undefined undefined

After:
assert.fail(null, null, 'message');
// result: AssertionError: message

PR-URL: #3378
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott Trott deleted the assert.fail branch January 13, 2022 22:29
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.

4 participants