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

Pass retainAllFiles from haste config #2796

Conversation

Reanmachine
Copy link

Summary

This change is to address the problems blocking create-react-app#1081, create-react-app#607, and is based on the conversation at #2318.

I have poured over the logic in jest-runtime and jest-haste-map related to how node_modules/ and general ignore patterns worked and read the histories of all the tickets in this chain all the way back to the original discussion about absolute paths in create-react-app and I have come to the conclusion that the safest way forward here is to simply pass the retainAllFiles flag forward from the jest.haste configuration.

While looking into modifying hastemap's _ignore or _isNodeModulesDir to support the spec of testPathIgnorePatterns I realised that we cannot make it "just work" out of the box without some rather large changes to the way the ignore patterns system works.

This is because the core behaviour is designed to treat all node_modules/ folders as ignored except for specific whitelisted children of node_modules/ configured by config.haste.providesModuleNodeModules setting.

Originally I figured we could use the test ignored patterns to configure additional rules in the whitelist but the reality is that we're dealing with an inverted problem here. The whitelist exempts certain folders from being excluded, where the testPathIgnorePatterns exempts paths. They're contrary and building a complex regular expression to cover the inverse of an exception would leave too many holes or be overly complicated.

This led me to feel that we couldn't really keep the _isNodeModulesDir logic and make the testPathIgnorePatterns override this without changing a core assumption about how jest analyses a project.

Conclusion

I feel that making a conscious decision to disable the assumption that a user doesn't want node_modules/ folders examined for tests and then explicitly crafting ignore patterns for the places they don't want scanned is the safest, least-disruptive, and most apt way forward for this concept.

  • Because this is a new feature flag it ensures that it's backwards compatible with existing projects, no behaviour will change since jest users could not configure retainAllFiles before.
  • This also leaves the functionality of jest-haste-map intact, meaning the impact on the facebook reliant modules referenced by @cpojer in WIP: haste ignores using testPathIgnorePatterns, not all node_modules #2318 should be unaffected
  • This relies on existing behavior at it's core, jest-haste-map already has a use case for disabling the _isNodeModulesDir check
  • This will allow create-react-app to document a manner which allows for absolute paths or src/node_modules to be used which was discussed as the preferred way forward in create-react-app#1065

Test plan

I have included a new integration test haste-retain-all-ignore-paths which demonstrates the selection behaviour for tests with the flag turned on.

Additionally I noticed that Runtime.createHasteMap did not have any unit tests (or I couldn't find any) so I've included a few verifying that the value is passed through to the HasteMap properly if provided or defaulted to false if not.

The entire project passes lint, flow check, and a full npm run test pass with no errors.

Lint:
Lint Output

Flow:
Flow Output

Tests:
Test Output

@facebook-github-bot
Copy link
Contributor

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 - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot
Copy link
Contributor

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

@Reanmachine
Copy link
Author

Hrm, It seems that the tests are failing on windows parsing the JSON output from running the integration test. My initial analysis turned up nothing:

PS E:\Code\jest\integration_tests\haste-retain-all-ignore-paths> node ..\..\packages\jest-cli\bin\jest.js --json
 PASS  example\src\node_modules\src-library\__tests__\should-run.js
  √ stub (2ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        0.707s, estimated 1s
Ran all test suites.
{"numFailedTestSuites":0,"numFailedTests":0,"numPassedTestSuites":1,"numPassedTests":1,"numPendingTestSuites":0,"numPendingTests":0,"numRuntimeErrorTestSuites":0,"numTotalTestSuites":1,"numTotalTests":1,"snapshot":{"added":0,"failure":false,"filesAdded":0,"filesRemoved":0
,"filesUnmatched":0,"filesUpdated":0,"matched":0,"total":0,"unchecked":0,"unmatched":0,"updated":0},"startTime":1486119613794,"success":true,"testResults":[{"assertionResults":[{"failureMessages":[],"status":"passed","title":"stub"}],"endTime":1486119614495,"message":"","
name":"E:\\Code\\jest\\integration_tests\\haste-retain-all-ignore-paths\\example\\src\\node_modules\\src-library\\__tests__\\should-run.js","startTime":1486119613814,"status":"passed","summary":""}],"wasInterrupted":false}
PS E:\Code\jest\integration_tests\haste-retain-all-ignore-paths>

I will have to have a look at why this fails in AppVeyor & Windows but passes in linux tomorrow as it's rather late.

@Reanmachine
Copy link
Author

Nevermind, I was missing the skipOnWindows call since the integration test relies on a nested jest call and windows doesn't support the hash-bang execution.

@Reanmachine
Copy link
Author

Just noticed this has some conflicts, I'll rebase this onto the tip of master later today & fix it up.

Before completing work on the fix we're adding a test-case
for the problem we're fixing. This allows us to demonstrate
a scenario where we want to have jest only ignore the files
in the testPathIgnorePatterns without making assumptions
about node_modules/ paths.
Passing retainAllFiles lets us configure haste to be
allowed to not make assumptions about node_modules/
folders while keeping the default behavior.
npm script clean-all was deleting the files in this example
so we moved it up a level to avoid the wildcard rm
It seems that the nested jest call doesn't work on windows
very well due to the reliance on the hashbang in the jest
cli bin file. Other integration tests that use this technique
skip the test on windows (I imagine for similar reasons).
Prettier was applied to the jest project some time after the original submission of this ticket. Following the rebase I've applied the changes from prettier from my code for consistency.
@Reanmachine Reanmachine force-pushed the reanmachine/haste-retain-all-files branch from f8bd236 to 334a489 Compare April 13, 2017 07:40
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2796   +/-   ##
======================================
  Coverage    63.9%   63.9%           
======================================
  Files         176     176           
  Lines        6475    6475           
  Branches        4       4           
======================================
  Hits         4138    4138           
  Misses       2336    2336           
  Partials        1       1
Impacted Files Coverage Δ
packages/jest-runtime/src/index.js 75% <ø> (ø) ⬆️

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 d9c2f99...334a489. Read the comment docs.

@cpojer
Copy link
Member

cpojer commented May 11, 2017

For now I prefer not to make any changes here. We'll consider how to improve this in the future.

@cpojer cpojer closed this May 11, 2017
@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 13, 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.

4 participants