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

dubzzz · 2019-02-19T22:47:03Z

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 · 2019-02-19T22:47:15Z

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 · 2019-02-19T22:50:06Z

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

jeysal

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

jeysal · 2019-02-20T09:49:33Z

CHANGELOG.md

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


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

jeysal · 2019-02-20T09:54:10Z

packages/expect/src/utils.js

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


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?

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

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?

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

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 · 2019-02-20T13:08:56Z

Codecov Report

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

@@           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 · 2019-02-20T14:07:37Z

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

dubzzz · 2019-02-20T14:23:11Z

No problem I will rebase asap

dubzzz · 2019-02-20T16:57:58Z

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

SimenB · 2019-02-20T17:03:40Z

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

dubzzz · 2019-02-20T17:09:10Z

@SimenB should be rebased now

pedrottimark

Thank you very much!

dubzzz · 2019-02-20T23:53:01Z

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?

jeysal · 2019-02-21T23:49:33Z

@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 · 2019-02-21T23:54:58Z

@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 · 2019-02-22T00:14:44Z

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 · 2019-02-24T20:59:39Z

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 · 2019-02-24T22:28:03Z

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 · 2019-02-24T22:46:08Z

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 · 2019-02-25T11:11:43Z

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 · 2019-02-28T15:38:20Z

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 · 2019-02-28T17:22:25Z

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! ❤️

github-actions · 2021-05-12T00:06:43Z

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.

facebook-github-bot added the cla signed label Feb 19, 2019

dubzzz force-pushed the fix-7937 branch from 4afb8dd to 367d65a Compare February 19, 2019 22:56

jeysal reviewed Feb 20, 2019

View reviewed changes

SimenB requested a review from pedrottimark February 20, 2019 14:06

SimenB approved these changes Feb 20, 2019

View reviewed changes

SimenB requested a review from thymikee February 20, 2019 14:08

dubzzz added 3 commits February 20, 2019 17:56

Suggestion of fix for jestjs#7937

d9dfdb3

Update CHANGELOG

976ad4d

Update CHANGELOG.md following review

d2f5228

Add an additonal test for deeper keys

04ebef5

dubzzz force-pushed the fix-7937 branch from 8274a76 to 04ebef5 Compare February 20, 2019 17:07

jeysal approved these changes Feb 20, 2019

View reviewed changes

pedrottimark approved these changes Feb 20, 2019

View reviewed changes

thymikee approved these changes Feb 20, 2019

View reviewed changes

thymikee merged commit 44bfa6e into jestjs:master Feb 20, 2019

This was referenced Feb 21, 2019

toStrictEqual fails to distinguish 0 from 5e-324 #7941

Closed

Fix asymmetric equal for Number #7948

Merged

dubzzz deleted the fix-7937 branch February 21, 2019 20:22

dubzzz mentioned this pull request Feb 24, 2019

toEqual not symmetric for Set #7975

Closed

This was referenced Mar 1, 2019

Add fuzzing based tests in Jest #8012

Merged

Add property based testing based on fast-check in Jest #8035

Closed

dubzzz mentioned this pull request Mar 19, 2019

Add fuzzing based tests in Jest (retry PR8012) #8164

Merged

snyk-bot mentioned this pull request Dec 15, 2019

[Snyk] Upgrade babel-jest from 24.0.0 to 24.9.0 jakeherp/burger-builder#14

Open

This was referenced Mar 21, 2020

[Snyk] Upgrade: jest, jest-cli lwojcik/sc2pte-panel-frontend#88

Closed

[Snyk] Upgrade jest from 24.0.0 to 24.9.0 DavidKindler/burger-builder#47

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dubzzz commented Feb 19, 2019

facebook-github-bot commented Feb 19, 2019

facebook-github-bot commented Feb 19, 2019

jeysal left a comment

jeysal Feb 20, 2019

dubzzz Feb 20, 2019

jeysal Feb 20, 2019

dubzzz Feb 20, 2019

jeysal Feb 20, 2019

dubzzz Feb 20, 2019

pedrottimark Feb 20, 2019

codecov-io commented Feb 20, 2019

SimenB commented Feb 20, 2019

dubzzz commented Feb 20, 2019

dubzzz commented Feb 20, 2019

SimenB commented Feb 20, 2019

dubzzz commented Feb 20, 2019

pedrottimark left a comment

dubzzz commented Feb 20, 2019

jeysal commented Feb 21, 2019

dubzzz commented Feb 21, 2019

jeysal commented Feb 22, 2019

dubzzz commented Feb 24, 2019

dubzzz commented Feb 24, 2019 •

edited

jeysal commented Feb 24, 2019

SimenB commented Feb 25, 2019

dubzzz commented Feb 28, 2019

SimenB commented Feb 28, 2019 •

edited

github-actions bot commented May 12, 2021

	- `[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))

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

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

Conversation

dubzzz commented Feb 19, 2019

Summary

Test plan

facebook-github-bot commented Feb 19, 2019

facebook-github-bot commented Feb 19, 2019

jeysal left a comment

Choose a reason for hiding this comment

jeysal Feb 20, 2019

Choose a reason for hiding this comment

dubzzz Feb 20, 2019

Choose a reason for hiding this comment

jeysal Feb 20, 2019

Choose a reason for hiding this comment

dubzzz Feb 20, 2019

Choose a reason for hiding this comment

jeysal Feb 20, 2019

Choose a reason for hiding this comment

dubzzz Feb 20, 2019

Choose a reason for hiding this comment

pedrottimark Feb 20, 2019

Choose a reason for hiding this comment

codecov-io commented Feb 20, 2019

Codecov Report

SimenB commented Feb 20, 2019

dubzzz commented Feb 20, 2019

dubzzz commented Feb 20, 2019

SimenB commented Feb 20, 2019

dubzzz commented Feb 20, 2019

pedrottimark left a comment

Choose a reason for hiding this comment

dubzzz commented Feb 20, 2019

jeysal commented Feb 21, 2019

dubzzz commented Feb 21, 2019

jeysal commented Feb 22, 2019

dubzzz commented Feb 24, 2019

dubzzz commented Feb 24, 2019 • edited

jeysal commented Feb 24, 2019

SimenB commented Feb 25, 2019

dubzzz commented Feb 28, 2019

SimenB commented Feb 28, 2019 • edited

github-actions bot commented May 12, 2021

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

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

dubzzz commented Feb 24, 2019 •

edited

SimenB commented Feb 28, 2019 •

edited