chore: update all test scripts to remove coverage flag #10582
Conversation
Adds a `test-coverage` script to each package. With this change, each package has these two scripts: - `test` - run tests with coverage disabled - `test-coverage` - run tests with coverage enabled (_if_ the package wants to support coverage) Before this change, `test` behaved like `test-coverage` (i.e. packages had coverage enabled in it). In CI, we then forced coverage off in some situations by passing a flag: `npm run test -- --coverage=false`. This resulted in commands like `jest --coverage --coverage=false`. While jest is happy with this, vitest is not. Given we are likely to move to vitest, it is worth fixing this ahead of time.
|
Thanks for the PR, @43081j! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
View your CI Pipeline Execution ↗ for commit 479af6d.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10582 +/- ##
==========================================
+ Coverage 87.27% 87.43% +0.15%
==========================================
Files 453 468 +15
Lines 15833 16025 +192
Branches 4639 4639
==========================================
+ Hits 13819 14011 +192
Misses 1658 1658
Partials 356 356
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Hey @43081j , we do need to ask that you follow the template and contributing guide For something like this please file an issue so we can discuss |
Do feel free to link a draft PR on the issue, though, if it's a helpful aid to the discussion |
|
No worries As mentioned, this came out of a related PR so didn't have it's own issue. Happy to make one, the guidelines certainly were read, I just didn't realise this needed it's own issue Will keep the branch around since it is likely we will need it |
|
reopening following discussion in #10579 and #7112 (comment) |
|
in the various PRs by @aryaemami59, an environment variable has been used instead of having two scripts. that may make more sense, if we don't care about being able to locally run with coverage (and instead we'd manually pass if we do that, i'm happy to update this PR to make use of the either way, we remove the probably a decision for you @kirkwaiblinger and @JoshuaKGoldberg if you decide an env var is the way to go, we have #10689 |
|
@43081j Honestly I think a |
|
@kirkwaiblinger @JoshuaKGoldberg any chance we can get some movement on this? it will unblock the rest of the vitest work. there are still pieces of the larger PRs that can be cut out into individual changes, but this needs to land first |
|
For visibility, I'm stuck doing a few other projects this week - but if nobody's moved it forward by early next week I can try to find some time. Thanks for pushing on it! ❤️ |
|
Hey! I also am not able to dive into this area at the moment, several things going on in my personal life and such. I expect it will be at least until the weekend but potentially next week. We definitely appreciate your work on this and do plan to unblock you as soon as we can. Thanks for understanding! 🙏 |
|
@kirkwaiblinger i think disabling the flag sorted what you were seeing can you double check? |
There was a problem hiding this comment.
I've gone ahead and pushed the tweaks requested in #10582 (review) to your branch, so I think we're good to go. @43081j let us know if there's any concerns!
|
Looks good to me. If there are any other changes, please do just push them so we can get this merged and reduce the iteration time Will be good to get this merged 👍 |
|
@43081j Excited to be merging! Thanks for your work on this! |
Changes
testof all packages to run without coverage, then alters the following:--coveragewhere needed--coverage(e.g.npm test -- --coverage) to enable coveragedevDependencyBefore this change,
testbehaved liketest --coverage(i.e. packages had coverage enabled in it). In CI, we then forced coverage off in some situations by passing a flag:npm run test -- --coverage=false.This resulted in commands like
jest --coverage --coverage=false.While jest is happy with this, vitest is not. Given we are likely to move to vitest, it is worth fixing this ahead of time.
PR Checklist