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.

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

assert: add default operator to assert.fail() #22694

Closed
wants to merge 2 commits into from
Closed

assert: add default operator to assert.fail() #22694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f92573f
assert: add default operator to `assert.fail()`
BridgeAR Sep 4, 2018
dd512bc
fixup: strictor operator check
BridgeAR Sep 11, 2018
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
21 changes: 16 additions & 5 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,9 @@ function innerFail(obj) {
function fail(actual, expected, message, operator, stackStartFn) {
const argsLen = arguments.length;

let internalMessage;
if (argsLen === 0) {
message = 'Failed';
internalMessage = 'Failed';
} else if (argsLen === 1) {
message = actual;
actual = undefined;
Expand All @@ -106,13 +107,23 @@ function fail(actual, expected, message, operator, stackStartFn) {
operator = '!=';
}

innerFail({
if (message instanceof Error) throw message;

Copy link
Member

Choose a reason for hiding this comment

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

☝️ Should use the util for isError (internal reference) for this since it avoids the error of errors from other realms.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only changing this here is likely not the best way. It would have to be changed throughout assert in total or not at all. It is also sad that the util function is not a safe check as the Symbol.toStringTag could be set and isError would happily return true in some weird cases.

Copy link
Member

Choose a reason for hiding this comment

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

Only changing this here is likely not the best way.

Ah, didn't know this was consistent with the rest of assert. A follow up would be best.

It is also sad that the util function is not a safe check as the Symbol.toStringTag could be set and isError would happily return true in some weird cases.

Oh I thought the new isNativeError was more robust.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I missed the isNativeError somehow. We have multiple places in core where we should likely switch to this check.

const errArgs = {
actual,
expected,
message,
operator,
operator: operator === undefined ? 'fail' : operator,
stackStartFn: stackStartFn || fail
});
};
if (message !== undefined) {
errArgs.message = message;
}
const err = new AssertionError(errArgs);
if (internalMessage) {
err.message = internalMessage;
err.generatedMessage = true;
}
throw err;
}

assert.fail = fail;
Expand Down
8 changes: 5 additions & 3 deletions test/parallel/test-assert-fail-deprecation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ assert.throws(() => {
message: '\'first\' != \'second\'',
operator: '!=',
actual: 'first',
expected: 'second'
expected: 'second',
generatedMessage: true
});

// Three args
Expand All @@ -29,9 +30,10 @@ assert.throws(() => {
code: 'ERR_ASSERTION',
name: 'AssertionError [ERR_ASSERTION]',
message: 'another custom message',
operator: undefined,
operator: 'fail',
actual: 'ignored',
expected: 'ignored'
expected: 'ignored',
generatedMessage: false
});

// Three args with custom Error
Expand Down
10 changes: 6 additions & 4 deletions test/parallel/test-assert-fail.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ assert.throws(
code: 'ERR_ASSERTION',
name: 'AssertionError [ERR_ASSERTION]',
message: 'Failed',
operator: undefined,
operator: 'fail',
actual: undefined,
expected: undefined
expected: undefined,
generatedMessage: true
}
);

Expand All @@ -23,9 +24,10 @@ assert.throws(() => {
code: 'ERR_ASSERTION',
name: 'AssertionError [ERR_ASSERTION]',
message: 'custom message',
operator: undefined,
operator: 'fail',
actual: undefined,
expected: undefined
expected: undefined,
generatedMessage: false
});

// One arg = Error
Expand Down