assert: improve the strict equal messages #23056

BridgeAR · 2018-09-24T12:23:47Z

In case reference (un)equal objects fail in assertion, it should be
clear that it is about the reference equality and not about the
object properties. This is fixed by improving the message in such
cases.

Refs: #22763

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

In case reference (un)equal objects fail in assertion, it should be clear that it is about the reference equality and not about the object properties. This is fixed by improving the message in such cases. Refs: nodejs#22763

nodejs-github-bot · 2018-09-24T12:23:49Z

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/995/pipeline

antsmartian · 2018-09-24T12:38:57Z

lib/internal/assert.js

@@ -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:',


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.

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 :)

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?

No strong opinion)

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

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.

BridgeAR · 2018-09-25T09:19:51Z

~~CI https://ci.nodejs.org/job/node-test-pull-request/17425/~~
CI https://ci.nodejs.org/job/node-test-pull-request/17426/

The wording seems clearer when using `values` instead of `inputs`.

BridgeAR · 2018-09-25T15:49:21Z

Resumed build: https://ci.nodejs.org/job/node-test-pull-request/17430/ ✔️ (besides Windows)

@nodejs/util PTAL

BridgeAR · 2018-09-26T07:53:40Z

Windows rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/21037/

mcollina · 2018-09-26T08:31:24Z

I think this is semver-major, as the change to those message could lead to test breakage in the ecosystem.

mcollina

LGTM

BridgeAR · 2018-09-26T08:37:11Z

@mcollina any change to the message could theoretically break the ecosystem but we did not handle it that way in almost all cases. This was an issue in a few cases before but all known ones were already fixed not to rely on the message in that way anymore. So if at all, it would already be broken and should not break any more. That's why I do not think that it's semver-major.

BridgeAR · 2018-09-26T08:37:27Z

@nodejs/tsc PTAL

mcollina · 2018-09-26T08:44:03Z

Ok

mcollina · 2018-09-26T08:44:26Z

Can you please confirm with a CITGM run?

BridgeAR · 2018-09-26T08:53:21Z

CITGM (this PR) https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1561/
CITGM (other PR) https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1560

(when looking at the current citgm failures it seems like we introduced something bad recently since the failures were normally significantly lower)

Update: I did not find anything but it is difficult to look through everything due to the many failures in general at the moment.

Trott · 2018-09-26T12:28:06Z

The wording changes look good to me.

ryzokuken

LGTM.

BridgeAR · 2018-09-26T14:31:45Z

Rebuild Windows again: https://ci.nodejs.org/job/node-test-commit-windows-fanned/21043/

BridgeAR · 2018-09-26T20:26:33Z

Rebuild Windows again: https://ci.nodejs.org/job/node-test-commit-windows-fanned/21052/ ✔️

In case reference (un)equal objects fail in assertion, it should be clear that it is about the reference equality and not about the object properties. This is fixed by improving the message in such cases. Refs: nodejs#22763 PR-URL: nodejs#23056 Refs: nodejs#22763 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

The wording seems clearer when using `values` instead of `inputs`. PR-URL: nodejs#23056 Refs: nodejs#22763 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

BridgeAR · 2018-09-27T11:34:40Z

Landed in be26c76 and b8a8eed.

BethGriggs · 2019-07-24T13:42:21Z

@BridgeAR, is this still something that should be backported to v10.x?

assert: improve the strict equal messages

b7def33

In case reference (un)equal objects fail in assertion, it should be clear that it is about the reference equality and not about the object properties. This is fixed by improving the message in such cases. Refs: nodejs#22763

BridgeAR requested review from addaleax and targos September 24, 2018 12:23

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Sep 24, 2018

antsmartian reviewed Sep 24, 2018

View reviewed changes

fixup: address comments

7b5ad04

assert: switch inputs to values

8532f22

The wording seems clearer when using `values` instead of `inputs`.

BridgeAR force-pushed the improve-strict-equal-message branch from d7c33aa to 8532f22 Compare September 25, 2018 10:48

joyeecheung mentioned this pull request Sep 25, 2018

CI failures: 20180925 nodejs/reliability#17

Closed

BridgeAR requested review from mcollina, bcoe, benjamingr, jasnell, jdalton and ryzokuken September 26, 2018 07:56

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 26, 2018

mcollina approved these changes Sep 26, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 26, 2018

mcollina removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 26, 2018

ryzokuken approved these changes Sep 26, 2018

View reviewed changes

thefourtheye approved these changes Sep 26, 2018

View reviewed changes

lpinca approved these changes Sep 26, 2018

View reviewed changes

BridgeAR closed this Sep 27, 2018

targos added the backport-requested-v10.x label Sep 27, 2018

BridgeAR deleted the improve-strict-equal-message branch January 20, 2020 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: improve the strict equal messages #23056

assert: improve the strict equal messages #23056

BridgeAR commented Sep 24, 2018

nodejs-github-bot commented Sep 24, 2018

antsmartian Sep 24, 2018

BridgeAR Sep 24, 2018

addaleax Sep 24, 2018

vsemozhetbyt Sep 24, 2018

Trott Sep 24, 2018

BridgeAR Sep 25, 2018

BridgeAR commented Sep 25, 2018 •

edited

Loading

BridgeAR commented Sep 25, 2018 •

edited

Loading

BridgeAR commented Sep 26, 2018

mcollina commented Sep 26, 2018

mcollina left a comment

BridgeAR commented Sep 26, 2018

BridgeAR commented Sep 26, 2018

mcollina commented Sep 26, 2018

mcollina commented Sep 26, 2018

BridgeAR commented Sep 26, 2018 •

edited

Loading

Trott commented Sep 26, 2018

ryzokuken left a comment

BridgeAR commented Sep 26, 2018

BridgeAR commented Sep 26, 2018 •

edited

Loading

BridgeAR commented Sep 27, 2018

BethGriggs commented Jul 24, 2019

assert: improve the strict equal messages #23056

assert: improve the strict equal messages #23056

Conversation

BridgeAR commented Sep 24, 2018

Checklist

nodejs-github-bot commented Sep 24, 2018

antsmartian Sep 24, 2018

Choose a reason for hiding this comment

BridgeAR Sep 24, 2018

Choose a reason for hiding this comment

addaleax Sep 24, 2018

Choose a reason for hiding this comment

vsemozhetbyt Sep 24, 2018

Choose a reason for hiding this comment

Trott Sep 24, 2018

Choose a reason for hiding this comment

BridgeAR Sep 25, 2018

Choose a reason for hiding this comment

BridgeAR commented Sep 25, 2018 • edited Loading

BridgeAR commented Sep 25, 2018 • edited Loading

BridgeAR commented Sep 26, 2018

mcollina commented Sep 26, 2018

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Sep 26, 2018

BridgeAR commented Sep 26, 2018

mcollina commented Sep 26, 2018

mcollina commented Sep 26, 2018

BridgeAR commented Sep 26, 2018 • edited Loading

Trott commented Sep 26, 2018

ryzokuken left a comment

Choose a reason for hiding this comment

BridgeAR commented Sep 26, 2018

BridgeAR commented Sep 26, 2018 • edited Loading

BridgeAR commented Sep 27, 2018

BethGriggs commented Jul 24, 2019

BridgeAR commented Sep 25, 2018 •

edited

Loading

BridgeAR commented Sep 25, 2018 •

edited

Loading

BridgeAR commented Sep 26, 2018 •

edited

Loading

BridgeAR commented Sep 26, 2018 •

edited

Loading