feat(cli): Support specifying a line number when filtering tests

[mzhubail]

Description

closes #5445.

1. Added a function to parse filters into { filename, lineNumber? }. The function attempts to parse each filter with the format <filename>:<line_number>. If succssful, lineNumber is set, and we have a "location filter", otherwise line number is not set and the filter is treated as a regular filter.

2. Added locationFilters to UserConfig and to VitestRunnerConfig.

   Note that the we only store filters that have lineNumber, in order to use them in interpretTaskModes.

3. The parsing is done in cac.ts:start because that's the place where we have access to both the options and filters.

I think I know where to do stuff for the most part, but I need help with some stuff:

• I'm not sure how to report errors in case the user included a range instead of a line number.

• I'm not happy with my use of Required in Required<Filter[]>, but I'm not sure what would be a better way to do it.

• I think we should match filename strictly when location filter is provided, but I'm not sure how to. Should I be using something similar to WorkspaceProject.filterFiles()?

  if (isAbsolute(f) && t.startsWith(f)) {
      return true
  }
  

• ~~As for printing an error when the user provides a wrong line number, it's a bit tough with interpretTaskModes being a recursive a function. What I have in mind is to keep track of all the filters and wether it was matched or not, and print out errors once we've been through all the tests.~~

  ~~The part I'm having an issue with is where to keep track of that. That would involve major changes to how interpretTaskModes is written. I could move the~~

  See 36c88e9574eafcdc6be2aa582056b118d4ebb33c

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• [ ] It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
• [ ] Ideally, include a test that fails without this PR but passes with it.
• [x] Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

• [ ] Run the tests with pnpm test:ci.

Documentation

• [ ] If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

• [ ] Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

[netlify[bot]]

✅ Deploy Preview for vitest-dev ready!

Name	Link
🔨 Latest commit	a6cae3cfdd96f7697064a39a4307f1504958e5eb
🔍 Latest deploy log	https://app.netlify.com/sites/vitest-dev/deploys/674ce477a54a3f0008f0ddfd
😎 Deploy Preview	https://deploy-preview-6411--vitest-dev.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.

[sheremet-va]

Vitest has specs that describe the test behaviour:

https://github.com/vitest-dev/vitest/blob/83638eb9ad58b9addfe74121f7ef2952518aeb96/packages/vitest/src/node/core.ts#L1083

I would prefer if we passed down locations there to keep the scope clean and avoid polluting the config.

[mzhubail]

I would prefer if we passed down locations there to keep the scope clean and avoid polluting the config.

As far as I can tell, globTestSpecs is run before we have access to tests (not collected yet), and I need access to the tests to filter based on line number.

I went with this approach as it is more similar to how -t, --testNamePattern is handled (#332)

[sheremet-va]

As far as I can tell, globTestSpecs is run before we have access to tests (not collected yet), and I need access to the tests to filter based on line number.

We don't need to collect tests for this to work, spec defines what tests to run. This is the perfect place to tell Vitest to limit what test functions to run.

I went with this approach as it is more similar to how -t, --testNamePattern is handled (#332)

I know. This doesn't really explain why it should follow the same implementation. You can keep the filtering in the test like it is, but change how the values are passed down from the main process.

[mzhubail]

I might be missing something. What kind of information do I have access to in a spec? How would I access the line numbers of a test?

I see location in TaskBase. Do I have access to it in globTestSpecs?

[sheremet-va]

I might be missing something. What kind of information do I have access to in a spec? How would I access the line numbers of a test?

I see location in TaskBase. Do I have access to it in globTestSpecs?

Spec is a specification, it doesn't have any information about the test or a file. It tells what file to run and how to run it. TestCase is the result. You need to pass down this information somewhere in the pool:

https://github.com/vitest-dev/vitest/blob/83638eb9ad58b9addfe74121f7ef2952518aeb96/packages/vitest/src/node/pools/threads.ts#L105

Maybe you can populate the provided context somehow or inject the config value, that's up to you, but the processing should happen there.

[mzhubail]

You can keep the filtering in the test like it is, but change how the values are passed down from the main process.

Do you mean that the issue at hand is not filtering inside interpretTaskModes, but rather the use of VitestRunnerConfig and UesrConfig to pass the values to interpretTaskModes?

Maybe you can populate the provided context somehow or inject the config value, that's up to you, but the processing should happen there.

Is the issue that we should provide each worker with only the values that are relevant values to its files (unlike testNamePatter which is global for all files)?

[sheremet-va]

Do you mean that the issue at hand is not filtering inside interpretTaskModes, but rather the use of VitestRunnerConfig and UesrConfig to pass the values to interpretTaskModes?

Yes. The interpretTaskModes code is fine.

Is the issue that we should provide each worker with only the values that are relevant values to its files (unlike testNamePatter which is global for all files)?

Yes, that's the idea.

[mzhubail]

Yeah, that makes sense. I'll take a shot at it.

[mzhubail]

@sheremet-va

I gave it a try. See 76bd9212b4f3abecce3e4cbc5178ec00d2069348. Previous code will be removed of course.

In globTestSpecs, the filters will be matched against the files, and if there is a location in the filter, it will be included with its file, in a FileSpec: { path, testLocations? }.

I thought it would be a good idea to include the files with the location in the same object, instead of having them in their own attribute in the config and then matching based on the path or something. But I ended up having to modify types everywhere to get this to work.

I had to modify groupFilesByEnv to include the testLocations in its output, and also modify pools to pass the test locations to runFiles.

Much of the code in this commit is hacked together, but I had to get something that somewhat works, sometimes, just to see if I'm on the right track.

Would appreciate inputs on this before I commit to making more changes.

[mzhubail]

The general direction seems fine to me, but there are some changes that we cannot make because it would be a breaking change.

One thing I shouldn't be doing is parsing the filters in cac.ts:start, this is forcing me to change types more than I have to.

https://github.com/vitest-dev/vitest/blob/76bd9212b4f3abecce3e4cbc5178ec00d2069348/packages/vitest/src/node/cli/cac.ts#L265

If I were to do the parsing inside globTestSpecs it will solve the type change related to Filter.

How about Instead of using an incompatible type like FileSpec, we use a less restrictive superset type, maybe something like string[] | FileSpec[] or (FileSpec | string)[], would that do for now? Do you have any other suggestions?

Unless you want to have this merged in a year,

This has been labelled a "nice to have", but I'm not sure I'll be here in a year :)

[sheremet-va]

How about Instead of using an incompatible type like FileSpec, we use a less restrictive superset type, maybe something like string[] | FileSpec[] or (FileSpec | string)[], would that do for now? Do you have any other suggestions?

I guess that would work but I don't understand why you need this if you will still parse it inside, right?

[mzhubail]

I guess that would work but I don't understand why you need this if you will still parse it inside, right?

I Forgot to add the declaration of FileSpec, my bad.

export interface FileSpec {
  file: string
  testLocations: number[] | undefined
}

The intention of this is to pass both the filepath and the testLocations togther, instead of just passing the filepath as string instead of specifying the testLocations separately in each worker's config. It seemed to me that passing them together would make more sense.

[mzhubail]

Cleaned up a bit

• I'm not sure if I should update these types to string [] | FileSpec[] too.

  https://github.com/vitest-dev/vitest/blob/8f49e9e544086544f4efa4bccc1256148d560606/packages/runner/src/run.ts#L516

  https://github.com/vitest-dev/vitest/blob/8f49e9e544086544f4efa4bccc1256148d560606/packages/runner/src/run.ts#L498

  https://github.com/vitest-dev/vitest/blob/8f49e9e544086544f4efa4bccc1256148d560606/packages/vitest/src/runtime/runBaseTests.ts#L21

  https://github.com/vitest-dev/vitest/blob/8f49e9e544086544f4efa4bccc1256148d560606/packages/vitest/src/runtime/runVmTests.ts#L25-L26

• In interpretTaskModes, I've renamed the argument to file. It seems to me that only a file gets passed. Please confirm.

  https://github.com/vitest-dev/vitest/blob/8f49e9e544086544f4efa4bccc1256148d560606/packages/runner/src/utils/collect.ts#L9

  and I'm not sure how to handle error reporting in case a wrong line number was provided. Pointers would be appreciated.

• I've tried to minimize the changes in packages/vitest/src/node/pools/forks.ts. I will replicate them for all pool types.

  https://github.com/vitest-dev/vitest/blob/8f49e9e544086544f4efa4bccc1256148d560606/packages/vitest/src/node/pools/forks.ts#L280

  Moved away from using a variable, because I wasn't sure what to name.

---

I hope I'm in the right direction. Thank you for you time @sheremet-va!

[mzhubail]

squashed and rebased. Should be up to date with main, will check it later.

For now, I've added an error IncludeTaskLocationDisabledError, it is ok? I'm thinking of adding a new one for range line number (location filters don't accept ranges).

[mzhubail]

@sheremet-va Should be good, if you can take a look.

Thanks you!

[sheremet-va]

@sheremet-va Should be good, if you can take a look.

Thanks you!

There are issues in CI it seems. Something with benchmarking?

[mzhubail]

There are issues in CI it seems. Something with benchmarking?

Aha, thought it had to do with stuff not related to me, will look into it.

[mzhubail]

For the failing build, seems like it is caused by import of FileSpec type. Seems like I should make this type public, am I right?

[mzhubail]

@sheremet-va should be mostly good.

Build is good. Test is good, but not for windows (or sometimes macos), idk why

Changed code to throw an error when a given filter doesn't match -- covered this in test cases by having math.test.ts and math-with-dashes-in-name.test.ts; specifying math:99 should result in an error.

[sheremet-va]

Test is good, but not for windows (or sometimes macos), idk why

It's probably related to / and \ difference. MacOS is just flaky, but your Windows tests are clearly failing because there is a bug somewhere

[sheremet-va]

Looks good to me! Do you think you can fix the Windows errors?

[mzhubail]

Seems like this can use some docs too.

[sheremet-va]

Thank you for keeping this PR up-to-date!

[mzhubail]

Thank you for keeping this PR up-to-date!

Anytime! Was quite challenging (and fun) working on this, although I got busy with some stuff throughout.

I hope I have the chance to contribute again.