test: add coverage for boxed bigints comparisons and edge cases

[Trott]

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

[nodejs-github-bot]

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

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/20571/

[BridgeAR]

The first commit seems fine.

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

consta=newNumber(5)constb=newString(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]

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

returnfoo&&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]

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.

[Trott]

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

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/20576/

[bcoe]

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

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

Landed in f31794b...5d52c8e.