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

[jest-validate] support recursive config check #6802

Merged
merged 10 commits into from
Aug 9, 2018

Conversation

thymikee
Copy link
Collaborator

@thymikee thymikee commented Aug 3, 2018

Summary

Adds recursive and recursiveBlacklist options to jest-validate.

  • recursive - checks config recursive (deep check). true by default.
  • recursiveBlacklist - a list of keyPaths to disable from recursive checks (helpful for moduleNameMapper in Jest, which accepts all strings as valid object keys). I always confuse blacklist with whitelist, so if you feel like this is rather a whitelist, let me know 😛
  • prints errors using JSON.stringify for nicer examples (couldn't use pretty-format as it adds constructors to printed strings)

TODOs:

  • update example config that we compare to inside Jest, because users will likely face some validation warnings with the current one.

screen shot 2018-08-03 at 18 07 29

Test plan

Added tests

@codecov-io
Copy link

codecov-io commented Aug 3, 2018

Codecov Report

Merging #6802 into master will increase coverage by 0.03%.
The diff coverage is 95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6802      +/-   ##
==========================================
+ Coverage   63.55%   63.59%   +0.03%     
==========================================
  Files         235      235              
  Lines        9030     9042      +12     
  Branches        3        4       +1     
==========================================
+ Hits         5739     5750      +11     
- Misses       3290     3291       +1     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-config/src/valid_config.js 100% <ø> (ø) ⬆️
packages/jest-validate/src/warnings.js 54.54% <ø> (ø) ⬆️
packages/jest-validate/src/default_config.js 100% <ø> (ø) ⬆️
packages/jest-validate/src/example_config.js 33.33% <ø> (-26.67%) ⬇️
packages/jest-validate/src/validate.js 97.05% <100%> (-2.95%) ⬇️
packages/jest-validate/src/utils.js 85.71% <100%> (+1.09%) ⬆️
packages/jest-validate/src/errors.js 54.54% <100%> (ø) ⬆️
packages/jest-config/src/normalize.js 91.33% <80%> (ø) ⬆️
... and 4 more

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 0591f22...d160590. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Aug 4, 2018

I'm not sure if we need the recursive option and not just make it the default. If it throws for people's config that's because it silently didn't work before, isn't it? Explicitly saying "this is wrong" seems like a bug fix, IMO

@@ -14,6 +14,7 @@ type Title = {|
|};

export type ValidationOptions = {
blacklist?: Array<string>,
Copy link
Member

Choose a reason for hiding this comment

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

recursiveBlacklist? Or something like that, I think it should be tied to what the blacklist is for, so I don't have to look up the docs

@thymikee
Copy link
Collaborator Author

thymikee commented Aug 4, 2018

I was thinking of modules like Prettier (cc @azz), or maybe some other ones that would break after this change.

@thymikee
Copy link
Collaborator Author

thymikee commented Aug 5, 2018

Updated.
Just to be clear, the thing with recursive being turned on by default imposes providing perfectly valid config with all possible properties (not simplified, e.g. by passing empty object instead of object with all props) by the lib authors that use jest-validate. We can treat it as a bugfix too, I'm not opposed.
cc @cpojer

@thymikee
Copy link
Collaborator Author

thymikee commented Aug 6, 2018

@endiliey the Netlify preview is failing again. Can you have a look?

@endiliey
Copy link
Contributor

endiliey commented Aug 6, 2018

@thymikee done. Retrying the deploy & clearing netlify build cache solved it.

@@ -87,7 +93,7 @@ export default ({
skipNodeResolution: false,
snapshotSerializers: ['my-serializer-module'],
testEnvironment: 'jest-environment-jsdom',
testEnvironmentOptions: {},
testEnvironmentOptions: {userAgent: 'Agent/007'},
Copy link
Contributor

Choose a reason for hiding this comment

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

License to kill? 🕵🏼‍♂️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🔫

@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 12, 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.

6 participants