Skip to content

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

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

Closed
Trott wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e4e98ba
assert: improve assert.fail() API
Trott Apr 9, 2017
00bbb98
tools: remove assert.fail() lint rule
Trott Apr 12, 2017
ec6c10e
test: remove common.fail()
Trott Apr 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
assert: improve assert.fail() API 
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'
  • Loading branch information
@Trott
Trott committed Apr 12, 2017
commit e4e98ba7e8ff3118973a1f9516fe16c0c7b8e19a
9 changes: 8 additions & 1 deletion doc/api/assert.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,14 +256,15 @@ If the values are not equal, an `AssertionError` is thrown with a `message`
property set equal to the value of the `message` parameter. If the `message`
parameter is undefined, a default error message is assigned.

## assert.fail(message)
## assert.fail(actual, expected, message, operator)
<!-- YAML
added: v0.1.21
-->
* `actual` {any}
* `expected` {any}
* `message` {any}
* `operator` {string}
* `operator` {string} (default: '!=')

Throws an `AssertionError`. If `message` is falsy, the error message is set as
the values of `actual` and `expected` separated by the provided `operator`.
Expand All @@ -277,6 +278,12 @@ assert.fail(1, 2, undefined, '>');

assert.fail(1, 2, 'whoops', '>');
// AssertionError: whoops

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

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

## assert.ifError(value)
Expand Down
4 changes: 4 additions & 0 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ function getMessage(self) {
// display purposes.

function fail(actual, expected, message, operator, stackStartFunction) {
if (arguments.length === 1)
message = actual;
if (arguments.length === 2)
operator = '!=';
Copy link
Copy Markdown
Member

@addaleax addaleax Apr 10, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@Trott Trott Apr 11, 2017

Choose a reason for hiding this comment

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

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?

throw new assert.AssertionError({
message: message,
actual: actual,
Expand Down
33 changes: 33 additions & 0 deletions test/parallel/test-assert-fail.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
'use strict';
require('../common');
const assert = require('assert');

// no args
assert.throws(
() => { assert.fail(); },
/^AssertionError: undefined undefined undefined$/
);

// one arg = message
assert.throws(
() => { assert.fail('custom message'); },
/^AssertionError: custom message$/
);

// two args only, operator defaults to '!='
assert.throws(
() => { assert.fail('first', 'second'); },
/^AssertionError: 'first' != 'second'$/
);

// three args
assert.throws(
() => { assert.fail('ignored', 'ignored', 'another custom message'); },
/^AssertionError: another custom message$/
);

// no third arg (but a fourth arg)
assert.throws(
() => { assert.fail('first', 'second', undefined, 'operator'); },
/^AssertionError: 'first' operator 'second'$/
);