property-based test for Node deepStrictEqual equivalence

[jeysal]

Summary

Builds on the awesome work of @dubzzz by adding a test that our expect.toStrictEqual is equivalent to Node's assert.deepStrictEqual. Even though I disagree with the behavior of deepStrictEqual in some situations, I think these are not too important and aligning with other equality implementations deepStrictEqual is more important. @SimenB @pedrottimark do you agree?

Running this a couple of times uncovered one mismatch:
Node (this btw means fast-deep-equal afaik) considers a primitive different from a new Primitive() (while it is of course equal to a Primitive() without new). This makes sense to me (they do not even have the same typeof!), and fwiw I tried concordance (=> AVA) and it agrees with Node.
I fixed this, which is breaking, but since using primitive constructors like new Number() etc. is a bad pattern anyway (without new is recommended) I do not think this will affect many users.

Also sneaked in a fix ensuring that all RegExp flags are considered for equality to avoid false negatives.

Side note:
I am not very happy with our equality implementation when comparing it to fast-deep-equal or concordance in terms of structure / maintainability. It needs a rewrite (possibly in line with refactors for #6184), or replacement with an external lib.

Test plan

Added property-based test as described.
Changed existing property-based tests to import expect so that they do not require rebuilds to be up to date.
Added specific test cases for the changed equality behavior.

[dubzzz]

I just had a quick look into your new property based test. It looks great 👍

In addition I believe that this behaviour change might partially fix an issue I opened in the past #7975. I haven't tried with your new implementation but as the aim is to fully mimic Node assert for toStrictEqual, it should fix the problem for toStrictEqual (not toEqual).

Indeed, today with node 12 and Jest 24.9.0, I get the following behaviour:

const s1 = new Set([false, true]);
const s2 = new Set([new Boolean(true), new Boolean(true)]);

test("plop 1", () => {
  expect(s1).not.toStrictEqual(s2); // expect passes
  expect(s2).not.toStrictEqual(s1); // expect fails
  assert.notDeepStrictEqual(s1, s2); // expect passes
  assert.notDeepStrictEqual(s2, s1); // expect passes
});

[SimenB]

Makes sense to me to match node's behavior here, yeah 👍. I'm also down with offloading our equality function to other implementations if that makes sense and still passes all our tests

[jeysal]

 Property failed after 18862 tests                                                                                                                                                           
    { seed: -621258559, path: "18861", endOnFailure: true }                                                                                                                                     
    Counterexample: [new Number(-0),new Number(0)]                                                                                                                                              
    Shrunk 0 time(s)                                                                                                                                                                            
    Got error: Error: expect(received).toBe(expected) // Object.is equality                                                                                                                     
                                                                                                                                                                                                
    Expected: true                                                                                                                                                                              
    Received: false                                                                                                                                                                             
                                                                                                                                                                                                
    Stack trace: Error: expect(received).toBe(expected) // Object.is equality                                                                                                                   
                                                                                                                                                                                                
    Expected: true                                                                                                                                                                              
    Received: false

[jeysal]

Ahhh Node 8 <3 didn't even realize the VPS I ran this on still had a nvm default of 8 ^^ restricted test to >=9 (fixed in Node 9.0.0)
I'll do another 200k runs on Node 10 (who knows if there's other stuff they fixed since then that might fail Node 10 CI if we merge this restricted to >= 9)

[jeysal]

200k runs on Node 10 passed

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.