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

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

Merged
merged 2 commits into from
Apr 2, 2019

Conversation

dubzzz
Copy link
Contributor

@dubzzz dubzzz commented Mar 19, 2019

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
Copy link
Member

SimenB commented Mar 19, 2019

We've actually had a failure on ci with the same error after reverting. So I think we're probably pretty close to some limit, and this just pushed us over. Not sure how to best debug this

@dubzzz
Copy link
Contributor Author

dubzzz commented Mar 19, 2019

In a way, it's pretty reassuring for this PR, but indeed the memory issue in CI is a pretty blocking issue :/

Did you try to play with --max_old_space_size flag in node?

@SimenB
Copy link
Member

SimenB commented Mar 19, 2019

No, haven't done anything with it

@dubzzz
Copy link
Contributor Author

dubzzz commented Mar 19, 2019

According to nodejs/node#7937 (comment) which references https://github.com/nodejs/node/blob/ec02b811a8a5c999bab4de312be2d732b7d9d50b/deps/v8/src/heap/heap.cc#L82, the size for a 64bits system would be 1400 MB = 700 * ((64/8) / 4) MB

@dubzzz
Copy link
Contributor Author

dubzzz commented Mar 19, 2019

It happened again :'(

@dubzzz
Copy link
Contributor Author

dubzzz commented Mar 19, 2019

@SimenB Can't we tweak the number of workers to use in the case of test-jest-circus?

It seems to have fixed a similar issue #5239

@SimenB
Copy link
Member

SimenB commented Mar 19, 2019

It shouldn't need any different config than the other runs with jasmine. But feel free to tweak the -w flag

It should provide a better memory footprint.

Previously the default value of maxKeys was 10 (not accessible by the end-user in previous version).
The value has been reduced to 5 as it should already be enough to detect most of problems related to objects.
@codecov-io
Copy link

Codecov Report

Merging #8164 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #8164   +/-   ##
======================================
  Coverage    62.3%   62.3%           
======================================
  Files         265     265           
  Lines       10473   10473           
  Branches     2542    2542           
======================================
  Hits         6525    6525           
  Misses       3366    3366           
  Partials      582     582

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 7e4e170...b63f1c8. Read the comment docs.

Copy link
Contributor

@scotthovestadt scotthovestadt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

LGTM, @SimenB are there still concerns about memory usage making CI flaky?

@SimenB
Copy link
Member

SimenB commented Apr 2, 2019

This has probably been alleviated by @scotthovestadt's work.

Great stuff, thanks again @dubzzz! (I'll respond to your RFC when I'm able to get my head above water, I feel bad for letting it linger 😞)

@SimenB SimenB merged commit 9c5c449 into jestjs:master Apr 2, 2019
@dubzzz dubzzz deleted the property-based-2 branch October 14, 2019 20:28
@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

6 participants