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

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

Closed
phryneas opened this issue Nov 28, 2023 · 25 comments · Fixed by #14934
Closed

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

phryneas opened this issue Nov 28, 2023 · 25 comments · Fixed by #14934

Comments

@phryneas
Copy link
Contributor

Version

30.0.0-alpha.2

Steps to reproduce

yarn jest --config ./config/jest.config.js useLoadableQuery

Expected behavior

Jest finds the matching file src/react/hooks/__tests__/useLoadableQuery.test.tsx and executes it

Actual behavior

No tests found, exiting with code 1

Additional context

I understand that jest 30 had a change (in #12519) from a [TestPathPattern] option to [TestPathPatterns], but honestly, I'm failing to get anything to match this new option, even specifying the full path to the test doesn't work.

From the issue, it seems like this doesn't accept a RegEx anymore, but an array of patterns - but I'm not sure how to write those patterns. I'm getting errors like Invalid testPattern **/useLoadableQuery.* supplied. Running all tests instead..

This might be user error, but this change is very unexpected, and will probably seriously impact the workflows of most jest users and a ton of CI scripts, so I thought I'd open an issue to bring this to your attention.

Environment

System:
    OS: macOS 13.6
    CPU: (8) arm64 Apple M1 Pro
  Binaries:
    Node: 20.5.0 - ~/.nvm/versions/node/v20.5.0/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 9.8.0 - ~/.nvm/versions/node/v20.5.0/bin/npm
    pnpm: 8.7.6 - /opt/homebrew/bin/pnpm
  npmPackages:
    jest: 30.0.0-alpha.2 => 30.0.0-alpha.2
Copy link

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

@github-actions github-actions bot added the Stale label Dec 28, 2023
@SimenB
Copy link
Member

SimenB commented Dec 28, 2023

Do you have a full reproduction?

@github-actions github-actions bot removed the Stale label Dec 28, 2023
@phryneas
Copy link
Contributor Author

@SimenB I would assume that this literally happens in every project using jest - it's just a completely different file name matching logic than before now.

But if this is actually some edge case and not intended to happen, I can create a branch of the Apollo Client repo with jest 30 after the new year; it definitely happened there, but I rolled back without creating a commit/branch, because it was completely unusable.

@SimenB
Copy link
Member

SimenB commented Dec 28, 2023

It does not happen in this very repo, at least 🙂

image

@phryneas
Copy link
Contributor Author

I'll try to reproduce it once I'm back at the job :)

@SimenB
Copy link
Member

SimenB commented Dec 29, 2023

Cool 👍 Enjoy your vacation!

Ideally the reproduction is not the entire @apollo/client 😅

phryneas added a commit to apollographql/apollo-client that referenced this issue Jan 5, 2024
execute
```sh
node --expose-gc ./node_modules/jest/bin/jest.js --config ./config/jest.config.js useSuspense
```
@phryneas
Copy link
Contributor Author

phryneas commented Jan 5, 2024

So the easy (for me) reproduction is unfortunately @apollo/client 😓

Check out
https://github.com/apollographql/apollo-client/tree/reproduction/jest-30-alpha

and run

npm i
node --expose-gc ./node_modules/jest/bin/jest.js --config ./config/jest.config.js useSuspense

which will error with (I ran it with --verbose here)

No tests found, exiting with code 1
Run with --passWithNoTests to exit with code 0
In /Users/tronic/tmp/apollo-client/src
379 files checked.
testMatch: /tests//.?([mc])[jt]s?(x), **/?(.)+(spec|test).?([mc])[jt]s?(x) - 142 matches
testPathIgnorePatterns: .tsx$ - 317 matches
testRegex: - 0 matches
In /Users/tronic/tmp/apollo-client/src
379 files checked.
testMatch: /tests//.?([mc])[jt]s?(x), **/?(.)+(spec|test).?([mc])[jt]s?(x) - 142 matches
testPathIgnorePatterns: .ts$, src/react/hooks/tests/useSuspenseQuery.test.tsx, src/react/hooks/tests/useBackgroundQuery.test.tsx, src/react/hooks/tests/useLoadableQuery.test.tsx, src/react/hooks/tests/useQueryRefHandlers.test.tsx, src/react/query-preloader/tests/createQueryPreloader.test.tsx - 75 matches
testRegex: - 0 matches
In /Users/tronic/tmp/apollo-client/src
379 files checked.
testMatch: /tests//.?([mc])[jt]s?(x), **/?(.)+(spec|test).?([mc])[jt]s?(x) - 142 matches
testPathIgnorePatterns: .ts$ - 80 matches
testRegex: - 0 matches
Pattern: useSuspense - 0 matches

The test setup here contains three projects, and the third one should match the file src/react/hooks/__tests__/useSuspenseQuery.test.tsx.

I'll try if I can distill this down to a smaller reproduction, but already wanted to post this here :)

@phryneas
Copy link
Contributor Author

phryneas commented Jan 5, 2024

I found the issue, and it has nothing to do with the new name matching!

The config file is in config/jest.config.js and has rootDir: "src".

In the past, that just worked, but version 30 is now looking in config/src, not in src.

I'm not sure - I did not see this pointed out in the changelog, but I might have missed it.

In case it's not there, it might be a good idea to point this change out, but feel free to close this issue if it's not actionable :)

See next post.

@phryneas
Copy link
Contributor Author

phryneas commented Jan 5, 2024

Follow-up:

"../src" doesn't work as soon as I get projects involved again:

● Validation Error:

Directory ../src in the rootDir option was not found.

So I switched it over to

   rootDir: `${__dirname}/../src`,

and it's failing to find the tests again.

So this might actually not be resolved? Do any alarm bells ring for you in combination with projects?

@phryneas
Copy link
Contributor Author

phryneas commented Jan 5, 2024

I believe I found it!

If you have a config file in a different folder, testPathPatterns are built relative to that folder and not the root folder:
image

The /config should not be part of the pattern here.

@phryneas
Copy link
Contributor Author

phryneas commented Jan 5, 2024

It seems like the rootDir in the config is overwritten somehow.

I hardcoded it to rootDir: "/Users/tronic/tmp/apollo-client/src" in the config file, and it still end up as '/Users/tronic/tmp/apollo-client/config' in the config object that is passed into the TestPathPatterns instance.

image

@phryneas
Copy link
Contributor Author

phryneas commented Jan 5, 2024

This seems to be caused by the test path pattern being generated from the global config, not the project config

https://github.com/jestjs/jest/blob/c04d13d7abd22e47b0997f6027886aed225c9ce4/packages/jest-util/src/TestPathPatterns.ts#L23C1-L25

In our case and with that knowledge, I can work around this by adding a global rootDir option like

module.exports = {
  rootDir: `../src`,
  projects: [tsStandardConfig, standardReact17Config, standardReact18Config],
};

But I assume it might be generally not correct to create the testPathPatterns relative to the "main" rootDir - project root dirs could point to completely different folders.

@SimenB
Copy link
Member

SimenB commented Jan 5, 2024

We looked at global config's root dir in v29 as well (have done for years). However, I do agree that seems odd 🤔

@brandon-leapyear thoughts on this one?

@brandonchinn178
Copy link
Contributor

brandonchinn178 commented Jan 5, 2024

FYI In the future, can you ping @brandonchinn178 instead? That account was my account for a company I worked at that has been acquired. Thanks!

Ah yes, so the test pattern logic used to not care about a root directory at all, it just searched the whole test file path. Now, the test pattern is actually sensitive on the root directory, and I just used the rootDir setting in the global config to fill this in. I didn't know that "project configs" was a concept that existed.

I think the best fix would be to change TestPathPatterns.fromGlobalConfig(GlobalConfig) to TestPathPatterns.fromConfig(GlobalConfig, ProjectConfig). Another option would be to remove fromGlobalConfig entirely and always use the TestPathPatterns constructor, but then someone else might call the constructor with globalConfig.rootDir again, if they also don't realize there's a difference between globalConfig.rootDir/projectConfig.rootDir

@phryneas
Copy link
Contributor Author

phryneas commented Jan 5, 2024

Is there a reason to include a directory at all? From looking through the code with the debugger, it seems to me like these tests will only be applied to files that have been found within a project's rootDir in the first place?

@brandonchinn178
Copy link
Contributor

it seems to me like these tests will only be applied to files that have been found within a project's rootDir in the first place?

Correct, but the pattern needs to only match the relative path of the files, while the list of files is stored as a list of absolute paths. So we need to apply the pattern to only the relative part of the path, however we define what "relative" means. So the two options are:

  1. Add the "base directory" to the pattern and match the absolute paths
  2. Strip the "base directory" from the list of files and match the pattern verbatim

I chose to do the first way, but the second is equally valid. But either way, you need to define the directory the paths should be considered relative against.

To reiterate the motivation for the PR, I was trying to run tests in my project named users.spec.js, but running jest users returned all tests because the absolute path of the files on my Mac is /Users/<username?...

@SimenB
Copy link
Member

SimenB commented Jan 6, 2024

I honestly don't really understand why we have rootDir on both places 😅 Just having it in ProjectConfig seems correct to me

Copy link

github-actions bot commented Feb 5, 2024

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

@github-actions github-actions bot added the Stale label Feb 5, 2024
@phryneas
Copy link
Contributor Author

phryneas commented Feb 5, 2024

Re: stale.
Has this been addressed?

@brandonchinn178
Copy link
Contributor

@phryneas probably not.

@SimenB If you agree that rootDir should only be in ProjectConfig and not GlobalConfig, should we open a PR for that change now?

@github-actions github-actions bot removed the Stale label Feb 5, 2024
@SimenB SimenB added the Pinned label Feb 6, 2024
@SimenB SimenB added this to the Jest 30 milestone Feb 6, 2024
@SimenB
Copy link
Member

SimenB commented Feb 6, 2024

I did start messing around with removing a bunch of config from global that should really be on project, but that quickly turned unwieldy.

For testPahPatterns's purposes though, we can probably migrate it over without touching GlobalConfig and ProjectConfig itself. I'd love a PR for that 👍

@SimenB
Copy link
Member

SimenB commented Mar 1, 2024

@brandonchinn178 would you be able to send that PR? 🙂

@brandonchinn178
Copy link
Contributor

@SimenB PR submitted!

@SimenB
Copy link
Member

SimenB commented May 30, 2024

@phryneas sorry about the delay - can you give https://github.com/jestjs/jest/releases/tag/v30.0.0-alpha.5 a whirl?

Copy link

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

Successfully merging a pull request may close this issue.

3 participants