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

test: add coverage for boxed bigints comparisons and edge cases #25932

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 4, 2019

First commit:

test: add BigInt test for isDeepStrictEqual

Add some BigInt tests for test-util-isDeepStrictEqual to get 100%
coverage for lib/internal/util/comparisons.js.

Second commit:

util: remove unnecessary conditions for isDeepStrictEqual

The internal `isEqualBoxedPrimitive()` checks some conditions that are
not necessary. Remove those conditions.

Second commit:

test: add util.isDeepStrictEqual edge case tests

Test for deep strict equality when prototype and toStringTag
have been modified in surprising ways.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Add some BigInt tests for test-util-isDeepStrictEqual to get 100%
coverage for lib/internal/util/comparisons.js.
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Feb 4, 2019
@Trott Trott requested a review from BridgeAR February 4, 2019 21:23
@Trott Trott closed this Feb 4, 2019
@Trott Trott reopened this Feb 4, 2019
@Trott
Copy link
Member Author

Trott commented Feb 4, 2019

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

The first commit seems fine.

The checks however are not obsolete. Removing them would cause some input to throw a wrong error.

const a = new Number(5)
const b = new String(5)
Object.defineProperty(b, Symbol.toStringTag, { value: 'Number' })
Object.setPrototypeOf(b, Number.prototype)
util.isDeepStrictEqual(a, b)
// TypeError: Number.prototype.valueOf requires that 'this' be a Number

@Trott
Copy link
Member Author

Trott commented Feb 4, 2019

/ping @bcoe There seems to be a (known?) bug in the coverage reports. The code I removed from the ternaries in this PR always evaluates to true in our tests. But coverage shows them as fully covered branches. I would expect it to only do that if there are tests that cause the condition to be true and tests that cause the condition to be false.

EDIT: Since I updated the PR such that it's probably not clear what I'm talking about anymore:

return foo && bar;

If foo is always true or always false, I would expect the line to show uncovered branches, but it doesn't (at least not in the code that I had modified earlier).

@Trott
Copy link
Member Author

Trott commented Feb 4, 2019

The checks however are not obsolete. Removing them would cause some input to throw a wrong error.

Ah, that's how you can do it! Thanks. We don't have any test cases for that sort of thing, but I'll remove that second commit and add a bunch to make sure we get every combination of values tested in those ternaries.

Test for deep strict equality when prototype and toStringTag
have been modified in surprising ways.
@Trott
Copy link
Member Author

Trott commented Feb 5, 2019

Modifications to lib removed. Test updated further. @BridgeAR PTAL, thanks!

@Trott Trott changed the title test,util: add coverage for boxed bigints comparisons; remove unnecessary code in internal util function test: add coverage for boxed bigints comparisons; remove unnecessary code in internal util function Feb 5, 2019
@Trott
Copy link
Member Author

Trott commented Feb 5, 2019

@Trott Trott changed the title test: add coverage for boxed bigints comparisons; remove unnecessary code in internal util function test: add coverage for boxed bigints comparisons and edge cases Feb 5, 2019
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 5, 2019
@bcoe
Copy link
Contributor

bcoe commented Feb 5, 2019

@Trott if you don't mind, perhaps make a tracking issue and assign to me? could be a problem with the remapping logic. Is it specifically when there's a foo && bar combined with a return? I believe we should treat as an example const banana = apple && strawberry as two distinct branches.

@Trott
Copy link
Member Author

Trott commented Feb 5, 2019

@Trott if you don't mind, perhaps make a tracking issue and assign to me?

An issue was opened by @BridgeAR at #25937. I've assigned it to you.

Is it specifically when there's a foo && bar combined with a return? I believe we should treat as an example const banana = apple && strawberry as two distinct branches.

I'll put a code example over in that issue along with c8 output that I believe to be incorrect. I'll also open a bug directly with c8. Hopefully reporting in two places like that is not too spammy of me.

@Trott
Copy link
Member Author

Trott commented Feb 6, 2019

Landed in f31794b...5d52c8e.

@Trott Trott closed this Feb 6, 2019
Trott added a commit to Trott/io.js that referenced this pull request Feb 6, 2019
Add some BigInt tests for test-util-isDeepStrictEqual to get 100%
coverage for lib/internal/util/comparisons.js.

PR-URL: nodejs#25932
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Feb 6, 2019
Test for deep strict equality when prototype and toStringTag
have been modified in surprising ways.

PR-URL: nodejs#25932
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 7, 2019
Add some BigInt tests for test-util-isDeepStrictEqual to get 100%
coverage for lib/internal/util/comparisons.js.

PR-URL: #25932
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 7, 2019
Test for deep strict equality when prototype and toStringTag
have been modified in surprising ways.

PR-URL: #25932
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Feb 14, 2019
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