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

Roll back micromatch #6661

Merged
merged 2 commits into from
Jul 9, 2018
Merged

Conversation

Tvrqvoise
Copy link
Contributor

@Tvrqvoise Tvrqvoise commented Jul 9, 2018

Summary

As part of #6400, micromatch was updated. While micromatch's CHANGELOG claims that this is safe, several regressions have been noted in cases where users relied upon invalid glob patterns. For example, these patterns would all match src/foo/bar/baz.js, but no longer do:

  • src/**/*.{js}
  • src/**.js
  • src/**/*.{js|ts}

Fixes #6563
Fixes #6546

Test plan

Unit tests were added which demonstrate the known cases.

@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. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

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

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

1 similar comment
@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!

@Tvrqvoise
Copy link
Contributor Author

I don't understand why "test-node-8" is failing. It looks like a test within jest-diff has a leak, but I did not touch jest-diff. Can someone with more permissions either re-run the job or take a look?

@thymikee thymikee changed the title Roll back micromatch, fixes #6563 Roll back micromatch Jul 9, 2018
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6661   +/-   ##
=======================================
  Coverage   63.73%   63.73%           
=======================================
  Files         235      235           
  Lines        8940     8940           
  Branches        4        4           
=======================================
  Hits         5698     5698           
  Misses       3241     3241           
  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 fa92643...7bda341. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Jul 9, 2018

@mjesun @cpojer I think this is worth a patch release

@thymikee thymikee merged commit e930c70 into jestjs:master Jul 9, 2018
@mjesun
Copy link
Contributor

mjesun commented Jul 9, 2018

I’ll take care tomorrow of it 🙂

@davidleacy
Copy link

Definitely appreciate a patch release for this, currently blocking package updates for one of our projects.

@mjesun
Copy link
Contributor

mjesun commented Jul 10, 2018

We'll do it. I'm waiting for some PRs to get merged and we'll perform a minor.

@jonschlinkert
Copy link

Hey folks, I'm the author of micromatch, just wanted to let you know that we'll be looking into the issue to try to understand what's happening. Based on the information provided by @Tvrqvoise, there shouldn't be any issues with those glob patterns.

While we look into it, my hunch is that it's one of the following:

  1. an option needs to be set (or unset), such as unixify which ensures file paths use forward slashes on windows.
  2. Node's path module is being used to join glob patterns or make them absolute, etc, and something isn't being joined correctly.

In the meantime, if anyone has any more info they can share, please feel free to create an issue on the micromatch repository so we can get to the bottom of this and figure it out. thanks!

@rickhanlonii
Copy link
Member

Thanks @jonschlinkert!

@Tvrqvoise
Copy link
Contributor Author

Tvrqvoise commented Jul 11, 2018

@jonschlinkert, I can see differing behavior for the test cases I added. I'm on OSX High Sierra, for what it matters. I'll throw together a minimal test case on your repository's issues page tomorrow.

@jonschlinkert
Copy link

I can see differing behavior for the test cases I added in the unit test. I'm on OSX High Sierra, for what it matters. I'll throw together a minimal test case on your repository tomorrow.

Thanks!

calebeby referenced this pull request in Pigmice2733/scouting-frontend Jul 11, 2018
This Pull Request updates dependency [jest](https://github.com/facebook/jest) from `v23.3.0` to `v23.4.0`



<details>
<summary>Release Notes</summary>

### [`v23.4.0`](https://github.com/facebook/jest/blob/master/CHANGELOG.md#&#8203;2340)
[Compare Source](jestjs/jest@v23.3.0...v23.4.0)
##### Features

- `[jest-haste-map]` Add `computeDependencies` flag to avoid opening files if not needed ([#&#8203;6667](`https://github.com/facebook/jest/pull/6667`))
- `[jest-runtime]` Support `require.resolve.paths` ([#&#8203;6471](`https://github.com/facebook/jest/pull/6471`))
- `[jest-runtime]` Support `paths` option for `require.resolve` ([#&#8203;6471](`https://github.com/facebook/jest/pull/6471`))
##### Fixes

- `[jest-runner]` Force parallel runs for watch mode, to avoid TTY freeze ([#&#8203;6647](`https://github.com/facebook/jest/pull/6647`))
- `[jest-cli]` properly reprint resolver errors in watch mode ([#&#8203;6407](`https://github.com/facebook/jest/pull/6407`))
- `[jest-cli]` Write configuration to stdout when the option was explicitly passed to Jest ([#&#8203;6447](`https://github.com/facebook/jest/pull/6447`))
- `[jest-cli]` Fix regression on non-matching suites ([6657](`https://github.com/facebook/jest/pull/6657`))
- `[jest-runtime]` Roll back `micromatch` version to prevent regression when matching files ([#&#8203;6661](`https://github.com/facebook/jest/pull/6661`))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
@jonschlinkert
Copy link

jonschlinkert commented Jul 11, 2018

I didn't catch this before, but all three of the patterns mentioned by @Tvrqvoise in the OP are invalid globs. (since I didn't catch it, and I spend a lot of time looking at globs, it's obviously... um, not obvious).

I go into more detail here.

@SimenB
Copy link
Member

SimenB commented Jul 11, 2018

Thanks for taking a look! I think the changes in v3 are all fine, but I also think holding off on the upgrade until the next major of Jest makes sense - it broke a lot of people's setup. Even though it broke because of invalid globs I think this should come in a major, along with these exact examples instructing people on how to migrate :)

@jonschlinkert
Copy link

Even though it broke because of invalid globs I think this should come in a major, along with these exact examples instructing people on how to migrate :)

Agreed! In the meantime I'm happy to help if anyone has issues.

Regarding the Windows path issue, in case it helps anyone for now, on our own projects we use to-absolute-glob to, um, ...create absolute globs (I seem to be doing this a lot. I must be stuck in some kind of language recursion loop lol).

image

@SimenB
Copy link
Member

SimenB commented Aug 9, 2018

@jonschlinkert do you by any chance know of a module that can validate the globs? We could print a deprecation warning for Jest 23 to help people fix their globs in time for jest 24

@jonschlinkert
Copy link

@SimenB that's an interesting question. Let me think about that, and how it would work. I might have a couple of ideas. Off-hand, it seems like a catch-22, meaning that a validator couldn't be trusted for the same reason that we can't say for sure if the user intended to use backslashes as escape characters versus path separators. The only way to know for sure is to have the path before it's joined to the glob.

However, it's quite possible that it's much simpler and I'm hung up on something that isn't important. Have there been any issues reported by users related to this yet? If I could see specific scenarios it might help me understand your use case more thoroughly and come up with ideas. Either way I'd be happy to help.

@SimenB
Copy link
Member

SimenB commented Aug 9, 2018

Escaping vs path separator is one thing, I'm thinking more of the simple cases such as the ones mentioned in the OP in this PR.

It doesn't have to be perfect, but if we can flag some obviously wrong ones that'd be awesome, and waaay better than nothing 🙂

@jonschlinkert
Copy link

jonschlinkert commented Aug 9, 2018

Ah, got it. I might need some clarification then, since my understanding was that this PR was user-error since the patterns listed are all invalid glob patterns (which we didn't notice at first).

Fwiw, we haven't had any other issues related to matching or regressions on micromatch itself. Not that issues can't surface here or elsewhere, but IMHO this issue here wasn't really an issue, as it was a false-negative.

Perhaps this comment will shed some light on that point.

edit: btw my intention isn't to be dismissive of the issue, I'm still very happy to help with whatever resolution is needed. Also fwiw, I mention in that comment about "being more accurate", but we haven't had any other issues about it besides that one.

@SimenB
Copy link
Member

SimenB commented Aug 9, 2018

my understanding was that this PR was user-error since the patterns listed are all invalid glob patterns (which we didn't notice at first).

Yes exactly. What I meant is that it would be great if we could say "hey, these globs are invalid, please fix them as they will stop working" for user-supplied patterns

@juliancoleman
Copy link

Is there any word on this? Tests still fail on Windows with jest@^23.5.0

@SimenB
Copy link
Member

SimenB commented Sep 4, 2018

@juliancoleman micromatch has been rolled back in that version, so id you still have issues I suggest opening up a new issue 🙂

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

Since Jest 23.2.0: "Coverage data for global was not found." Jest cant find any test with 23.2.0
10 participants