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,util: always verify both sides #30764

Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 2, 2019

Please have a look at the commit messages for details.

Refs: #30743

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

This veryfies that both input arguments are always of the identical
type. It was possible to miss a few cases before. This change applies
to all deep equal assert functions (e.g., `assert.deepStrictEqual()`)
and to `util.isDeepStrictEqual()`.
This makes sure the assertion does not depend on the argument order.
It also removes comments that do not apply anymore and verifies the
behavior for the loose and strict implementation.
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Dec 2, 2019
@BridgeAR BridgeAR requested a review from ljharb December 2, 2019 14:36
@BridgeAR BridgeAR changed the title assert: always verify both sides assert,util: always verify both sides Dec 2, 2019
@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 2, 2019
if (isDate(val1)) {
if (DatePrototypeGetTime(val1) !== DatePrototypeGetTime(val2)) {
} else if (isDate(val1)) {
if (!isDate(val2) ||
Copy link
Member

Choose a reason for hiding this comment

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

there's no failing tests that necessitate any change to the Date or RegExp or Error or boxed primitive logic - can you add those tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Boxed primitives already have such test case.

I could add more tests for the other types but I actually like things like these to keep them around for coverage tasks at Code and Learn events. I am also fine to add the tests though, if you think that should be done in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

If there's that much of a shortage of tasks for those events, then so be it, but it seems unwise to make untested changes.

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 just added multiple tests and I indeed forgot to add one check 😆
There are a couple more cases that are not yet completely covered: Set, Map, ArrayBuffer and non-native errors.

@@ -1123,3 +1116,33 @@ assert.throws(
// The descriptor is not compared.
assertDeepAndStrictEqual(a, { a: 5 });
}

// Verify object types being identical on both sides.
{
Copy link
Member

Choose a reason for hiding this comment

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

these are the only tests that fail without your change

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is correct. The second commit is just there to prevent any further issues like these. It's somewhat independent but it fit together. Would you rather separate the two commits into two PRs?

Copy link
Member

Choose a reason for hiding this comment

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

nah this is fine to me, was highlighting here in reference to #30764 (comment)

if (!isEqualBoxedPrimitive(val1, val2)) {
return false;
}
} else if (ArrayIsArray(val2) ||
Copy link
Member

Choose a reason for hiding this comment

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

why check isArray of val2 way down here, instead of only up by the place where val1 is checked (as in #30743)?

Copy link
Member Author

@BridgeAR BridgeAR Dec 2, 2019

Choose a reason for hiding this comment

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

It's less CPU work.

In #30743 all entries have to check both sides of both types. But if the arguments are e.g., sets, we'd only have to know that one side is not a set and then continue until we match sets. There we verify the other side.
If it's neither of the explicitly listed entries, we'll verify that the right side is none of the already tested types.

ljharb added a commit to inspect-js/node-deep-equal that referenced this pull request Dec 2, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this pull request Dec 3, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this pull request Dec 3, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this pull request Dec 3, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this pull request Dec 3, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this pull request Dec 3, 2019
@ljharb
Copy link
Member

ljharb commented Dec 3, 2019

This LGTM as an alternative to #30743; once this lands, I'll either update my PR (if there's more tests worth adding) or close it (if there's no longer anything to add).

@nodejs-github-bot

This comment has been minimized.

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2019

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2110/ (everything seems normal).

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 6, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/27406/ ✅ (yellow build with a single Windows flake. It passed in another CI run before)

BridgeAR added a commit that referenced this pull request Dec 6, 2019
This veryfies that both input arguments are always of the identical
type. It was possible to miss a few cases before. This change applies
to all deep equal assert functions (e.g., `assert.deepStrictEqual()`)
and to `util.isDeepStrictEqual()`.

PR-URL: #30764
Refs: #30743
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit that referenced this pull request Dec 6, 2019
This makes sure the assertion does not depend on the argument order.
It also removes comments that do not apply anymore and verifies the
behavior for the loose and strict implementation.

PR-URL: #30764
Refs: #30743
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2019

Landed in b4d48c0, b5f2942 🎉

@BridgeAR BridgeAR closed this Dec 6, 2019
targos pushed a commit that referenced this pull request Dec 9, 2019
This veryfies that both input arguments are always of the identical
type. It was possible to miss a few cases before. This change applies
to all deep equal assert functions (e.g., `assert.deepStrictEqual()`)
and to `util.isDeepStrictEqual()`.

PR-URL: #30764
Refs: #30743
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants