Skip to content

Conversation

@codepunkt
Copy link
Contributor

The vitest plugin is not properly handling workspace configurations.

This PR should enable that, and thus detect various things (e.g. globalSetup, setupFiles, reporters or coverage.provider) inside a workspace array - like in this example:

import { defineConfig } from 'vitest/config';

export default defineConfig({
  test: {
    workspace: [
      {
        test: {
          name: 'integration',
          setupFiles: './vitest.integration.setup.mjs',
        },
      },
      {
        test: {
          globalSetup: './vitest.unit.setup.ts',
          name: 'unit',
        },
      },
    ],
  },
});

});

assert(issues.unresolved['vitest.config.ts']['./vitest.integration.setup.mjs']);
assert(issues.unresolved['vitest.config.ts']['./vitest.unit.setup.ts']);
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PR! Happy to merge. If you could please touch those (empty) files there? Then this test file and the counters are tidy too 🍃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure that I understand the counters and issues returned by await main(). When i touch the files, the test fails because they're not unresolved then.

How exactly would touching the files make this test file and counters tidy?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I wasn't clear. With "tidy" I meant that the issue counters end up being zero (0). When the files are there (i.e. not unresolved), there are no issues left (only a number of processed/total files). Then we don't need to assert numbers for unresolved etc. which in my mind is "tidy".


return [...[...environments, ...reporters, ...coverage].map(id => toDependency(id)), ...setupFiles, ...globalSetup];
const workspaceDependencies: Input[] = [];
if (testConfig.workspace !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Apr 3, 2025

Open in StackBlitz

npm i https://pkg.pr.new/knip@1003

commit: 33da58d

@webpro
Copy link
Member

webpro commented Apr 3, 2025

Thanks! Only a few lint issues left it seems.

@codepunkt
Copy link
Contributor Author

I'm not sure what failed. What can I run locally to ensure that everything is alright?

@webpro
Copy link
Member

webpro commented Apr 3, 2025

Here are the commands that can be used for QA and that should go green: https://github.com/webpro-nl/knip/blob/main/.github/DEVELOPMENT.md#qa

There's currently one job in Run Knip against external projects that fails, we can ignore that.

@codepunkt
Copy link
Contributor Author

bun knip:production wasn't found.
bun lint had an error due to unused variables and a small spacing issue, which I've fixed.

bun run test failed on the new testcase that I've added, because I'm specifically testing the non-existance of files, but you asked me to add them. I've sinced removed them, and now everything listed in the QA link you've sent (except knip:production) should pass.

Is there anything else I can help with?

@webpro
Copy link
Member

webpro commented Apr 4, 2025

bun knip:production wasn't found.

Forgot to update that one, should be bun knip --strict now.

Edit: actually it's there, but only in packages/knip

@webpro webpro merged commit 3323602 into webpro-nl:main Apr 4, 2025
23 checks passed
@webpro
Copy link
Member

webpro commented Apr 4, 2025

Thanks Christoph!

@codepunkt
Copy link
Contributor Author

My thanks go to you! 👍🏻

@webpro
Copy link
Member

webpro commented Apr 6, 2025

🚀 This pull request is included in v5.47.0. See Release 5.47.0 for release notes.

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

@fshowalter
Copy link

This doesn’t account for extends so if you define your coverage engine as Istanbul in the root config and set extends: true in the workspace, Knip will think the workspaces have no coverage definition and default to v8, leading Knip to flag it as a missing dep.

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.

3 participants