Skip to content

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

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
BridgeAR wants to merge 2 commits into from
+27 −12
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 numberDiff line numberDiff 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;

@jdaltonjdaltonSep 12, 2018

Copy link
Copy Markdown
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.

@BridgeARBridgeARSep 12, 2018

Copy link
Copy Markdown
MemberAuthor

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.

@jdaltonjdaltonSep 12, 2018

Copy link
Copy Markdown
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.

@BridgeARBridgeARSep 12, 2018

Copy link
Copy Markdown
MemberAuthor

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 numberDiff line numberDiff 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 numberDiff line numberDiff 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