Only match relative paths when specifying paths #12519

brandon-leapyear · 2022-03-01T02:38:46Z

Summary

Adds a new TestPathPatterns class which consolidates all the logic related to checking if a test path matches user-specified patterns. This class was necessary to add because previously, GlobalConfig just stored the final regex in testPathPattern, which would be shown in output like:

Ran tests matching pattern: <testPathPattern regex>

But with this change, the regex could get rather complicated; e.g. if a user ran jest foo bar in /Users/myuser/github/my-project, the output would be:

Ran tests matching pattern: /\/Users\/myuser\/github\/my-project\/(.*)foo|\/Users\/myuser\/github\/my-project\/(.*)bar/

So we need to separate the notion of "how to match paths against patterns" from "how to show the patterns to the user" in the config. So instead of the config storing the final regex, it now stores the original list of patterns specified and then builds the regex on demand; in fact, the reality that TestPathPatterns uses a regex is now an implementation detail (it pretty much only exposes the isMatch function as the public entrypoint), so if the pattern-matching logic needs to be more complex than a regex can handle, and we want to just do a naive

for (const pattern of self.patterns) {
  if (isRegex(pattern) && pattern.test(path)) {
    return true
  }
  if (isOtherComplicatedPattern(pattern) && pattern.matches(path)) {
    return true
  }
}
return false

we're now free to do so!

Commit explanation

Pure refactoring + simplification
- Factor out toString() call
- Remove redundant 'globalConfig.testPathPattern != null' check
Add + use TestPathPatterns
- Consolidate testPathPatterns logic
- Return TestPathPatterns in normalize helper
- Store parsed patterns in global config
- Change normalize tests to test testPathPatterns
- Replace testPathPatternToRegExp with TestPathPatterns
- Update testPathPatterns in updateGlobalConfig
- Update docs + tests to use globalConfig.testPathPatterns
- Replace remaining usages of globalConfig.testPathPattern
- Fix tests using partial GlobalConfigs
- Remove testPathPattern from GlobalConfig
Fix relative path issue
- Add failing tests for matching outside relative path
- Only match within relative path
Update CHANGELOG

Test plan

Running yarn jest --listTests user before this branch listed all tests on a Mac, where everything is under /Users/<username/. After this branch, it only shows the users.test.js tests

SimenB · 2022-03-01T07:31:51Z

Oh, looks like a pretty large refactor! Ping me when you have something semi-working and I'll take a look! 🙂

brandon-leapyear · 2022-03-05T01:58:16Z

@SimenB looks ready to go!

SimenB · 2022-03-05T10:44:04Z

This disconnects --testPathPattern from testPathPattern in config, any particular reason for that? I think it's confusing those are different

brandon-leapyear · 2022-03-05T17:57:34Z

@SimenB how's that?

github-actions · 2022-06-03T23:25:16Z

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

brandonchinn178 · 2022-06-03T23:36:05Z

@SimenB ping

github-actions · 2022-09-01T23:46:22Z

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

brandonchinn178 · 2022-09-01T23:50:09Z

@SimenB

github-actions · 2022-12-01T00:08:05Z

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

brandon-leapyear · 2022-12-01T00:17:17Z

Not stale

legobeat · 2023-04-25T03:32:33Z

@brandon-leapyear @SimenB needs rebase on main

netlify · 2023-04-26T02:02:19Z

✅ Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`09aed7d`
🔍 Latest deploy log	https://app.netlify.com/sites/jestjs/deploys/651ab6141b358d00073e2b83
😎 Deploy Preview	https://deploy-preview-12519--jestjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

linux-foundation-easycla · 2023-04-26T02:02:47Z

The committers listed above are authorized under a signed CLA.

✅ login: brandonchinn178 / name: Brandon Chinn (344bc43, 317d079, 86993d4, ecd979f, 4caaa45, 225b6fe, 993fc50, 518c357, e347e72, 6713c2c, a4efd87, ae8ee58, 1911750, 5fd3eda, 0225d22, e228478, de9edca, 042fddd, a52fc2f, 70a3999, 3c22ef3)
✅ login: SimenB / name: Simen Bekkhus (0b22568, 09aed7d)

brandonchinn178 · 2023-04-26T02:28:35Z

@legobeat done

SimenB · 2023-09-20T07:12:06Z

Hey @brandon-leapyear! Finally ready to land this - could I bother you with one last rebase? 🙏

SimenB · 2023-09-21T09:59:05Z

Thanks! There's some CI errors - could you take a look?

brandonchinn178 · 2023-09-22T02:40:23Z

@SimenB Updated!

SimenB

just some quick feedback (currently travelling) - overall it looks really good!

CHANGELOG.md

SimenB · 2023-09-24T06:53:22Z

packages/jest-util/src/TestPathPatterns.ts

+   * for simplicity.
+   */
+  toPretty(): string {
+    const regex = this.patterns.map(p => p.replace(/\//g, '\\/')).join('|');


why do we want the escape characters? for copy paste?

I'm not sure I find it very "pretty" 😅

ah I was trying to imitate what the original code was doing. Instead of doing this manually, I really should just return this.regex.toString().

And, sure, maybe it's not "pretty" anymore, so I'll rename the function to toString (really it's just rendering the patterns back to a string to show to the user)

Ah that's right, I can't do this.regex.toString() because the regex now contains extra information about the root directory:

# Original message Ran all test suites matching /a\/b|c\/d/i. # TestPathPatterns.toPretty() Ran all test suites matching /a\/b|c\/d/i. # TestPathPatterns.regex.toString() Ran all test suites matching /\/(.*)?a\/b|\/(.*)?c\/d/i.

It's "pretty" in the sense that it's not literally the regex that we're using to match tests. If your main concern is the name of the function, I'm happy to rename it to something else, I just can't think of a better name

My main "objection" is that I find the escaped path separators to be harder to read than the original patterns

Is just more characters to ignore in order to read the pattern.

I guess I understand it's to render the regex flag, but still

Oh ok so currently, jest renders the "no tests found" and the "ran tests matching" messages differently:

$ jest foo/bar bar/baz --testPattern x/a Pattern: foo/bar|bar/baz|x/a - 0 matches Ran all test suites matching /foo\/bar|bar\/baz|x\/a/i.

Should these messages render the pattern the same way? Or should it be the case that the no-matches message shows a "pretty" version, and the tests-found message shows the actual regex?

If they should be rendered the same way, which way should they be rendered?

I'd personally prefer them to render without escape characters - it's just easier to read (same as we change backslashes to forward slashes for windows in reporters and such). And consistency is always good! 🙂

So 1. yes - 2. Pattern.

Updated: 3c22ef3

packages/jest-util/src/index.ts

packages/jest-util/src/TestPathPatterns.ts

SimenB

wonderful, thank you so much for your patience!

I think my only comment at this moment in time would be that it'd be nice to avoid reparsing the regexes all the time in the new TestPathPatterns class we instantiate all over the place. But that is by no means a blocker, so if it proves hard, I'm happy to merge this as-is 🙂

brandonchinn178 · 2023-10-03T03:59:11Z

I would weakly prefer merging as-is, mostly because I'm not sure where to put the initialized TestPathPatterns in a place reachable by everything. The obvious thing is to store it in GlobalConfig, but it seems like jest-util depends on @jest/types, so I can't pull it into there. And I'm not sure the entire TestPathPatterns logic belongs in @jest/types. If you could provide some guidance on how to go about this, I'm happy to, but I think it would equally fine to merge this already-large PR and do this in another PR?

github-actions · 2023-11-04T00:07:49Z

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.

facebook-github-bot added the cla signed label Mar 1, 2022

brandon-leapyear changed the title ~~Only match relative paths when specifying paths~~ [WIP] Only match relative paths when specifying paths Mar 1, 2022

SimenB marked this pull request as draft March 1, 2022 07:28

brandon-leapyear mentioned this pull request Mar 1, 2022

chore: refactor tests + clean up unused code in SearchSource #12526

Merged

brandon-leapyear marked this pull request as ready for review March 5, 2022 01:58

brandon-leapyear changed the title ~~[WIP] Only match relative paths when specifying paths~~ Only match relative paths when specifying paths Mar 5, 2022

github-actions bot added the Stale label Jun 3, 2022

github-actions bot removed the Stale label Jun 3, 2022

github-actions bot added the Stale label Sep 1, 2022

github-actions bot removed the Stale label Sep 1, 2022

github-actions bot added the Stale label Dec 1, 2022

github-actions bot removed the Stale label Dec 1, 2022

SimenB added this to the Jest 30 milestone Jan 1, 2023

SimenB added the Pinned label Jan 1, 2023

Factor out toString() call

344bc43

brandonchinn178 added 14 commits September 21, 2023 20:16

Consolidate testPathPatterns logic

86993d4

Return TestPathPatterns in normalize helper

ecd979f

Store parsed patterns in global config

4caaa45

Change normalize tests to test testPathPatterns

225b6fe

Replace testPathPatternToRegExp with TestPathPatterns

993fc50

Update testPathPatterns in updateGlobalConfig

518c357

Update docs + tests to use globalConfig.testPathPatterns

e347e72

Replace remaining usages of globalConfig.testPathPattern

6713c2c

Remove testPathPattern from GlobalConfig

a4efd87

Add failing tests for matching outside relative path

ae8ee58

Only match within relative path

1911750

Fix tests using partial GlobalConfigs

5fd3eda

Update CHANGELOG

0225d22

Fix --testPathPattern => --testPathPatterns

e228478

SimenB reviewed Sep 24, 2023

View reviewed changes

brandonchinn178 and others added 7 commits September 24, 2023 17:49

Don't overload TestPathPatterns constructor

de9edca

Update docs

042fddd

Add deprecation error for --testPathPattern

a52fc2f

Convert isValid() to validate()

70a3999

Make toPretty() render prettier output

3c22ef3

Merge branch 'main' into chinn/relative-paths

0b22568

oops

09aed7d

SimenB approved these changes Oct 2, 2023

View reviewed changes

SimenB merged commit 41133b5 into jestjs:main Oct 3, 2023
60 checks passed

brandonchinn178 deleted the chinn/relative-paths branch October 4, 2023 01:01

github-actions bot locked as resolved and limited conversation to collaborators Nov 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only match relative paths when specifying paths #12519

Only match relative paths when specifying paths #12519

brandon-leapyear commented Mar 1, 2022 •

edited

Loading

SimenB commented Mar 1, 2022

brandon-leapyear commented Mar 5, 2022

SimenB commented Mar 5, 2022

brandon-leapyear commented Mar 5, 2022

github-actions bot commented Jun 3, 2022

brandonchinn178 commented Jun 3, 2022

github-actions bot commented Sep 1, 2022

brandonchinn178 commented Sep 1, 2022

github-actions bot commented Dec 1, 2022

brandon-leapyear commented Dec 1, 2022

legobeat commented Apr 25, 2023

netlify bot commented Apr 26, 2023 •

edited

Loading

linux-foundation-easycla bot commented Apr 26, 2023 •

edited

Loading

brandonchinn178 commented Apr 26, 2023

SimenB commented Sep 20, 2023

SimenB commented Sep 21, 2023

brandonchinn178 commented Sep 22, 2023

SimenB left a comment

SimenB Sep 24, 2023

brandonchinn178 Sep 24, 2023

brandonchinn178 Sep 25, 2023

SimenB Sep 27, 2023 •

edited

Loading

brandonchinn178 Sep 27, 2023

SimenB Sep 28, 2023

brandonchinn178 Sep 30, 2023 •

edited

Loading

SimenB left a comment

brandonchinn178 commented Oct 3, 2023

github-actions bot commented Nov 4, 2023

Only match relative paths when specifying paths #12519

Only match relative paths when specifying paths #12519

Conversation

brandon-leapyear commented Mar 1, 2022 • edited Loading

Summary

Commit explanation

Test plan

SimenB commented Mar 1, 2022

brandon-leapyear commented Mar 5, 2022

SimenB commented Mar 5, 2022

brandon-leapyear commented Mar 5, 2022

github-actions bot commented Jun 3, 2022

brandonchinn178 commented Jun 3, 2022

github-actions bot commented Sep 1, 2022

brandonchinn178 commented Sep 1, 2022

github-actions bot commented Dec 1, 2022

brandon-leapyear commented Dec 1, 2022

legobeat commented Apr 25, 2023

netlify bot commented Apr 26, 2023 • edited Loading

✅ Deploy Preview for jestjs ready!

linux-foundation-easycla bot commented Apr 26, 2023 • edited Loading

brandonchinn178 commented Apr 26, 2023

SimenB commented Sep 20, 2023

SimenB commented Sep 21, 2023

brandonchinn178 commented Sep 22, 2023

SimenB left a comment

Choose a reason for hiding this comment

SimenB Sep 24, 2023

Choose a reason for hiding this comment

brandonchinn178 Sep 24, 2023

Choose a reason for hiding this comment

brandonchinn178 Sep 25, 2023

Choose a reason for hiding this comment

SimenB Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

brandonchinn178 Sep 27, 2023

Choose a reason for hiding this comment

SimenB Sep 28, 2023

Choose a reason for hiding this comment

brandonchinn178 Sep 30, 2023 • edited Loading

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

brandonchinn178 commented Oct 3, 2023

github-actions bot commented Nov 4, 2023

brandon-leapyear commented Mar 1, 2022 •

edited

Loading

netlify bot commented Apr 26, 2023 •

edited

Loading

linux-foundation-easycla bot commented Apr 26, 2023 •

edited

Loading

SimenB Sep 27, 2023 •

edited

Loading

brandonchinn178 Sep 30, 2023 •

edited

Loading