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

property-based test for Node deepStrictEqual equivalence #9167

Merged
merged 3 commits into from Nov 13, 2019

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Nov 12, 2019

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.

Copy link
Contributor

@dubzzz dubzzz left a comment

Choose a reason for hiding this comment

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

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
});

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

jeysal commented Nov 13, 2019

 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
Copy link
Contributor Author

jeysal commented Nov 13, 2019

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
Copy link
Contributor Author

jeysal commented Nov 13, 2019

200k runs on Node 10 passed

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants