Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Filter non-JS files from require when using glob with --projects #5412

Merged
merged 6 commits into from
Feb 1, 2018

Conversation

ericclemmons
Copy link
Contributor

Summary

Fixes #5199, where a monorepo with packages/README.md was being required as a config file due to the glob from --projects packages/*.

This change filters out non-require-able files.

Test plan

yarn build
yarn jest integration-tests/__tests__/multi_project_runner.test.js --watch

@ericclemmons ericclemmons changed the title 5199 workspaces Fix non-JS files from being require when using glon with --patterns Jan 28, 2018
@ericclemmons ericclemmons changed the title Fix non-JS files from being require when using glon with --patterns Filter non-JS files from require when using glob with --projects Jan 28, 2018
@ericclemmons
Copy link
Contributor Author

It was difficult to pin down which part of the code was ultimately responsible, as the glob packages/* gets converted to actual file paths very early on.

So, because the list of projects are directly required or traversed later on, I explicitly decided to filter out files that would cause runtime errors when required.

@codecov-io
Copy link

codecov-io commented Jan 28, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5412   +/-   ##
=======================================
  Coverage   62.26%   62.26%           
=======================================
  Files         205      205           
  Lines        6925     6925           
  Branches        4        4           
=======================================
  Hits         4312     4312           
  Misses       2612     2612           
  Partials        1        1

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 0e55c7c...e57d517. Read the comment docs.

// Ignore globbed files that cannot be `require`d.
if (
fs.existsSync(root) &&
!fs.lstatSync(root).isDirectory() &&
Copy link
Member

Choose a reason for hiding this comment

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

should we just do a single lstatSync and try-catch it to avoid 2 separate IO operations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's also a possibility. My take is:

  • --projects is often single to double-digits of entries, whereas the difference between 1 lookup vs. 2 wasn't discernible in my testing. (I originally didn't have the directory check there)

  • If there are any edge-cases that aren't accounted for, we'll have the opportunity to see & correct the error in the future, vs. it being silently ignored (& not running any tests for that directory).

@cpojer cpojer merged commit dea270b into jestjs:master Feb 1, 2018
jessecarfb pushed a commit to jessecarfb/jest that referenced this pull request Feb 2, 2018
…estjs#5412)

* Add test for workspaces with a README.md in the root

* Filter projects to directories & require-able files only

This ensures `packages/*` catches folders & not README.md

* Add reference to jestjs#5199 in CHANGELOG.md

* Remove custom testDir
@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
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--projects should filter by directory
5 participants