Skip to content

Conversation

nadrane
Copy link
Contributor

@nadrane nadrane commented Dec 21, 2017

I got this project through @Trott on http://nodetodo.org/. Thank you for reviewing it.

Here's a description of the changes:

assert.strictEqual accepts 3 arguments, the last of which allows for
user-specified error message to be thrown when the assertion fails.
Unfortunately, this error message is less helpful than the default when it
is vague. This commit removes vague, user-specified error messages, instead relying on
clearer, default error messages.

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)

tests

assert.strictEqual accepts 3 arguments, the last of which allows for
user-specified error message to be thrown when the assertion fails.
Unfortunately, this error message is less helpful than the default when it
is vague. This commit removes vague, user-specified error messages, instead relying on
clearer, default error messages.
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Dec 21, 2017
@Trott
Copy link
Member

Trott commented Dec 22, 2017

@Trott
Copy link
Member

Trott commented Dec 22, 2017

(Lone CI build failure is clearly unrelated.)

@maclover7 maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 22, 2017

error = test_error.createRangeError();
assert.ok(error instanceof RangeError,
'expected error to be an instance of RangeError');
assert.strictEqual(error.message,
'range error',
'expected message to be "range error"');
'range error');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you please put this (and the others) on the same line now that it doesn't exceed 80 chars?

@Trott
Copy link
Member

Trott commented Dec 23, 2017

@Trott
Copy link
Member

Trott commented Dec 25, 2017

Sole CI failure is unrelated build issue. This is good to land.

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 25, 2017
assert.strictEqual accepts 3 arguments, the last of which allows for
user-specified error message to be thrown when the assertion fails.
Unfortunately, this error message is less helpful than the default when
it is vague. This commit removes vague, user-specified error messages,
instead relying on clearer, default error messages.

PR-URL: nodejs#17812
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@Trott
Copy link
Member

Trott commented Dec 25, 2017

Landed in 094d92b .

Thanks for the contribution! 🎉

@Trott Trott closed this Dec 25, 2017
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
assert.strictEqual accepts 3 arguments, the last of which allows for
user-specified error message to be thrown when the assertion fails.
Unfortunately, this error message is less helpful than the default when
it is vague. This commit removes vague, user-specified error messages,
instead relying on clearer, default error messages.

PR-URL: #17812
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
assert.strictEqual accepts 3 arguments, the last of which allows for
user-specified error message to be thrown when the assertion fails.
Unfortunately, this error message is less helpful than the default when
it is vague. This commit removes vague, user-specified error messages,
instead relying on clearer, default error messages.

PR-URL: #17812
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
assert.strictEqual accepts 3 arguments, the last of which allows for
user-specified error message to be thrown when the assertion fails.
Unfortunately, this error message is less helpful than the default when
it is vague. This commit removes vague, user-specified error messages,
instead relying on clearer, default error messages.

PR-URL: #17812
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
assert.strictEqual accepts 3 arguments, the last of which allows for
user-specified error message to be thrown when the assertion fails.
Unfortunately, this error message is less helpful than the default when
it is vague. This commit removes vague, user-specified error messages,
instead relying on clearer, default error messages.

PR-URL: nodejs#17812
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
assert.strictEqual accepts 3 arguments, the last of which allows for
user-specified error message to be thrown when the assertion fails.
Unfortunately, this error message is less helpful than the default when
it is vague. This commit removes vague, user-specified error messages,
instead relying on clearer, default error messages.

PR-URL: nodejs#17812
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
assert.strictEqual accepts 3 arguments, the last of which allows for
user-specified error message to be thrown when the assertion fails.
Unfortunately, this error message is less helpful than the default when
it is vague. This commit removes vague, user-specified error messages,
instead relying on clearer, default error messages.

Backport-PR-URL: #19447
PR-URL: #17812
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
MylesBorins pushed a commit that referenced this pull request May 1, 2018
assert.strictEqual accepts 3 arguments, the last of which allows for
user-specified error message to be thrown when the assertion fails.
Unfortunately, this error message is less helpful than the default when
it is vague. This commit removes vague, user-specified error messages,
instead relying on clearer, default error messages.

Backport-PR-URL: #19265
PR-URL: #17812
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants