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

Add --coverage option to build scripts for tests #54499

Merged
merged 5 commits into from
Jun 2, 2023
Merged

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Jun 2, 2023

This adds a --coverage option to our build scripts for use with runtests, runtests-parallel, and runtests-watch. When --coverage is specified, tests will be run through c8 and dump an lcov format file to coverage/lcov.info for use with a code coverage extension for VS Code or other tooling.

@rbuckton rbuckton requested review from weswigham and jakebailey June 2, 2023 15:19
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 2, 2023
@rbuckton
Copy link
Member Author

rbuckton commented Jun 2, 2023

c8 isn't strictly necessary. We could do much the same thing by just specifying the NODE_V8_COVERAGE environment variable, but it's helpful to have it produce a formatted coverage file rather than requiring a second step.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been wanting this officially for a while, thanks!

(c8 hereby runtests-parallel was an ok thing up until now)

.c8rc.json Outdated Show resolved Hide resolved
// enable code coverage using 'c8'
let execPath = process.execPath;
if (coverage) {
args.push("exec", "c8", "--clean", execPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we dev dep on c8 and run it directly instead of using npm exec?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easier to continue using npm exec to avoid having to deal with trying to spawn a local package bin and deal with multiple platforms myself, though I can add --prefer-offline or --offline to have it prefer the local version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, I am both continuing to use npm exec for convenience and also adding c8 to devDependencies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easier to continue using npm exec to avoid having to deal with trying to spawn a local package bin and deal with multiple platforms myself, though I can add --prefer-offline or --offline to have it prefer the local version.

What do you mean by platforms? It should be at node_modules/c8/bin/c8.js; we execute eslint and mocha using similar methods.

rbuckton added a commit that referenced this pull request Jun 2, 2023
@rbuckton rbuckton merged commit 913e556 into main Jun 2, 2023
@rbuckton rbuckton deleted the coverage-option branch June 2, 2023 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants