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

Fix testPathPatterns when config is in subdirectory #14934

Merged
merged 21 commits into from
May 29, 2024
Merged

Fix testPathPatterns when config is in subdirectory #14934

merged 21 commits into from
May 29, 2024

Conversation

brandonchinn178
Copy link
Contributor

Summary

Resolves #14726. No CHANGELOG entry, as it fixes an issue with a new feature that's already in the CHANGELOG.

Follow up to #12519 (which fixes both #10746 and #8226). In this PR, we separate the TestPathPatterns class into two classes: one that just represents the list of patterns and one that can match the patterns against a test file. The separation is necessary because there are some uses where we aren't in a project, and can't match against a regex, but we do want to do static analysis of the patterns. For example, when outputting the test summary, we want to show the patterns the user specified, but the summary is global, not per-project.

I also made a change I had wanted to do in the initial PR, which is store TestPathPatterns directly in GlobalConfig. Now, the fact that patterns are represented as Array<string> is an implementation detail, and everything using the patterns can only use the provided API.

This will make even more sense when rootDir is removed from GlobalConfig, as discussed. It would be even nicer if ProjectConfig stored TestPathPatternsExecutor, but it was getting weird with InitialProjectConfig, so I didn't want to touch that.

Test plan

I wrote a new test that is executed with yarn jest testPathPatterns. It fails on main with "no tests found", and passes on this branch.

Copy link

netlify bot commented Mar 3, 2024

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit c22a670
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/6642bf571a92de000831be37
😎 Deploy Preview https://deploy-preview-14934--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@brandonchinn178
Copy link
Contributor Author

@SimenB Okay CI should be good now!

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.

so sorry about the slow reply!

The code overall looks good to me, I'd just like to see it not be in @jest/types. I can probably move it myself if you're busy 🙂

import {escapePathForRegex, replacePathSepForRegex} from 'jest-regex-util';

type PatternsConfig = {
export class TestPathPatterns {
Copy link
Member

Choose a reason for hiding this comment

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

didn't notice this was here before 😅

I don't wanna export non-types from this package. I'm fine with not having it in jest-util, but could you pull it out into its own new package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it can't be in jest-util because jest-util imports from @jest/types and TestPathPatterns is used in GlobalConfig, so there'd be a circular dependency.

But I broke it out into a new jest-pattern library and pushed a commit. Hopefully it builds fine

CHANGELOG.md Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented May 13, 2024

I also made a change I had wanted to do in the initial PR, which is store TestPathPatterns directly in GlobalConfig. Now, the fact that patterns are represented as Array<string> is an implementation detail, and everything using the patterns can only use the provided API.

I'm not too sure about this change. Currently the config is JSON serializable - that changes if we stick a class instance on it 🤔

@brandonchinn178
Copy link
Contributor Author

It should still be JSON serializable, because it implements toJSON

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#toJSON()_behavior

Pretty sure theres a test somewhere checking this

@SimenB
Copy link
Member

SimenB commented May 13, 2024

CI fails now with type errors in the tests. Not sure if that's something we wanna fix, or add the module to the ignore list:

jest/scripts/lintTs.mjs

Lines 32 to 33 in 343d450

// TODO: remove this list at some point and run against all packages
const packagesNotToTest = [

@brandonchinn178
Copy link
Contributor Author

hm I'll take a look

@brandonchinn178
Copy link
Contributor Author

@SimenB seems like its passing now!

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.

Sorry about the delay - thanks for sticking with us!

'--testPathPatterns=project-1',
'--testPathPatterns=setup1',
Copy link
Member

Choose a reason for hiding this comment

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

right, since this is relative to the project config now, and not the global config. I'm afraid this will be quite breaking as well (when using jets projects just to have a single jest command to run from root rather than as a way to isolate test suites), but let's try! 🙂

@SimenB SimenB merged commit 68dca2a into jestjs:main May 29, 2024
84 checks passed
@brandonchinn178 brandonchinn178 deleted the bchinn-testpathpatterns-subprojects branch June 1, 2024 06:21
Copy link

github-actions bot commented Jul 3, 2024

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 Jul 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: v30 alpha: testPathPatterns that worked in the past not working any more.
2 participants