8000 Repo: add a test-coverage script to simplify test flags · Issue #10607 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

Repo: add a test-coverage script to simplify test flags #10607

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.

Sign up for GitHub

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

Jump to bottom
Open
43081j opened this issue Jan 4, 2025 · 8 comments
Open

Repo: add a test-coverage script to simplify test flags #10607

43081j opened this issue Jan 4, 2025 · 8 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue repo maintenance things to do with maintenance of the repo, and not with code/docs

Comments

@43081j
Copy link
Contributor
43081j commented Jan 4, 2025

Suggestion

This is explained already in #10582 but the summary is as follows:

We currently enable coverage in our test scripts and pass coverage=false when we want to disable it

This results in jest --coverage --coverage=false once everything is expanded

The duplicate flag works in jest but doesn't seem right. More importantly, it does not work with vitest (which we may soon want to move to).

We can solve this by having two scripts in each package:

  • test with coverage disabled
  • test-coverage with coverage enabled (if the package supports coverage)

CI can then call test-coverage instead of manually passing flag overrides

Additional Info

No response
@43081j 43081j added repo maintenance things to do with maintenance of the repo, and not with code/docs triage Waiting for team members to take a look labels Jan 4, 2025
@43081j 43081j mentioned this issue Jan 4, 2025
Merged
3 tasks
@kirkwaiblinger
Copy link
Member

At that point wouldn't we be just as well off keeping one test script without coverage and passing coverage=true when needed?

@kirkwaiblinger
Copy link
Member

Also - this assumes that we'd simply replace jest with vitest in the test scripts (in #7112), but that's not necessarily true, since we might need to create a separate task if they live side by side for a while, no? I guess I ask because the way I see it

so, personally, my instinct is to let it be for now, until we know for certain what are needs are regarding #7112 (at which point we can certainly do this!). I'm not the expert on our repo tooling though so maybe another @typescript-eslint/triage-team member has an opinion?

@43081j
Copy link
Contributor Author
43081j commented Jan 7, 2025 •
edited
Loading

At that point wouldn't we be just as well off keeping one test script without coverage and passing coverage=true when needed?

The whole point before this change was so packages could opt in to coverage if they want to. Rather than ci enforcing it across all of them

So where would you do that if you're passing a flag?

It has to go in each individual script unless you're prepared to turn it on for all of them, or have two matrices in the workflow

Whether you run one test runner or two doesn't really matter, you still need a way to opt in to coverage per package (re your other point)

The script is passing a duplicate flag right now and gets away with it by chance because jest reads the last one

I disagree that we should leave it be because of this. It's prep for moving to vitest and will inevitably have to happen if we do such a move (whichever approach we go with, it isn't what's there now). Nobody wants a monolithic "move to vitest" pr either, so here we are with a standalone change

@kirkwaiblinger kirkwaiblinger added the accepting prs Go ahead, send a pull request that resolves this issue label Jan 20, 2025
@kirkwaiblinger
Copy link
Member

Sounds good!

I'd also invite @aryaemami59 to chime in here and on the PR (#10582) since it seems they've given this thought as well (coming from #7112)

@aryaemami59
Copy link
Co 8000 ntributor

@kirkwaiblinger Honestly I think a test-coverage script would be better and will probably cause less issues as it's more explicit and can be used more reliably locally.

@43081j
Copy link
Contributor Author
43081j commented Jan 21, 2025

FYI we have both PRs here so just need to choose one:

#10582
#10689

for local development, i agree that running a script is nicer than setting COLLECT_COVERAGE=true

so i'd still lean into that one

@aryaemami59
Copy link
Contributor

@43081j I vote #10582.

@JoshuaKGoldberg JoshuaKGoldberg removed the triage Waiting for team members to take a look label Jan 28, 2025
@JoshuaKGoldberg
Copy link
Member

Agreed, let's go with scripts. Env vars tend to get convoluted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue repo maintenance things to do with maintenance of the repo, and not with code/docs
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@JoshuaKGoldberg @43081j @kirkwaiblinger @aryaemami59
0