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

Improve support for parallel runs and coverage report merging when --shard option is used #5125

Closed
4 tasks done
mirayashi opened this issue Feb 6, 2024 · 4 comments · Fixed by #5736
Closed
4 tasks done
Labels
enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority)

Comments

@mirayashi
Copy link

Clear and concise description of the problem

I'm making this suggestion because currently my setup is the following:

  • I'm using v8 coverage on a very large React project
  • To speed up tests, we chose to use following poolOptions:
    "poolOptions": {
      "forks": {
        "isolate": false,
        "singleFork": true
      }
    }
  • To speed up tests even more, we chose to run Vitest with the --shard option and run 4 instances of them in parallel using npm-run-all (one for each shard)
  • This required us to use the following additional config:
    cache: false,
    coverage: {
      clean: false,
      reportsDirectory: './.nyc_output',
      reporter: [['json', { file: `${uuid.v4()}.json` }]]
    }

This setup worked fine up to version v1.2.1. But in v1.2.2, the change introduced in #5008 breaks it as we get errors like Error: EONENT: No such file or directory, open '/.../.nyc_output/.tmp/coverage-184.json'

Suggested solution

Solution 1: Allow to customize the name of the .tmp directory so each instance can read/write on a different one

Solution 2: Make the auto-cleaning of .tmp optional

Alternative

No response

Additional context

No response

Validations

@AriPerkkio
Copy link
Member

The --shard option doesn't have good support for generated test and coverage reports. I think Vitest should instead detect --shard option, apply the value in all report names, and finally provide a command to merge these reports. Users shouldn't need to worry about conflicting filenames etc.

What you could do now is use similar unique name in coverage.reportDirectory as you have in JSON reporter's file. I guess you are already somehow merging these reports together in another script?

@mirayashi
Copy link
Author

The --shard option doesn't have good support for generated test and coverage reports. I think Vitest should instead detect --shard option, apply the value in all report names, and finally provide a command to merge these reports. Users shouldn't need to worry about conflicting filenames etc.

What you could do now is use similar unique name in coverage.reportDirectory as you have in JSON reporter's file. I guess you are already somehow merging these reports together in another script?

We are indeed using nyc report to merge the reports. While we can customize the name of the directory in that command, it still expects that all .json reports are in the same directory. I guess it could work if we add another script to copy files to the same directory before merging reports, but I agree that Vitest should be providing native support for that, hence I'll keep this issue open for now.

@mirayashi mirayashi changed the title Add option to customize behavior of temp files for v8 coverage Improve support for parallel runs and coverage report merging when --shard option is used Feb 7, 2024
@AriPerkkio AriPerkkio added the enhancement New feature or request label Feb 8, 2024
@AriPerkkio
Copy link
Member

As quick fix for this I think we could append the .tmp directory's name with value of --shard option, e.g. --shard=1/3 would use .tmp-13 or similar. The name of .tmp directory is not part of public API so that can be changed easily.

In longer term we'll need to think how to properly support all test and coverage reporters to work nicely with --shard. It might require breaking changes by changing the filenames of generated test and coverage reports. If someone is already using --shard and relying on identical filenames, this would break their existing merge scripts.

It also might be worth to check how Playwright generates test reports and coverage reports with --shard option.

@sheremet-va sheremet-va moved this to P2 - 4 in Team Board Feb 9, 2024
@sheremet-va sheremet-va moved this from P2 - 4 to P2 - 3 in Team Board Feb 9, 2024
@sheremet-va sheremet-va added the p2-to-be-discussed Enhancement under consideration (priority) label Feb 9, 2024
@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 12, 2024

It looks like playwright does shard-based file naming for their blob reporter automatically https://playwright.dev/docs/test-reporters#blob-reporter since it's intended be used with playwright merge-reports https://playwright.dev/docs/test-sharding#merging-reports-from-multiple-shards

For Vitest, I'm not sure how much we need to proactively help avoiding filename conflict since we don't have such merging functionality out-of-the-box yet.

Btw, I don't think playwright has coverage feature builtin microsoft/playwright#7030, microsoft/playwright#9208

@sheremet-va sheremet-va added p2-nice-to-have Not breaking anything but nice to have (priority) and removed p2-to-be-discussed Enhancement under consideration (priority) labels Feb 12, 2024
@sheremet-va sheremet-va moved this from P2 - 3 to Has plan in Team Board Apr 25, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants