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

mzhubail · 2024-08-27T11:41:24Z

Description

closes #5445.

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.
Added locationFilters to UserConfig and to VitestRunnerConfig.

Note that the we only store filters that have lineNumber, in order to use them in interpretTaskModes.
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 36c88e9

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.
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 · 2024-08-27T11:44:15Z

✅ Deploy Preview for vitest-dev ready!

Name	Link
🔨 Latest commit	`a6cae3c`
🔍 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 · 2024-08-27T11:47:17Z

Vitest has specs that describe the test behaviour:

vitest/packages/vitest/src/node/core.ts

Line 1083 in 83638eb

public async globTestSpecs(filters: string[] = []) {

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

mzhubail · 2024-08-27T12:01:57Z

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 · 2024-08-27T12:05:30Z

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 · 2024-08-27T12:16:59Z

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 · 2024-08-27T12:31:47Z

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:

vitest/packages/vitest/src/node/pools/threads.ts

Line 105 in 83638eb

const data: WorkerContext = {

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 · 2024-08-27T13:05:09Z

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 · 2024-08-27T13:07:18Z

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 · 2024-08-27T13:24:36Z

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

mzhubail · 2024-08-28T11:33:39Z

@sheremet-va

I gave it a try. See 76bd921. 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.

sheremet-va

The general direction seems fine to me, but there are some changes that we cannot make because it would be a breaking change. Unless you want to have this merged in a year,

packages/vitest/src/node/core.ts

packages/vitest/src/node/logger.ts

packages/vitest/src/node/workspace.ts

packages/runner/src/collect.ts

mzhubail · 2024-08-28T12:00:59Z

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.

vitest/packages/vitest/src/node/cli/cac.ts

Line 265 in 76bd921

const filters = cliFilters.map(normalize).map(parseFilter)

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 · 2024-08-29T15:40:42Z

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 · 2024-09-01T06:27:11Z

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 · 2024-09-04T08:46:18Z

Cleaned up a bit

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

vitest/packages/runner/src/run.ts

Line 516 in 8f49e9e

async function publicCollect(specs: FileSpec[], runner: VitestRunner): Promise<File[]> {

vitest/packages/runner/src/run.ts

Line 498 in 8f49e9e

export async function startTests(specs: FileSpec[], runner: VitestRunner): Promise<File[]> {

vitest/packages/vitest/src/runtime/runBaseTests.ts

Line 21 in 8f49e9e

files: FileSpec[],

vitest/packages/vitest/src/runtime/runVmTests.ts

Lines 25 to 26 in 8f49e9e

// TODO consider type

files: FileSpec[],
In interpretTaskModes, I've renamed the argument to file. It seems to me that only a file gets passed. Please confirm.

vitest/packages/runner/src/utils/collect.ts

Line 9 in 8f49e9e

file: Suite,

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.

vitest/packages/vitest/src/node/pools/forks.ts

Line 280 in 8f49e9e

files.map(f => f.file),

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!

packages/runner/src/utils/collect.ts

packages/vitest/src/node/cli/cli-api.ts

packages/runner/src/run.ts

packages/vitest/src/node/core.ts

packages/vitest/src/types/worker.ts

mzhubail · 2024-10-24T10:44:27Z

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 · 2024-10-30T12:12:27Z

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

Thanks you!

sheremet-va · 2024-10-30T12:55:41Z

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

Thanks you!

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

mzhubail · 2024-10-30T13:09:51Z

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.

sheremet-va

Looks good! We need more tests, all the tests now are using list command

packages/vitest/src/node/cli/cac.ts

packages/runner/src/utils/collect.ts

packages/vitest/src/node/cli/cli-api.ts

packages/vitest/src/node/errors.ts

sheremet-va · 2024-10-30T13:19:29Z

packages/vitest/src/node/cli/cli-api.ts

@@ -282,6 +331,53 @@ export function formatCollectedAsString(files: File[]) {
  }).flat()
 }

+export function parseFilter(filter: string): Filter {


It doesn't seem like these two functions belong here because they are not used for the CLI specifically. Maybe create a separate file for them?

What do you think of vitest/src/node/filter.ts?

sheremet-va · 2024-10-30T13:21:13Z

packages/vitest/src/node/core.ts

+    const dir = this.config.dir || this.config.root
+    const parsedFilters = filters.map(f => parseFilter(f))
+    const testLocations = groupFilters(parsedFilters.map(
+      f => ({ ...f, filename: resolve(dir, f.filename) }),


I wonder if it makes sense to validate that the path exists. Filters can be anything, you can provide a part of the file

I think we should throw an error if:

There is a location in filters

File is resolved with process.cwd(), file, not root (because filters are provides via a CLI normally)

File doesn't exist, or it's not a file at all (a directory for example)

Interesting.

What came to mind is to limit matching to full filename, instead of part of it, in order not to match multiple files at the same time. Does that sound right to you?

Note that I'm passing the filters as is (before resolving) to project.globTestFiles, and then I narrow the matching for filters with location specified. Later at line 1143, I'm skipping over reporting errors for filters that don't have location specified.

For now I'm just reporting error and continuing. Now that I'm thinking about it, this has weird behavior for example pnpm exec vitest basic.test.ts:3 would report an error about "not finding the file", but will run the tests anyways.

It seems I might be able to fix this by only resolving filters with test location and then passing on to project.globTestFiles, does that sound right to you?

Good point on resolving with process.cwd().

I don't know if I follow what you want to do instead of what I proposed. Maybe you can build a prototype?

packages/vitest/src/node/spec.ts

packages/vitest/src/runtime/runVmTests.ts

packages/vitest/src/typecheck/collect.ts

mzhubail · 2024-11-10T13:42:00Z

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?

sheremet-va · 2024-11-12T16:07:52Z

packages/vitest/src/node/pools/forks.ts

@@ -1,3 +1,4 @@
+import type { FileSpec } from '@vitest/runner/types/runner'


@vitest/runner/types/runner doesn't exist when built. you need to expose it in @vitest/runner

- Expand on error description - Add private marker - Remove TODO - Remove obsolete snapshot - Add `filter.ts` - Other changes

I don't know how this got over here.

- rebase issues

In `globTestSpecs`, test locations are matched with their test file by full path (`globTestFiles` returns full path). I think we should keep it this way because:(A) matching multiple files doesn't make much sense -- for example, `fileA.test.ts` might have test at line 21, but `fileB.test.ts` won't; and (B) this feature will be used in command line mostly, where entering full path is easy with autocomplete. Issue arises when location filter (let's say `file`) doesn't match a file by full path. Previously, I thought about dropping files `fileA.test.ts` and `fileB.test.ts`, and running the rest of the files but implementing this doesn't seem straightforward to me. For now, I'll be throwing error.

- Previous implementation failed on windows (drive letter detected as range location `D:/...`) - Add related test cases too

- Modify test case to use a file that doesn't exist instead

docs/guide/filtering.md

sheremet-va · 2024-12-01T23:18:09Z

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

mzhubail · 2024-12-02T13:12:26Z

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.

mzhubail marked this pull request as draft August 28, 2024 11:31

sheremet-va requested changes Aug 28, 2024

View reviewed changes

mzhubail requested a review from sheremet-va August 29, 2024 06:17

sheremet-va requested changes Oct 14, 2024

View reviewed changes

mzhubail force-pushed the feature/cli-support-line-number-filters branch from a4c4a04 to 83e103e Compare October 24, 2024 10:40

mzhubail force-pushed the feature/cli-support-line-number-filters branch 2 times, most recently from beba5a5 to c7831bf Compare October 30, 2024 11:07

sheremet-va requested changes Oct 30, 2024

View reviewed changes

mzhubail force-pushed the feature/cli-support-line-number-filters branch from e505eeb to 7a53882 Compare October 31, 2024 07:27

mzhubail force-pushed the feature/cli-support-line-number-filters branch from 7a53882 to 2d727fd Compare November 10, 2024 13:18

sheremet-va reviewed Nov 12, 2024

View reviewed changes

mzhubail added 21 commits November 28, 2024 08:52

Fix nasty recursion bug

8c0e1f7

Fixes

e126427

- Expand on error description - Add private marker - Remove TODO - Remove obsolete snapshot - Add `filter.ts` - Other changes

something

2319d98

Add type export

3d71d0d

Comment out test case

fa0b568

Remove temp file

2981c1b

I don't know how this got over here.

Remove workspace.ts

65eda19

- rebase issues

Change FileSpec imports

cfe6504

Remove unnecessary changes

f80b6ca

Modify testing setup

f2cd068

Add run command tests

52d5e3b

Add a couple of test cases

68debe6

Remove duplicate tests from list.test.ts

8ead31c

Remove more unused stuff

1607250

One more test

0952978

Resolve against process.cwd()

21fd0a9

Fix windows

e3487ad

Make range line detection stricter

237f718

- Previous implementation failed on windows (drive letter detected as range location `D:/...`) - Add related test cases too

Remove file with a colon in name

dbce189

- Modify test case to use a file that doesn't exist instead

Add doc

beed808

mzhubail force-pushed the feature/cli-support-line-number-filters branch from 8a266d4 to beed808 Compare November 28, 2024 05:52

sheremet-va previously approved these changes Dec 1, 2024

View reviewed changes

docs/guide/filtering.md Outdated Show resolved Hide resolved

docs/guide/filtering.md Outdated Show resolved Hide resolved

docs: add vitest 2.2 mention

a6cae3c

sheremet-va dismissed their stale review via a6cae3c December 1, 2024 22:34

sheremet-va merged commit 4d94b95 into vitest-dev:main Dec 1, 2024
6 checks passed

sheremet-va mentioned this pull request Dec 6, 2024

Support specifying a line number when filtering tests #5445

Closed

4 tasks

mzhubail mentioned this pull request Dec 8, 2024

feat(cli): Support location filters for suites #7048

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mzhubail commented Aug 27, 2024

netlify bot commented Aug 27, 2024 •

edited

Loading

sheremet-va commented Aug 27, 2024

mzhubail commented Aug 27, 2024 •

edited

Loading

sheremet-va commented Aug 27, 2024 •

edited

Loading

mzhubail commented Aug 27, 2024

sheremet-va commented Aug 27, 2024

mzhubail commented Aug 27, 2024

sheremet-va commented Aug 27, 2024

mzhubail commented Aug 27, 2024

mzhubail commented Aug 28, 2024

sheremet-va left a comment

mzhubail commented Aug 28, 2024 •

edited

Loading

sheremet-va commented Aug 29, 2024

mzhubail commented Sep 1, 2024 •

edited

Loading

mzhubail commented Sep 4, 2024

mzhubail commented Oct 24, 2024

mzhubail commented Oct 30, 2024

sheremet-va commented Oct 30, 2024

mzhubail commented Oct 30, 2024

sheremet-va left a comment

sheremet-va Oct 30, 2024

mzhubail Nov 10, 2024

sheremet-va Oct 30, 2024

mzhubail Oct 31, 2024

sheremet-va Oct 31, 2024 •

edited

Loading

mzhubail commented Nov 10, 2024

sheremet-va Nov 12, 2024

sheremet-va commented Dec 1, 2024

mzhubail commented Dec 2, 2024

		@@ -1,3 +1,4 @@
		import type { FileSpec } from '@vitest/runner/types/runner'

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

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

Conversation

mzhubail commented Aug 27, 2024

Description

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

Tests

Documentation

Changesets

netlify bot commented Aug 27, 2024 • edited Loading

✅ Deploy Preview for vitest-dev ready!

sheremet-va commented Aug 27, 2024

mzhubail commented Aug 27, 2024 • edited Loading

sheremet-va commented Aug 27, 2024 • edited Loading

mzhubail commented Aug 27, 2024

sheremet-va commented Aug 27, 2024

mzhubail commented Aug 27, 2024

sheremet-va commented Aug 27, 2024

mzhubail commented Aug 27, 2024

mzhubail commented Aug 28, 2024

sheremet-va left a comment

Choose a reason for hiding this comment

mzhubail commented Aug 28, 2024 • edited Loading

sheremet-va commented Aug 29, 2024

mzhubail commented Sep 1, 2024 • edited Loading

mzhubail commented Sep 4, 2024

mzhubail commented Oct 24, 2024

mzhubail commented Oct 30, 2024

sheremet-va commented Oct 30, 2024

mzhubail commented Oct 30, 2024

sheremet-va left a comment

Choose a reason for hiding this comment

sheremet-va Oct 30, 2024

Choose a reason for hiding this comment

mzhubail Nov 10, 2024

Choose a reason for hiding this comment

sheremet-va Oct 30, 2024

Choose a reason for hiding this comment

mzhubail Oct 31, 2024

Choose a reason for hiding this comment

sheremet-va Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

mzhubail commented Nov 10, 2024

sheremet-va Nov 12, 2024

Choose a reason for hiding this comment

sheremet-va commented Dec 1, 2024

mzhubail commented Dec 2, 2024

netlify bot commented Aug 27, 2024 •

edited

Loading

mzhubail commented Aug 27, 2024 •

edited

Loading

sheremet-va commented Aug 27, 2024 •

edited

Loading

mzhubail commented Aug 28, 2024 •

edited

Loading

mzhubail commented Sep 1, 2024 •

edited

Loading

sheremet-va Oct 31, 2024 •

edited

Loading