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

feat: jest coverage output #71

Merged
merged 1 commit into from
Jan 3, 2023
Merged

Conversation

kormide
Copy link
Member

@kormide kormide commented Dec 2, 2022

#34

First time doing anything coverage-related in Bazel, so I may be doing something weird.

@@ -122,6 +123,15 @@ if (updateSnapshots) {
config.snapshotResolver = bazelSnapshotResolverPath;
}

if (coverageEnabled) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be enabled or disabled via a rule attribute?

Copy link
Member

Choose a reason for hiding this comment

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

Does bazel not provide a way to do that already and COVERAGE_OUTPUT_FILE reflects that? Something like a tag to disable it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've learned that it's enabled by running bazel coverge and ensuring this target is instrumented in the --instrumentation_filter. Can be checked with ctx.coverage_instrumented().

Comment on lines 127 to 163
config.collectCoverage = true;
config.coverageDirectory = path.dirname(process.env.COVERAGE_OUTPUT_FILE);
config.coverageReporters = [
"text",
["lcovonly", { file: path.basename(process.env.COVERAGE_OUTPUT_FILE) }],
];
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Should I take what they already have in their jest config into account, or always overwrite?

Copy link
Member

Choose a reason for hiding this comment

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

Could probably overwrite to start, wait to see if anyone would want to customize it more?

@gregmagolan
Copy link
Member

@thesayyn knows this stuff well and could give a good review here

set -o errexit -o nounset -o pipefail

# Case 6: generate a coverage report
bazel coverage //jest/tests:case6 --instrument_test_targets
Copy link
Member Author

Choose a reason for hiding this comment

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

--instrument_test_targets doesn't seem to have any effect on the COVERAGE_OUTPUT_FILE var existing. I'm not not too sure what purpose it should serve here or if it's even needed.

Copy link
Member

Choose a reason for hiding this comment

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

this flag only affects process.env.COVERAGE_MANIFEST. It'll only add more items to the coverage manifest.

@thesayyn
Copy link
Member

thesayyn commented Dec 2, 2022

A few missing pieces;

_lcov_merger attribute is missing; example: https://github.com/aspect-build/rules_js/blob/4b41a61fb9c96e60a44eb32973d70fd4eda870bc/js/private/js_binary.bzl#L512-L518

coverage_common.instrumented_files_info is missing; example: https://github.com/aspect-build/rules_js/blob/4b41a61fb9c96e60a44eb32973d70fd4eda870bc/js/private/js_binary.bzl#L448-L463

I am not sure which files jest decides to generate the coverage report for but that should be limited to what described in COVERAGE_MANIFEST env. you might wanna parse the coverage manifest file and pass it down to collectCoverageFrom option of jest. see: https://github.com/aspect-build/rules_js/blob/4b41a61fb9c96e60a44eb32973d70fd4eda870bc/js/private/coverage/bundle/c8.js#L17-L21 and https://github.com/aspect-build/rules_js/blob/4b41a61fb9c96e60a44eb32973d70fd4eda870bc/js/private/coverage/bundle/c8.js#L29-L30

@gregmagolan gregmagolan marked this pull request as draft December 16, 2022 17:50
@gregmagolan
Copy link
Member

Switched to DRAFT while Derek on vacation. This will get finished up in January.

@kormide kormide force-pushed the jest-coverage branch 3 times, most recently from 12ac706 to 0ac4b49 Compare December 26, 2022 18:25
@kormide
Copy link
Member Author

kormide commented Dec 26, 2022

Thanks for the feedback @thesayyn. This is ready for review again.

@kormide kormide marked this pull request as ready for review December 26, 2022 18:29
jest/private/jest_test.bzl Outdated Show resolved Hide resolved
jest/private/noop.sh Outdated Show resolved Hide resolved
Copy link
Member

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

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

🦖

@kormide kormide merged commit bf74849 into aspect-build:main Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants