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

util: assert: fix deepStrictEqual comparing a real array and fake array #30743

Closed
wants to merge 1 commit into from

Conversation

@ljharb
Copy link
Member

ljharb commented Dec 1, 2019

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

(following in the tradition of #29029)

@ljharb ljharb added the util label Dec 1, 2019
@ljharb ljharb requested review from Trott, targos and devsnek Dec 1, 2019
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 1, 2019

@nodejs/assert

@ljharb ljharb force-pushed the ljharb:deep_equal_arg_order branch from 684d1ce to 81be8ab Dec 1, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this pull request Dec 1, 2019
 - Skip one, pending nodejs/node#30743
@devsnek devsnek removed their request for review Dec 1, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this pull request Dec 1, 2019
 - Skip one, pending nodejs/node#30743
@ljharb ljharb force-pushed the ljharb:deep_equal_arg_order branch from 81be8ab to 3d7c1c7 Dec 1, 2019
@ljharb ljharb requested a review from addaleax Dec 1, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this pull request Dec 1, 2019
 - Skip one, pending nodejs/node#30743
ljharb added a commit to inspect-js/node-deep-equal that referenced this pull request Dec 1, 2019
 - Skip one, pending nodejs/node#30743
@@ -1064,7 +1079,7 @@ assert.throws(
Object.defineProperty(a, 'length', {
value: 2
});
assert.notDeepStrictEqual(a, [1, 1]);
assertOnlyDeepEqual(a, [1, 1]);

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 1, 2019

Author Member

this was the failing test, because it tries the arguments in reverse order

@@ -1050,6 +1050,21 @@ assert.throws(
assert.deepStrictEqual(a, b);
}

// Verify that an array and the equivalent fake array object

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 1, 2019

Author Member

this test was previously passing, but it felt like a good addition anyways.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 1, 2019

Does this require an update to the documented deepStrictEqual() algorithm in our documentation? https://github.com/nodejs/node/blob/d25db11312e9ef4392042380b812359fad103707/doc/api/assert.md#comparison-details-1

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 2, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@d7b8ae7). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #30743   +/-   ##
=========================================
  Coverage          ?   97.36%           
=========================================
  Files             ?      187           
  Lines             ?    62171           
  Branches          ?        0           
=========================================
  Hits              ?    60532           
  Misses            ?     1639           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7b8ae7...66a3dc4. Read the comment docs.

@ljharb

This comment has been minimized.

Copy link
Member Author

ljharb commented Dec 2, 2019

@Trott all this change does is makes sure that what already works for (a, b) also works for (b, a); i don't believe any part of that documentation needs to change.

@Trott Trott requested a review from BridgeAR Dec 2, 2019
@BridgeAR BridgeAR mentioned this pull request Dec 2, 2019
4 of 4 tasks complete
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 2, 2019

I originally decided against doing full two sides checks because it's a very rare edge case and we can not check both sides for all types (we could do that for built-in types but that would be a lot of overhead).

I am fine to implement this at least for the currently used one sided type checks. That way it's always consistent, no matter what argument is first.

It would be good to keep the performance overhead in mind though. Especially the typed array type check is relatively expensive.

@ljharb I went ahead and opened #30764 as alternative to this one as it addresses all open one sides versions with the least overhead possible.

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>
@ljharb

This comment has been minimized.

Copy link
Member Author

ljharb commented Dec 7, 2019

Now that #30764 has landed, I'll update or close this PR soon, based on if there's anything of value left in it.

@ljharb ljharb force-pushed the ljharb:deep_equal_arg_order branch from 3d7c1c7 to de24d40 Dec 8, 2019
@ljharb

This comment has been minimized.

Copy link
Member Author

ljharb commented Dec 8, 2019

I updated the PR; there's just a single test case left. Feel free to land or close.

@ljharb ljharb added the test label Dec 8, 2019
@Trott
Trott approved these changes Dec 8, 2019
@ljharb ljharb force-pushed the ljharb:deep_equal_arg_order branch from de24d40 to 66a3dc4 Dec 9, 2019
@targos
targos approved these changes Dec 9, 2019
@nodejs-github-bot

This comment has been minimized.

targos added 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>
targos added a commit that referenced this pull request Dec 9, 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>
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 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: nodejs#30764
Refs: nodejs#30743
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 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: nodejs#30764
Refs: nodejs#30743
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@bmeck bmeck added the author ready label Dec 11, 2019
bmeck added a commit that referenced this pull request Dec 11, 2019
PR-URL: #30743
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Dec 11, 2019

Landed in 916cc82

@bmeck bmeck closed this Dec 11, 2019
@ljharb ljharb deleted the ljharb:deep_equal_arg_order branch Dec 11, 2019
MylesBorins added a commit that referenced this pull request Dec 13, 2019
PR-URL: #30743
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.