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

fix(jest-config): parse testEnvironmentOptions if received from CLI #11902

Merged
merged 10 commits into from
Sep 28, 2021
Merged

fix(jest-config): parse testEnvironmentOptions if received from CLI #11902

merged 10 commits into from
Sep 28, 2021

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Sep 28, 2021

Resolves #11900

Summary

See #11900 (comment).

Test plan

I have added a unit test.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Wait, this seems like a bug - it should be parsed (the example in #11900 isn't JSON (it's JS literal) so that would fail, but that's separate).

https://github.com/facebook/jest/blob/8024306c365cdf8b17b2256c73f1f4c9f23f8f77/packages/jest-config/src/Defaults.ts#L67 is an object (and we use it as such internally: https://github.com/facebook/jest/blob/8024306c365cdf8b17b2256c73f1f4c9f23f8f77/packages/jest-environment-jsdom/src/index.ts#L37


I do think an improved description saying JSON string is a good idea, but it should be parsed into an object before being passed to custom envs

@mrazauskas
Copy link
Contributor Author

Ah.. That’s right.

I can fix this and also will add it to CLI docs for completeness.

@SimenB
Copy link
Member

SimenB commented Sep 28, 2021

Awesome, thank you @mrazauskas!

@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2021

Codecov Report

Merging #11902 (05f4239) into main (8024306) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11902      +/-   ##
==========================================
- Coverage   68.73%   68.72%   -0.01%     
==========================================
  Files         322      322              
  Lines       16582    16583       +1     
  Branches     4784     4785       +1     
==========================================
  Hits        11397    11397              
- Misses       5152     5153       +1     
  Partials       33       33              
Impacted Files Coverage Δ
packages/jest-config/src/setFromArgv.ts 86.11% <50.00%> (-2.47%) ⬇️

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 8024306...05f4239. Read the comment docs.

@mrazauskas mrazauskas changed the title fix(cli): improve description of testEnvironmentOptions option fix(jest-config): parse testEnvironmentOptions if received from CLI Sep 28, 2021
@mrazauskas
Copy link
Contributor Author

@SimenB Perhaps it is not worth to include this option in the versioned CLI docs, because most probably the implementation is buggy. Or?

@SimenB
Copy link
Member

SimenB commented Sep 28, 2021

@SimenB Perhaps it is not worth to include this option in the versioned CLI docs, because most probably the implementation is buggy. Or?

Agreed 👍

You can add it do 27.2 as I'll publish a patch with this 🙂

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks! just the one versioned docs change and this should be good to go 🙂

case 'transform':
case 'haste':
const str = argv[key];
if (isJSONString(str)) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should throw if it returns false... not for this PR anyways

docs/CLI.md Outdated Show resolved Hide resolved
@SimenB SimenB merged commit 7e40893 into jestjs:main Sep 28, 2021
@SimenB
Copy link
Member

SimenB commented Sep 28, 2021

Thanks!

@mrazauskas mrazauskas deleted the docs-fix-testEnvironmentOptions branch September 28, 2021 10:11
@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 Oct 29, 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.

[Bug]: testEnvironmentOptions documentation/types are wrong
4 participants