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

chore: upgrade to micromatch 3 #6650

Merged
merged 38 commits into from
Jan 18, 2019
Merged

Conversation

aldarund
Copy link
Contributor

@aldarund aldarund commented Jul 6, 2018

Summary

Fixes #6546 .
From micromatch docs ->

when forward slashes are defined in a glob pattern, the resulting regular expression will match windows or POSIX path separators just fine.

Test plan

Same config as in referenced issue work fine with this fix

yarn test
yarn run v1.8.0-20180601.1611
$ jest
 PASS  tests/unit/components/passwordValidation.spec.js
 PASS  tests/unit/components/paymentForm.spec.js
 PASS  tests/unit/pages/index.spec.js
 PASS  tests/unit/components/loginForm.spec.js
 PASS  tests/unit/pages/signup.spec.js (5.426s)

Test Suites: 5 passed, 5 total
Tests:       14 passed, 14 total
Snapshots:   0 total
Time:        8.644s
Ran all test suites.
Done in 10.40s.

@SimenB
Copy link
Member

SimenB commented Jul 7, 2018

Thanks for the PR! Mind adding a test as well?

@thymikee
Copy link
Collaborator

thymikee commented Jul 7, 2018

@aldarund
Copy link
Contributor Author

aldarund commented Jul 7, 2018

@SimenB there is already tests that was able to catch this problem, but they are disabled on windows
https://github.com/facebook/jest/blob/master/packages/jest-cli/src/__tests__/search_source.test.js#L28
I have checked it and there only 1 test fails on windows ( last one with /foo/bar/prefix path ). But there other test that fail without this fix and work with it. So tests exists but they all disabled due to one test.

@thymikee ye, i saw, and there also similar thing in jest-regex-util, but not exactly same.

@thymikee
Copy link
Collaborator

thymikee commented Jul 7, 2018

@aldarund feel free to wrap this test single test in process.platform === 'win32' check

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.

This is great, thank you so much or tackling it!

Mind adding an entry to the changelog?

Might be a good idea to rebase first, as you'll probably get a conflict

@codecov-io
Copy link

codecov-io commented Jul 7, 2018

Codecov Report

Merging #6650 into master will decrease coverage by <.01%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6650      +/-   ##
==========================================
- Coverage   68.28%   68.27%   -0.01%     
==========================================
  Files         249      250       +1     
  Lines        9628     9633       +5     
  Branches        5        6       +1     
==========================================
+ Hits         6574     6577       +3     
- Misses       3052     3054       +2     
  Partials        2        2
Impacted Files Coverage Δ
packages/jest-cli/src/runJest.js 75% <ø> (ø) ⬆️
packages/jest-runtime/src/shouldInstrument.js 100% <ø> (ø) ⬆️
packages/jest-message-util/src/index.js 82.3% <ø> (ø) ⬆️
packages/jest-util/src/index.js 0% <ø> (ø) ⬆️
packages/jest-haste-map/src/HasteFS.js 50% <0%> (ø) ⬆️
packages/jest-util/src/replacePathSepForGlob.js 0% <0%> (ø)
packages/jest-cli/src/SearchSource.js 58.75% <100%> (+0.52%) ⬆️
packages/jest-config/src/normalize.js 83.01% <75%> (-0.19%) ⬇️

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 cfc1782...9e28572. Read the comment docs.

@aldarund
Copy link
Contributor Author

aldarund commented Jul 7, 2018

@SimenB done

@SimenB
Copy link
Member

SimenB commented Jul 7, 2018

@mjesun this might be worth a patch release

@thymikee
Copy link
Collaborator

thymikee commented Aug 8, 2018

We've switched back to micromatch 2. Is this still necessary?

@SimenB
Copy link
Member

SimenB commented Jan 13, 2019

Not sure about the last commit, although it seems correct: https://www.npmjs.com/package/micromatch#backslashes

@SimenB
Copy link
Member

SimenB commented Jan 13, 2019

Finally green. The normalization is super ugly, but I think we should clean that up in jest 25 with #7185

The issue here is that globs need forward slash as separator, which doesn't work for regexes (at least not how we use them currently). I think it'll be easier to normalize correctly when it's all globs (or we'll at least be forced to go through where it's used and fix it properly)

@thymikee
Copy link
Collaborator

Finally green. The normalization is super ugly, but I think we should clean that up in jest 25 with #7185

Agreed. Btw, how about placing replacePathSepForGlob inside jest-util and use it consistently for all globs?

@thymikee
Copy link
Collaborator

Looks like we're pretty consistent now in terms of escaping globs for micromatch. Would be even better to extract a helper for matching globs that uses micromatch as its implementation detail, which seems like a nice followup refactor to this PR :)

@SimenB
Copy link
Member

SimenB commented Jan 14, 2019

I can try to PR replacePathSepForGlob to micromatch - seems like a nice utility function it should export?

@thymikee
Copy link
Collaborator

thymikee commented Jan 14, 2019

Would be awesome if they agreed! Maybe it's better to ask first

@SimenB
Copy link
Member

SimenB commented Jan 14, 2019

Done: micromatch/micromatch#142

@SimenB SimenB merged commit 44054af into jestjs:master Jan 18, 2019
@SimenB
Copy link
Member

SimenB commented Jan 18, 2019

🤞

hopefully it'll work out 🙂

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

Jest cant find any test with 23.2.0
5 participants