assert.fail() accept a single argument or two arguments #12293

Trott · 2017-04-09T17:56:54Z

First commit:

assert.fail() has two possible function signatures, both of which are
not intuitive. It virtually guarantees that people who try to use
assert.fail() without carefully reading the docs will end up using it
incorrectly.

This change maintains backwards compatibility with the two valid uses
(arguments 1 2 and 4 supplied but argument 3 falsy, and argument 3
supplied but arguments 1 2 and 4 all falsy) but also adds the far more
intuitive first-argument-only and first-two-arguments-only
possibilities.

assert.fail('boom');
// AssertionError: boom

assert.fail('a', 'b');
// AssertionError: 'a' != 'b'

Second commit: Remove lint rule that flags use of assert.fail() with a single argument.

Third commit: Remove common.fail() in tests since assert.fail() with a single argument works as expected.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

refack · 2017-04-09T18:03:21Z

Isn't this semver-major? a change in a stable API?

Trott · 2017-04-09T18:09:36Z

Isn't this semver-major? a change in a stable API?

I won't argue if someone thinks it's major. But to answer the question from my view: I think it's minor because using the current API with only one or two arguments will provide a message that I would describe as broken.

assert.fail('one arg') results in AssertionError: 'one arg' undefined undefined. With this change, it results in AssertionError: one arg.

assert.fail(1, 2) results in AssertionError: 1 undefined 2. With this change, it results in AssertionError: 1 != 2.

It's hard for me to imagine a situation where the current behavior is desirable or expected.

Trott · 2017-04-09T18:14:04Z

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

aqrln · 2017-04-09T18:18:05Z

A typo in the message of the first commit:

supplied but arguments 1 2 and 4 all flasy) but also adds the far more

refack · 2017-04-09T18:29:14Z

It's hard for me to imagine a situation where that behavior is desirable or expected.

Code in the wild got used to it (maybe someone tests the exception text or whatever other silly things people do), I agree you only change the message, but it's not a "Bug Fix" per se.

"not an actual bug fix" ⚖ "change only in message text"

Although I'm an anarchist at heart, and am pro change, I would vote major

Anyway this discussion is kinda mute since v8 (ohh hello ambiguity 🤦) is just around the corner...

gibfahn · 2017-04-09T18:37:19Z

Anyway this discussion is kinda mute since v8 (ohh hello ambiguity 🤦) is just around the corner...

The plan with that was to call it Node 8, as it's as easy to say as v8 and a whole lot clearer. There's an issue discussing it somewhere.

refack · 2017-04-09T18:45:58Z

Node 8

Nodate isn't that a verb? Not that Nodge is unambiguous (darn it Ryan. And Douglas 😩)
~~BTW Nod (נוד) is Hebrew slang for [redacted by author, if you're curious google it]...~~

refack · 2017-04-09T19:52:53Z

test/windows-fanned — running tests

Yay, I just learned how to retrigger just windows-fanned. One small step...

Trott · 2017-04-09T20:47:30Z

For what it's worth:

I agree it's not just a bug fix. It's a new feature at a minimum. So semver-minor at a minimum. But yeah, I said I wouldn't argue against major if others thought it was major, so I'll stop there.
I've been sticking to "version 8" although I know "Node.js 8" has its proponents too. Whatever the shortcomings of these things, all of them are better than calling the Node.js version "v8". :-D

Trott · 2017-04-09T20:48:48Z

@aqrln Typo in commit message fixed. Thanks!

addaleax · 2017-04-10T15:53:12Z

lib/assert.js

+  if (arguments.length === 1)
+    message = actual;
+  if (arguments.length === 2)
+    operator = '!=';


You could just write message = actual, operator = '!=', stackStartFunction = undefined as part of the function arguments, right?

There are at least three issues with that.

First, that introduces additional behavior changes I was hoping to avoid.

Current behavior preserved in this PR:

> assert.fail() AssertionError: undefined undefined undefined

With the default parameters change:

> assert.fail() AssertionError: undefined != undefined

Second, it breaks the two-argument feature added here.

Current PR:

> assert.fail('first', 'second'); AssertionError: 'first' != 'second'

With the suggested change:

> assert.fail('first', 'second') AssertionError: first

Third, and most importantly, it breaks one of the two current usable argument combinations for the function.

Current behavior also preserved in this PR:

> assert.fail('first', 'second', undefined, 'operator') AssertionError: 'first' operator 'second'

With the default parameters as suggested replacing the arguments.length checks:

> assert.fail('first', 'second', undefined, 'operator') AssertionError: first

There's no doubt a way around this, and maybe the problem is my lack of familiarity with default parameters?

addaleax · 2017-04-10T15:54:04Z

test/parallel/test-assert-fail.js

+
+// no third arg (but a fourth arg)
+assert.throws(
+  assert.fail.bind(assert, 'first', 'second', undefined, 'operator'),


I’d personally prefer arrow functions over .bind for all of these… no strong opinion, though :)

I don't have a strong opinion either so I'll change it to arrow functions. :-D

assert.fail() has two possible function signatures, both of which are not intuitive. It virtually guarantees that people who try to use assert.fail() without carefully reading the docs will end up using it incorrectly. This change maintains backwards compatibility with the two valid uses (arguments 1 2 and 4 supplied but argument 3 falsy, and argument 3 supplied but arguments 1 2 and 4 all falsy) but also adds the far more intuitive first-argument-only and first-two-arguments-only possibilities. assert.fail('boom'); // AssertionError: boom assert.fail('a', 'b'); // AssertionError: 'a' != 'b'

assert.fail() has been updated to accept a single argument so that is no longer an error. Remove lint rule that checks for assert.fail() with a single argument.

sam-github · 2017-09-19T19:06:15Z

I think yes, it touched ~50 test files, if we don't backport it we'll get conflicts soon (or already?) when backporting tests.

sam-github · 2017-09-19T19:42:16Z

Gave a quick look, its got lots of conflicts, so will need some backport TLC if we agree we want it.

BridgeAR · 2017-09-19T19:47:16Z

I am also +1 for backporting. Out of my perspective this use case is the only suitable one for assert.fail() and it is actually weird that it used to use more arguments than one.

refack · 2017-09-19T20:14:06Z

@BridgeAR does #14427 make assert.fail usable in v6.x?

Trott · 2017-09-19T22:36:36Z

Backport #15479

assert.fail() has two possible function signatures, both of which are not intuitive. It virtually guarantees that people who try to use assert.fail() without carefully reading the docs will end up using it incorrectly. This change maintains backwards compatibility with the two valid uses (arguments 1 2 and 4 supplied but argument 3 falsy, and argument 3 supplied but arguments 1 2 and 4 all falsy) but also adds the far more intuitive first-argument-only and first-two-arguments-only possibilities. assert.fail('boom'); // AssertionError: boom assert.fail('a', 'b'); // AssertionError: 'a' != 'b' PR-URL: nodejs#12293 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>

assert.fail() has been updated to accept a single argument so that is no longer an error. Remove lint rule that checks for assert.fail() with a single argument. PR-URL: nodejs#12293 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>

common.fail() was added to paste over issues with assert.fail() function signature. assert.fail() has been updated to accept a single argument so common.fail() is no longer necessary. PR-URL: nodejs#12293 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>

assert.fail() has two possible function signatures, both of which are not intuitive. It virtually guarantees that people who try to use assert.fail() without carefully reading the docs will end up using it incorrectly. This change maintains backwards compatibility with the two valid uses (arguments 1 2 and 4 supplied but argument 3 falsy, and argument 3 supplied but arguments 1 2 and 4 all falsy) but also adds the far more intuitive first-argument-only and first-two-arguments-only possibilities. assert.fail('boom'); // AssertionError: boom assert.fail('a', 'b'); // AssertionError: 'a' != 'b' Backport-PR-URL: #15479 PR-URL: #12293 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>

assert.fail() has been updated to accept a single argument so that is no longer an error. Remove lint rule that checks for assert.fail() with a single argument. Backport-PR-URL: #15479 PR-URL: #12293 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>

common.fail() was added to paste over issues with assert.fail() function signature. assert.fail() has been updated to accept a single argument so common.fail() is no longer necessary. Backport-PR-URL: #15479 PR-URL: #12293 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>

assert.fail() has two possible function signatures, both of which are not intuitive. It virtually guarantees that people who try to use assert.fail() without carefully reading the docs will end up using it incorrectly. This change maintains backwards compatibility with the two valid uses (arguments 1 2 and 4 supplied but argument 3 falsy, and argument 3 supplied but arguments 1 2 and 4 all falsy) but also adds the far more intuitive first-argument-only and first-two-arguments-only possibilities. assert.fail('boom'); // AssertionError: boom assert.fail('a', 'b'); // AssertionError: 'a' != 'b' Backport-PR-URL: #15479 PR-URL: #12293 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>

assert.fail() has been updated to accept a single argument so that is no longer an error. Remove lint rule that checks for assert.fail() with a single argument. Backport-PR-URL: #15479 PR-URL: #12293 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>

common.fail() was added to paste over issues with assert.fail() function signature. assert.fail() has been updated to accept a single argument so common.fail() is no longer necessary. Backport-PR-URL: #15479 PR-URL: #12293 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notable Changes: * assert: - assert.fail() can now take one or two arguments (Rich Trott) #12293 * crypto: - add sign/verify support for RSASSA-PSS (Tobias Nießen) #11705 * deps: - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu) #16691 - upgrade libuv to 1.15.0 (cjihrig) #15745 - upgrade libuv to 1.14.1 (cjihrig) #14866 - upgrade libuv to 1.13.1 (cjihrig) #14117 - upgrade libuv to 1.12.0 (cjihrig) #13306 * fs: - Add support for fs.write/fs.writeSync(fd, buffer, cb) and fs.write/fs.writeSync(fd, buffer, offset, cb) as documented (Andreas Lind) #7856 * inspector: - enable --inspect-brk (Refael Ackermann) #12615 * process: - add --redirect-warnings command line argument (James M Snell) #10116 * src: - allow CLI args in env with NODE_OPTIONS (Sam Roberts) #12028) - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts) #13932 - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts) #13172 - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts) #12677 * test: - remove common.fail() (Rich Trott) #12293 PR-URL: #16263

Trott added assert Issues and PRs related to the assert subsystem. semver-minor PRs that contain new features and should be released in the next minor version. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Apr 9, 2017

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. dont-land-on-v4.x test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Apr 9, 2017

Trott mentioned this pull request Apr 9, 2017

tools: replace custom assert.fail lint rule #12287

Closed

2 tasks

Trott force-pushed the assert-fail-fix branch from 5ee2e1f to 9ce64f3 Compare April 9, 2017 20:48

refack added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 9, 2017

addaleax approved these changes Apr 10, 2017

View reviewed changes

Trott force-pushed the assert-fail-fix branch from 9ce64f3 to d86a553 Compare April 11, 2017 04:11

Trott mentioned this pull request Apr 11, 2017

8.0.0 Release Proposal #12220

Closed

targos approved these changes Apr 12, 2017

View reviewed changes

targos added the ctc-review label Apr 12, 2017

jasnell approved these changes Apr 12, 2017

View reviewed changes

Trott added 2 commits April 12, 2017 13:35

tools: remove assert.fail() lint rule

00bbb98

assert.fail() has been updated to accept a single argument so that is no longer an error. Remove lint rule that checks for assert.fail() with a single argument.

BridgeAR mentioned this pull request Sep 19, 2017

[v6.x backport] doc,assert: document stackStartFunction in fail #14427

Closed

4 tasks

Trott mentioned this pull request Sep 19, 2017

[v6.x backport] assert.fail() accept a single argument or two arguments #15479

Closed

4 tasks

MylesBorins added backported-to-v6.x and removed lts-watch-v6.x labels Oct 10, 2017

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

Merged

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

Trott deleted the assert-fail-fix branch January 13, 2022 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert.fail() accept a single argument or two arguments #12293

assert.fail() accept a single argument or two arguments #12293

Trott commented Apr 9, 2017 •

edited

Loading

refack commented Apr 9, 2017

Trott commented Apr 9, 2017 •

edited

Loading

Trott commented Apr 9, 2017

aqrln commented Apr 9, 2017

refack commented Apr 9, 2017

gibfahn commented Apr 9, 2017

refack commented Apr 9, 2017 •

edited

Loading

refack commented Apr 9, 2017

Trott commented Apr 9, 2017

Trott commented Apr 9, 2017

addaleax Apr 10, 2017

Trott Apr 11, 2017

addaleax Apr 10, 2017

Trott Apr 11, 2017

sam-github commented Sep 19, 2017

sam-github commented Sep 19, 2017

BridgeAR commented Sep 19, 2017

refack commented Sep 19, 2017

Trott commented Sep 19, 2017

assert.fail() accept a single argument or two arguments #12293

assert.fail() accept a single argument or two arguments #12293

Conversation

Trott commented Apr 9, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Apr 9, 2017

Trott commented Apr 9, 2017 • edited Loading

Trott commented Apr 9, 2017

aqrln commented Apr 9, 2017

refack commented Apr 9, 2017

gibfahn commented Apr 9, 2017

refack commented Apr 9, 2017 • edited Loading

refack commented Apr 9, 2017

Trott commented Apr 9, 2017

Trott commented Apr 9, 2017

addaleax Apr 10, 2017

Choose a reason for hiding this comment

Trott Apr 11, 2017

Choose a reason for hiding this comment

addaleax Apr 10, 2017

Choose a reason for hiding this comment

Trott Apr 11, 2017

Choose a reason for hiding this comment

sam-github commented Sep 19, 2017

sam-github commented Sep 19, 2017

BridgeAR commented Sep 19, 2017

refack commented Sep 19, 2017

Trott commented Sep 19, 2017

Trott commented Apr 9, 2017 •

edited

Loading

Trott commented Apr 9, 2017 •

edited

Loading

refack commented Apr 9, 2017 •

edited

Loading