Add fuzzing based tests in Jest

[dubzzz]

Summary

Following discussions with @jeysal and @SimenB in the PR #7938, I open a PR to add property based tests into the CI of Jest.

In a nutshell what it property based testing:

Property based can be classified as being some kind of fuzzing technic. The need for such technic comes from the fact that it is difficult and barely impossible to cover all the edge cases a code can have with classical unit-tests (so called example based tests).

Property based framework relies on assessing whether a property stays true for any valid inputs. It can be summarized by the following mathematical statement:

for all (x, y, ...)
such as precondition(x, y, ...) holds
property(x, y, ...) is true

In case of failure, it is able to reduce the randomly generated x, y to smaller entries to be more human readable.

@SimenB I will try to work on the RFC next week

[SimenB]

To my eyes this looks great. Thank you so much for putting it together!

Could you show an example (screenshot) of how a failure looks?

[dubzzz]

Here is an example of an error output (non verbose mode):

[jeysal]

This is great, thanks @dubzzz! I found some errors in the comments. Please go over them to make sure they accurately describe what the test cases do so people unfamiliar with property-based testing do not get confused 😄

[pedrottimark]

Thank you for bringing explicit property-based assertions to improve the quality of matchers.

/cc @grosto your thoughts are welcome in this discussion

I will let Simen and Tim brainstorm options (like seed and search depth) for CI versus local run.

To learn fast-check package, I will follow your example for another matcher.

• Suggestion: despite current pattern in matchers.test.js file, let’s simplify matcher names to describe('toEqual', …) because I found recently regexp characters especially () are trouble for --testNamePattern option (but see next question)
• Question: what do y’all think are strength and weakness of splitting tests from huge file to matchers-toEqual.test.ts and so on?
• Question: Can we organize existing example-based tests as specifics of property-based tests?
• First impression about Jest integration: is fc.assert what needs to be integrated? in a way that tester can still explicitly import/require whichever version offast-check package they want?

@dubzzz Your thoughts also welcome about tests for diff-sequences package in Jest repo.

P.S. The effort to find broken edge cases in expect package has code name “expectorant” ;)

[jeysal]

• Question: Can we organize existing-based tests as specific examples of property-based tests?

This sounds really interesting, @dubzzz seen something like that been done before?

[pedrottimark]

Forgot to say that fast-check will also be local research tool to confirm understanding about differences between assertion libraries when we fix subtle problems (for example, sets and maps).

[dubzzz]

@pedrottimark, @jeysal here are some of my answers:

Suggestion: despite current pattern in matchers.test.js file, let’s simplify matcher names to describe('toEqual', …) because I found recently regexp characters especially () are trouble for --testNamePattern option (but see next question)

✅ I am updating the review accordingly. Indeed it seems easier to write a regex for toEqual than for .toEqual() (escaping needed for this one).

Question: what do y’all think are strength and weakness of splitting tests from huge file to matchers-toEqual.test.ts and so on?

✅ IMO it would be a great idea. Today the file gathering tests for matchers is quite huge. Splitting it into smaller entities would make it more readable and easier to find - just need to look for a file whose name is toEqual for instance.

Question: Can we organize existing example-based tests as specifics of property-based tests?

fast-check makes it possible for users to specify special values they always want to test against a given property. If you add custom examples, the first random values would be replaced by the custom values you wanted to assess.

It's important to see that sometimes, the property would not be as accurate as a real unit test. For instance, in the test Given a, b random values, expect(a).toEqual(b) should be equivalent to expect(b).toEqual(a) does not check if the expect if successful or failing. Ii only checks for symmetry. In a way properties should be seen as a complement to classical unit-tests.

Documentation of the feature: https://github.com/dubzzz/fast-check/blob/master/documentation/Tips.md#add-custom-examples-next-to-generated-ones.

See the initial feature request: dubzzz/fast-check#141.

✅ I added examples for the issues I recently opened in Jest.

First impression about Jest integration: is fc.assert what needs to be integrated? in a way that tester can still explicitly import/require whichever version offast-check package they want?

In my wrapper for ava [here], fast-check is a peer dependency of the package so that users can choose which version of fast-check they want to use. I don't known exactly which would be the best thing for an integration with Jest ;)

Choice A: peer dependency

Users would need to explicitely install fast-check to have it available.

Choice B: dependency on 1.x.x

Users would directly benefit from fast-check. I think they might still be able to bump to the latest version just by referencing it as a dev dependency of their own project - has to be confirmed.

Your thoughts also welcome about tests for diff-sequences package in Jest repo.

I will have a look to this one to see how I would have tested it with property based.

[SimenB]

Side note (better discussed in the RFC), but if we include fast-check in Jest, it will not be as a peer dep - we don't want people to have to install extra packages to use built-in features. The prettier requirement for inline snapshots is unfortunate and not something I think we should repeat.

I think they might still be able to bump to the latest version just by referencing it as a dev dependency of their own project - has to be confirmed.

Yeah, that should be fine. We'll have a semver range, so unless you release a new major it'll be within reach

[dubzzz]

@pedrottimark If I understand well what is the purpose of diff-sequences, it is pretty similar to: find the longest common substring except we are dealing with arrays of objects instead of strings.

I think we can adapt tests I did on commonLongestSubstr to diff-sequences:

• symmetry: diff(a, b) has the same length as diff(b, a) -- there is not a real symmetry but lengths are equal
• sub-part of the input: diff(a, b) is a subset of a and a subset of b
• other cases described in the review below

Full code: dubzzz/javascript-algorithms@67102e4

[dubzzz]

@SimenB I will try to open the RFC later this week to discuss those points ;)
Or maybe now

[dubzzz]

@pedrottimark @SimenB Let's discuss direct integration within Jest here: #8035

[dubzzz]

For the moment I will remove the property called should distinguish distinct values.

1. The precondition relying on a tweaked version of JSON.stringify is too fragile. For instance it would consider that {a:0,b:1} and {b:1,a:0} are distinct and move to the expect(a).not.toStrictEqual(b). -- was not detected so far because the probability of such entry is barely null given the scope of valid inputs
2. Generated objects are far from each others, I need to find a better solution for generating objects closer to each others.

Meanwhile those properties are still a good starting point :)

[jeysal]

This mostly LGTM in its current form and I'm excited to see what other bugs we may find with further property tests for other matchers.
Just one request: Given that we apparently agree something like matchers.test.ts shouldn't exist but rather be split into tests for individual matchers, I think we should set a good example here and not make the "everything into one file" mistake for property-based tests.
@dubzzz Could you split the test (essentially converting the current describe blocks into individual files)?

[dubzzz]

@jeysal I just updated the review based on your last comment. I divided the spec into four distinct specs. Common settings have been moved into a file shared accross those files called sharedSettings.ts.

[dubzzz]

Some checks failed, but I don't if they are related to the review. Can you help me?

#!/bin/bash -eo pipefail
yarn test-ci

yarn run v1.13.0
$ yarn jest-coverage -i --config jest.config.ci.js && yarn test-leak && node scripts/mapCoverage.js && codecov
$ yarn jest --coverage -i --config jest.config.ci.js
$ node ./packages/jest-cli/bin/jest.js --coverage -i --config jest.config.ci.js
........(node:335) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 exit listeners added. Use emitter.setMaxListeners() to increase limit
......................................................................................................................................................................................................................................
  ● Test suite failed to run

    Your test suite must contain at least one test.

      at packages/jest-core/build/TestScheduler.js:221:24

..............................................................................

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Exited with code 1

See https://circleci.com/gh/facebook/jest/56322?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

[jeysal]

I divided the spec into four distinct specs.

Great, thanks! The set issue is not close to being fixed, so I guess we can merge this now and enable the Set tests / add further tests later.

[dubzzz]

@thymikee @pedrottimark Do you approve the review on your side too?

[pedrottimark]

@dubzzz I apologize for delay to finish the review. I will look with fresh eyes tomorrow.

[thymikee]

👍

[dubzzz]

Just rebased all the commits due to a conflict in package.json. Test should be green again following this rebase

[pedrottimark]

Thank you for building a beautiful foundation! Now we know who will test the testers :)

We can clean up existing tests while moving them into new files as examples, when possible.

[SimenB]

Thank you so much @dubzzz!

[SimenB]

Hmm, I merged this thinking the CI failure was transient. But it failed twice on master with the same error as well.

@dubzzz mind checking out why the node process aborts using circus? To run with circus locally, do JEST_CIRCUS=1 yarn jest. Might have to revert tomorrow (we plan on pushing quite a lot this coming week, and need green CI before that)

[dubzzz]

@SimenB Please revert for the moment :'(, if it breaks your CI. I will spend some time to investigate the issue next week (I'll not be able to make it today)

[dubzzz]

What's strange is that it passed before my rebase. Like you I thought it was just transient :/

Last week, I started to investigate whether it can be related to the size of the instances generated by fc.anything(). But instances were not so huge :/

I'll re-run the circus test suite locally to have a better understanding of what is going wrong. Sorry for the inconvenience :(

[SimenB]

Sorry for the inconvenience :(

No worries! If it hadn't been for the fact we're gonna be working on this quite a lot the coming week there'd have been no rush. So just fix it whenever you find the time!

[dubzzz]

@SimenB I do not reproduce the crash on my machine. Did you reproduce it locally?
Did you encounter the failures on other test suites - not ci/circleci: test-jest-circus?

Investigation

On my side I started to investigate what could have led fast-check to consume so many ressources.

The issue seems to come from the fact, that fast-check will generate large objects when asked to build fc.array(fc.anything()). Basically you can imagine that:

• fc.array generates arrays having between 0 to 10 items
• fc.anything generates an object of maximal depth 2 having at most 10 keys (depth 0 is equivalent to a primitive)

In addition to the generated object, fast-check provides a shrinker. The shrinker will be used internally in case of failure to reduce an object to a smaller one (easier to debug). Unfortunately it takes lots of memory for large fc.anything() and might explain the out-of-memory in some extreme cases where multiple references are kept alive in memory.

In order to confirm my say and reduce the memory footprint of fc.array(fc.anything()), I wrote a code snippet to measure its memory consumption. I found a way to divide the memory consumption by a factor of 2 to 3 by reducing the number of keys of the objects generated by fc.anything().

Next

Maybe the best choice to check if my coming soon fix works properly will be to open a new PR to try it against the CI. We can confirm its stability by relaunching the CI multiple times on the next PR if needed.

Still confirming the explanation above on my side

[SimenB]

I was unable to reproduce this locally as well...

Opening up a PR seems reasonable to me!

[dubzzz]

@SimenB Let's retry with PR #8164

Before merging it, we should definitely confirm we will not encounter the out-of-memory issue with it.

The fix: I basically decreased the size of the generated instances (and also the memory consumed to store the shrinker). Instances go from 10 (this PR) to 5 (the new PR) keys per object with a maximal depth of 2 (already the case before). When I tested it manually, it reduced by a factor of 2-3 the size required to store value+shrinker in memory.

[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.