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:.

It attempts to parse each filter with the format `<filename>:<line_number>`, while keeping mind that some files might have `:` in their filename. Throw an error when line range is provided. Note that there is an edge case when a file has `:` and `-` in its filename.

netlify · 2024-08-27T11:44:15Z

✅ Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`36c88e9`
🔍 Latest deploy log	https://app.netlify.com/sites/vitest-dev/deploys/66cdbb676c44300008d3c6d5
😎 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.

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 also 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.

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,

sheremet-va · 2024-08-28T11:36:38Z

packages/vitest/src/node/core.ts

@@ -374,7 +377,7 @@ export class Vitest {
    }
  }

-  async start(filters?: string[]) {
+  async start(filters?: Filter[]) {


We cannot change the type of this because it would be a big breaking change

sheremet-va · 2024-08-28T11:36:43Z

packages/vitest/src/node/core.ts

@@ -1080,18 +1083,22 @@ export class Vitest {
    return this.globTestSpecs().then(specs => specs.map(spec => spec.moduleId))
  }

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


We cannot change the type of this because it would be a big breaking change

sheremet-va · 2024-08-28T11:36:53Z

packages/vitest/src/node/logger.ts

@@ -141,7 +141,8 @@ export class Logger {
    return code
  }

-  printNoTestFound(filters?: string[]) {
+  printNoTestFound(filters_?: Filter[]) {


We cannot change the type of this because it would be a big breaking change

sheremet-va · 2024-08-28T11:37:33Z

packages/vitest/src/node/workspace.ts

+  public createSpec(
+    moduleId: string,
+    pool: string,
+    testLocations: number[] | undefined,


Suggested change

testLocations: number[] | undefined,

testLocations?: number[] | undefined,

sheremet-va · 2024-08-28T11:38:11Z

packages/runner/src/collect.ts

@@ -20,14 +20,15 @@ import { runSetupFiles } from './setup'
 const now = Date.now

 export async function collectTests(
-  paths: string[],
+  specs: FileSpec[],


We cannot change the type of this because it would be a big breaking change

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.

- Move the filter parsing inside `globTestSpecs` and revert related type changes - Loosened up the type in `packages/runner/src/collect.ts:collectTests` - Fix type in `createSpec` - Modify `groupFilesByEnv` return structure - Modified `FileSpec`. Name `filepath` is better than just `file` - Functional matching by ___location in `interpretTaskModes`

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!

mzhubail added 7 commits August 27, 2024 12:34

Add filter interface

6f6026b

Parse filters and pass to interpretTaskModes

8fd14e4

Add filtering based on line number

cb8c438

Enable includeTaskLocation if ___location filter is given

f9e5d59

Don't apply ___location filtering if none is relevant

02f9efc

Error on a wrong line number

36c88e9

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

mzhubail added 2 commits September 4, 2024 11:29

Remove Filter type

8f49e9e

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

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

sheremet-va Aug 28, 2024

sheremet-va Aug 28, 2024

sheremet-va Aug 28, 2024

sheremet-va Aug 28, 2024

sheremet-va Aug 28, 2024

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

	testLocations: number[] \| undefined,
	testLocations?: number[] \| undefined,

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

Are you sure you want to change the base?

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

✅ 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

sheremet-va Aug 28, 2024

Choose a reason for hiding this comment

sheremet-va Aug 28, 2024

Choose a reason for hiding this comment

sheremet-va Aug 28, 2024

Choose a reason for hiding this comment

sheremet-va Aug 28, 2024

Choose a reason for hiding this comment

sheremet-va Aug 28, 2024

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 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