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: Fix incorrect rootPath resolution in jest plugin #833

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

tryggvigy
Copy link
Contributor

@tryggvigy tryggvigy commented Nov 12, 2024

This PR

Stops joining contructing a jest config rootDir via join(configFileDir, localConfig.rootDir)
when a localConfig rootDir is defined. Instead, we always prefer the local config rootDir and fall back to configFileDir.

I also include a new unit test that tests that this bug is fixed.

Context

I noticed that jest setup files that used the '<rootDir>/...' alias in places like setupFilesAfterEnv were resolving incorrectly. For example,

when I specify

config: ['tests/framework/e2e/jest-configs/jest.config.base.ts']

in the jest plugin,

Where jest.config.base.ts contains:

// abs path to repo root (/Users/me/work/repo/)
rootDir: REPO_ROOT_PATH,
setupFilesAfterEnv: [
    '<rootDir>/tests/framework/e2e/setup-files/setup.base.ts',
  ],

I get the following incorrect resolve:

'deferResolve:/Users/me/work/repo/tests/framework/e2e/jest-configs/Users/me/work/repo/tests/framework/e2e/setup-files/setup.base.ts (tests/framework/e2e/jest-configs/jest.config.base.ts)'

Where it was joining

  • the directory of the config file path (/Users/me/work/repo/tests/framework/e2e/jest-configs/)
  • with the specified rootDir in the config file (/Users/me/work/repo/)
  • and then adding the rest of the path (tests/framework/e2e/setup-files/setup.base.ts)

This is happening

I might be wrong, I don't have all the context, but this feels like wrong behaviour.

@webpro
Copy link
Collaborator

webpro commented Nov 12, 2024

Thanks for the PR, @tryggvigy!

Would be great if you could add fixtures for this case. There's one for Jest in packages/knip/fixtures/plugins/jest, you could add a folder next to it like jest2. And then use it from a new test next to packages/knip/test/plugins/jest.test.ts.

And there's DEVELOPMENT.md which might be helpful here.

@tryggvigy
Copy link
Contributor Author

Thanks for the pointers @webpro, I'll try to get some test coverage on this asap

Copy link

pkg-pr-new bot commented Nov 13, 2024

Open in Stackblitz

bun add https://pkg.pr.new/knip@833

commit: 041ee1f

This test akes sure that jest configs specified in non-root folders
can declare a rootPath option that can be referenced in places
like setupFilesAfterEnv
@tryggvigy
Copy link
Contributor Author

tryggvigy commented Nov 13, 2024

@webpro I've pushed a new commit which includes a unit test that verifies the code changes fix the issue.
I can see the test failing on main and passing on this PR.

Screenshot 2024-11-13 at 13 10 23

@tryggvigy
Copy link
Contributor Author

I think I found two more missig features the jest plugin could handle.

  1. Reporters like this are not caught
  reporters: [
    'default',
    [
      'jest-junit',
      {
        outputDirectory: '<rootDir>',
        outputName: 'junit-e2e.xml',
        testSuitePropertiesFile: 'junitProperties.e2e.cjs',
      },
    ],
   ...
]
  1. testEnvironment like this is not caught
  testEnvironment:
    '<rootDir>/tests/framework/e2e/jest-environments/E2EEnvironment.ts',

I can make separate PRs to try and fix those.

@webpro
Copy link
Collaborator

webpro commented Nov 13, 2024

@webpro I've pushed a new commit which includes a unit test that verifies the code changes fix the issue. I can see the test failing on main and passing on this PR.

Perfect, thank you!

I can make separate PRs to try and fix those.

That would be great. Guess we could use the new fixture/test for this as well. Thanks in advance!

@webpro webpro merged commit 9730d02 into webpro-nl:main Nov 13, 2024
23 checks passed
@webpro
Copy link
Collaborator

webpro commented Nov 14, 2024

🚀 This pull request is included in v5.37.0. See Release 5.37.0 for release notes.

Using Knip in a commercial project? Please consider becoming a sponsor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants