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.

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: improve the strict equal messages #23056

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
31 changes: 26 additions & 5 deletions lib/internal/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ let white = '';
const kReadableOperator = {
deepStrictEqual: 'Expected inputs to be strictly deep-equal:',
strictEqual: 'Expected inputs to be strictly equal:',
strictEqualObject: 'Expected reference equal inputs but got:',
Copy link
Contributor

Choose a reason for hiding this comment

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

One small suggestion here, can't we have:

Expected "actual" reference to be equal to "expected" but got:. For me the word inputs (why we use plural word here?) is bit confusing and also the counter part message (notStrictEqualObject) uses the term "actual" and "expected", so it would make sense to use the same here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right that either notStrictEqualObject should be 'Expected inputs not to be reference equal:' or that this message should be changed.

I used inputs instead of naming the variables because the message itself is significantly shorter that way. But I personally have no strong opinion on it. Maybe others can weight in? :)

@Trott @vsemozhetbyt it would be good if you could also have a look :)

Copy link
Member

Choose a reason for hiding this comment

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

If the current wording remains, maybe this should be reference-equal since it’s used as a single adjective? I’ve also always been wondering about inputs in these error messages … it makes sense from the PoV of the assert module, but for me something more generic like values would seem more intuitive?

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion)

Copy link
Member

Choose a reason for hiding this comment

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

No strong opinion from me either. Maybe @nodejs/documentation even though this isn't a doc question?

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to go for a more verbose version and I also added the dash in reference-equal.

I also added another commit to switch from inputs to values.

deepEqual: 'Expected inputs to be loosely deep-equal:',
equal: 'Expected inputs to be loosely equal:',
notDeepStrictEqual: 'Expected "actual" not to be strictly deep-equal to:',
notStrictEqual: 'Expected "actual" to be strictly unequal to:',
// eslint-disable-next-line max-len
notStrictEqualObject: 'Expected "actual" not to be reference equal to "expected":',
notDeepEqual: 'Expected "actual" not to be loosely deep-equal to:',
notEqual: 'Expected "actual" to be loosely unequal to:',
notIdentical: 'Inputs identical but not reference equal:',
Expand Down Expand Up @@ -67,13 +70,20 @@ function createErrDiff(actual, expected, operator) {
const actualInspected = inspectValue(actual);
const actualLines = actualInspected.split('\n');
const expectedLines = inspectValue(expected).split('\n');
const msg = kReadableOperator[operator] +
`\n${green}+ actual${white} ${red}- expected${white}`;
const skippedMsg = ` ${blue}...${white} Lines skipped`;

let i = 0;
let indicator = '';

// In case both inputs are objects explicitly mark them as not reference equal
// for the `strictEqual` operator.
if (operator === 'strictEqual' &&
typeof actual === 'object' &&
typeof expected === 'object' &&
actual !== null &&
expected !== null) {
operator = 'strictEqualObject';
}

// If "actual" and "expected" fit on a single line and they are not strictly
// equal, check further special handling.
if (actualLines.length === 1 && expectedLines.length === 1 &&
Expand All @@ -89,7 +99,7 @@ function createErrDiff(actual, expected, operator) {
return `${kReadableOperator[operator]}\n\n` +
`${actualLines[0]} !== ${expectedLines[0]}\n`;
}
} else {
} else if (operator !== 'strictEqualObject') {
// If the stderr is a tty and the input length is lower than the current
// columns per line, add a mismatch indicator below the output. If it is
// not a tty, use a default value of 80 characters.
Expand Down Expand Up @@ -156,6 +166,10 @@ function createErrDiff(actual, expected, operator) {
}

let printedLines = 0;
const msg = kReadableOperator[operator] +
`\n${green}+ actual${white} ${red}- expected${white}`;
const skippedMsg = ` ${blue}...${white} Lines skipped`;

for (i = 0; i < maxLines; i++) {
// Only extra expected lines exist
const cur = i - lastPos;
Expand Down Expand Up @@ -274,8 +288,15 @@ class AssertionError extends Error {
operator === 'notStrictEqual') {
// In case the objects are equal but the operator requires unequal, show
// the first object and say A equals B
let base = kReadableOperator[operator];
const res = inspectValue(actual).split('\n');
const base = kReadableOperator[operator];

// In case "actual" is an object, it should not be reference equal.
if (operator === 'notStrictEqual' &&
typeof actual === 'object' &&
actual !== null) {
base = kReadableOperator.notStrictEqualObject;
}

// Only remove lines in case it makes sense to collapse those.
// TODO: Accept env to always show the full error.
Expand Down
47 changes: 44 additions & 3 deletions test/parallel/test-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -415,9 +415,9 @@ assert.throws(
{
code: 'ERR_ASSERTION',
name: 'AssertionError [ERR_ASSERTION]',
message: strictEqualMessageStart +
message: 'Expected reference equal inputs but got:\n' +
'+ actual - expected\n\n' +
'+ [Error: foo]\n- [Error: foobar]\n ^'
'+ [Error: foo]\n- [Error: foobar]'
}
);

Expand Down Expand Up @@ -1022,7 +1022,8 @@ assert.throws(
assert.throws(
() => assert.strictEqual(args, { 0: 'a' }),
{
message: `${strictEqualMessageStart}+ actual - expected\n\n` +
message: 'Expected reference equal inputs but got:\n' +
'+ actual - expected\n\n' +
"+ [Arguments] {\n- {\n '0': 'a'\n }"
}
);
Expand Down Expand Up @@ -1091,3 +1092,43 @@ assert.throws(
}
);
}

// Indicate where the strings diverge.
assert.throws(
() => assert.strictEqual('test test', 'test foobar'),
{
code: 'ERR_ASSERTION',
name: 'AssertionError [ERR_ASSERTION]',
message: strictEqualMessageStart +
'+ actual - expected\n\n' +
"+ 'test test'\n" +
"- 'test foobar'\n" +
' ^'
}
);

// Check for reference equal objects in `notStrictEqual()`
assert.throws(
() => {
const obj = {};
assert.notStrictEqual(obj, obj);
},
{
code: 'ERR_ASSERTION',
name: 'AssertionError [ERR_ASSERTION]',
message: 'Expected "actual" not to be reference equal to "expected": {}'
}
);

assert.throws(
() => {
const obj = { a: true };
assert.notStrictEqual(obj, obj);
},
{
code: 'ERR_ASSERTION',
name: 'AssertionError [ERR_ASSERTION]',
message: 'Expected "actual" not to be reference equal to "expected":\n\n' +
'{\n a: true\n}\n'
}
);