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

Jump to bottom

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 1 commit
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 || 'fail',
Copy link
Member

Choose a reason for hiding this comment

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

Does Node have a consistent way of checking for optional params or default params?
In this case the falsey check means that users can't specify it as null or "".

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's often mixed and fasly values are often set to a default while in other cases only undefined is set to a default.

Copy link
Member

Choose a reason for hiding this comment

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

This method has a mix of deprecated/legacy and new/different functionality. Would moving the operator juggle into a argLength < 2 branch with the other argLen checks be doable? That way it touches less of the overlapping code paths.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was done on purpose since I believe it is best to just properly do this for all cases.

Shall I maybe change the check to: 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