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

`toStrictEqual` does not consider arrays with objects having undefined values correctly #7938

Merged
merged 4 commits into from Feb 20, 2019

Conversation

@dubzzz
Copy link
Contributor

dubzzz commented Feb 19, 2019

Summary

Fixes #7937: toStrictEqual does not consider arrays with objects having undefined values correctly

Test plan

Before that change, the following assertion was failing:

expect([{a: undefined}]).not.toStrictEqual([{}]);

Now it passes as expected.

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Feb 19, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Feb 19, 2019

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@dubzzz dubzzz force-pushed the dubzzz:fix-7937 branch from 4afb8dd to 367d65a Feb 19, 2019
Copy link
Collaborator

jeysal left a comment

Thanks for reporting this issue and submitting the fix! Left a few comments with things that came to my mind

@@ -12,6 +12,7 @@
- `[jest-cli]` Fix prototype pollution vulnerability in dependency ([#7904](https://github.com/facebook/jest/pull/7904))
- `[jest-cli]` Refactor `-o` and `--coverage` combined ([#7611](https://github.com/facebook/jest/pull/7611))
- `[expect]` Fix custom async matcher stack trace ([#7652](https://github.com/facebook/jest/pull/7652))
- `[expect]` `toStrictEqual` does not consider arrays with objects having undefined values correctly ([#7938](https://github.com/facebook/jest/pull/7938))

This comment has been minimized.

Copy link
@jeysal

jeysal Feb 20, 2019

Collaborator
Suggested change
- `[expect]` `toStrictEqual` does not consider arrays with objects having undefined values correctly ([#7938](https://github.com/facebook/jest/pull/7938))
- `[expect]` Fix `toStrictEqual` not considering arrays with objects having undefined values correctly ([#7938](https://github.com/facebook/jest/pull/7938))

This comment has been minimized.

Copy link
@dubzzz

dubzzz Feb 20, 2019

Author Contributor

Will do

@@ -251,7 +251,9 @@ export const sparseArrayEquality = (a: any, b: any) => {
// A sparse array [, , 1] will have keys ["2"] whereas [undefined, undefined, 1] will have keys ["0", "1", "2"]
const aKeys = Object.keys(a);
const bKeys = Object.keys(b);
return equals(a, b) && equals(aKeys, bKeys);
return (
equals(a, b, [iterableEquality, typeEquality], true) && equals(aKeys, bKeys)

This comment has been minimized.

Copy link
@jeysal

jeysal Feb 20, 2019

Collaborator

Should this have sparseArrayEquality too? There could be nested arrays for which we need to do that check. Or does that already work this way?

This comment has been minimized.

Copy link
@dubzzz

dubzzz Feb 20, 2019

Author Contributor

It would create an infinite loop:
toStrictEqual(a, b) -> equals(a, b, [iterableEquality, typeEquality, sparseArrayEquality], true) -> sparseArrayEquality(a, b) -> equals(a, b, [iterableEquality, typeEquality, sparseArrayEquality], true) -> ...

😁 I did try it at the beginning

This comment has been minimized.

Copy link
@jeysal

jeysal Feb 20, 2019

Collaborator

I've just checked and it also works e.g. for

expect([[{a: undefined}]]).not.toStrictEqual([[]]);
expect([{a: [{a: undefined}]}]).not.toStrictEqual([{a: []}]);

I just wasn't quite sure how exactly the recursion works in the beginning.
Maybe add those test cases as well for peace of mind?

This comment has been minimized.

Copy link
@dubzzz

dubzzz Feb 20, 2019

Author Contributor

I just added an extra test to check that strictEqual will work fine with deeper objects

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Feb 20, 2019

Collaborator

The regression and the question about correctness of its correction is evidence that as we continue to fix edge cases in expect package, we also need to think how to reduce the complexity of coupling between custom testers.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 20, 2019

Codecov Report

Merging #7938 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7938   +/-   ##
=======================================
  Coverage   65.33%   65.33%           
=======================================
  Files         255      255           
  Lines        9941     9941           
  Branches     1041     1041           
=======================================
  Hits         6495     6495           
  Misses       3195     3195           
  Partials      251      251
Impacted Files Coverage Δ
packages/expect/src/utils.js 98.37% <100%> (ø) ⬆️

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 b8a9a71...8274a76. Read the comment docs.

@SimenB SimenB requested a review from pedrottimark Feb 20, 2019
@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Feb 20, 2019

Thanks for the PR! We just merged a migration of expect to TypeScript, sorry about the conflict.

@SimenB
SimenB approved these changes Feb 20, 2019
@SimenB SimenB requested a review from thymikee Feb 20, 2019
@dubzzz

This comment has been minimized.

Copy link
Contributor Author

dubzzz commented Feb 20, 2019

No problem I will rebase asap

@dubzzz

This comment has been minimized.

Copy link
Contributor Author

dubzzz commented Feb 20, 2019

@SimenB I rebased locally with no issue, is the migration to Typescript of èxpect on master?

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Feb 20, 2019

It's on master, yes. You can see GitHub reporting a merge conflict

@dubzzz dubzzz force-pushed the dubzzz:fix-7937 branch from 8274a76 to 04ebef5 Feb 20, 2019
@dubzzz

This comment has been minimized.

Copy link
Contributor Author

dubzzz commented Feb 20, 2019

@SimenB should be rebased now

@jeysal
jeysal approved these changes Feb 20, 2019
Copy link
Collaborator

pedrottimark left a comment

Thank you very much!

@thymikee thymikee merged commit 44bfa6e into facebook:master Feb 20, 2019
12 checks passed
12 checks passed
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
facebook.jest #20190220.6 succeeded
Details
@dubzzz

This comment has been minimized.

Copy link
Contributor Author

dubzzz commented Feb 20, 2019

Thanks a lot for your quick reviews 👍

Following this bug, I wanted to come with some kind of automatic way to detect such regression or bug in the future.

For the moment I commited a draft proposal for such test on my fork: dubzzz@114022f

The test basically checks the following:

Given a and b values such as a different from b
We expect expect(a).not.toStrictEqual(b) to succeed

It caught another bug in toStrictEqual: expect(0).not.toStrictEqual(5e-324) fails

@SimenB, @thymikee, @pedrottimark, @jeysal:
Any opinion concerning this kind of test?
Do you think it can be useful to have this kind of tests into Jest CI?

@dubzzz dubzzz deleted the dubzzz:fix-7937 branch Feb 21, 2019
@jeysal

This comment has been minimized.

Copy link
Collaborator

jeysal commented Feb 21, 2019

@dubzzz I've heard of fast-check (and jsverify) before and I like the idea, could imagine adding them in a separate test file

@dubzzz

This comment has been minimized.

Copy link
Contributor Author

dubzzz commented Feb 21, 2019

@jeysal Thanks a lot for your input, I will write a new spec with only properties for matchers. I let you know when I have something ready so that we can iterate on it ;)

@jeysal

This comment has been minimized.

Copy link
Collaborator

jeysal commented Feb 22, 2019

I'd be particularly interested to learn how to achieve determinism with this, but let's see when you have prepared a draft and discuss such topics there :)

@dubzzz

This comment has been minimized.

Copy link
Contributor Author

dubzzz commented Feb 24, 2019

I clearly get your concerns of potential flakiness added by such test suites.

There are multiple things to keep in mind with property based testing:

  • runs can be seeded - when seeded no flakyness possible - but I would rather recommend to have a new seed for each run
  • in case of failure, the seed causing the failure is visible in the logs so that you can reproduce the issue and open a ticket to track it
  • in case of failure, the final case is the minimal failing case, that is to say: not an ugly random large entry
  • in case of failure, it means there is a real bug in the code (or in the test if the test is wrong)
  • the default number of runs is 100 (barely all frameworks do this)

I would say that bug detection is pretty good and stable - I mean it finds bugs consistently.

Justification: integration test and examples of fast-check - executed for all CI runs on node 6, 8 and 10 - check that fast-check properly detects bugs in buggy code. And they are stable. Example: detect a bug occuring only when an object contains a sub-object.

But indeed sometimes bugs are not easy to detect and might be detected a bit later. In a way it is still useful as it means the bug is now known.

@dubzzz

This comment has been minimized.

Copy link
Contributor Author

dubzzz commented Feb 24, 2019

I think it detected another bug:

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

expect(s1).not.toEqual(s2); // success

expect(s2).not.toEqual(s1); // failure
//    expect(received).not.toEqual(expected)
//    Expected: Set {false, true}
//    Received: Set {{}, {}}

An issue with symmetry of toEqual.

@jeysal

This comment has been minimized.

Copy link
Collaborator

jeysal commented Feb 24, 2019

Interesting, thanks for elaborating! As mentioned, I'm in favor of integrating such tests in places where it makes sense, and expect definitely seems like such a case 👍

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Feb 25, 2019

I'm very much in favor of adding fuzz testing to expect.

This got me thinking, should we ship fuzz testing as part of jest itself? Might make it a bit more mainstream. Should be enough to just wrap jsverify, fast-check or whatever

@dubzzz

This comment has been minimized.

Copy link
Contributor Author

dubzzz commented Feb 28, 2019

As you both agree to add property based testing or fuzzing into the tests of Jest, I am going to open a PR for that purpose.

@SimenB I definitely welcome the idea to wrap property based testing tools directly within Jest. I think it is a great idea: growing the popularity of such approach among JavaScript world would be very nice.

I am obviously a little biased concerning the choice of the framework as I wrote fast-check. In a nutshell when I first wanted to do property based in my company I opted for jsverify but got stuck on some of its limitations (due to initial design choices). So I started to explore how it worked and somehow started fast-check.

Why fast-check?
Maintained, used by awslabs/aws-cdk for instance, multiple unique features: replay, verbose, commands for UI tests, pre-conditions...
More on the read me.

Concerning integration in Jest, I did some wrapper of fast-check for ava. Might give you some ideas: https://github.com/dubzzz/ava-fast-check/

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Feb 28, 2019

As you both agree to add property based testing or fuzzing into the tests of Jest, I am going to open a PR for that purpose.

Awesome!

@SimenB I definitely welcome the idea to wrap property based testing tools directly within Jest. I think it is a great idea: growing the popularity of such approach among JavaScript world would be very nice.

Would you mind opening an RFC for it? It doesn't have to be very detailed, the paragraphs you have above are great.
The only thing I'd like to see in addition is some API proposals of how this would look like if it were built into Jest. Would this live on describe, it and test (like test.each), on expect (like expect.extend, expect.hasAssertions and resolves/rejects) or on jest (like jest.retryTimes and jest.runAllTimers). Immediately, I think this makes sense on expect, like expect.fuzz.blablabla, but I might be wrong 🙂
Another consideration is how would you pass the seed? Would we just allow you to pass in inline, or should it be as configuration? If its configuration, it would be trivial to implement a watch plugin which allowed you to set the seed, run tests (ad fix them), then ditch the seed without having to edit the test itself. That might very well be overkill though! I mainly write it down to see if it triggers any ideas for you.

I'd like to see a separate RFC so it's more visible to other people, instead of being discussed in a merged PR.

Thanks! ❤️

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.