-
Notifications
You must be signed in to change notification settings - Fork 532
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 code-coverage-tools package to compare code coverage on the PR build. #22452
Conversation
build-tools/packages/build-cli/src/commands/report/codeCoverageStats.ts
Outdated
Show resolved
Hide resolved
build-tools/packages/build-cli/src/commands/report/codeCoverageStats.ts
Outdated
Show resolved
Hide resolved
build-tools/packages/build-cli/src/commands/report/codeCoverageStats.ts
Outdated
Show resolved
Hide resolved
build-tools/packages/build-cli/src/commands/report/codeCoverageStats.ts
Outdated
Show resolved
Hide resolved
build-tools/packages/build-cli/src/commands/report/codeCoverageStats.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
…eStats.ts Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
…to coveragecmnd
Nit: it would probably be good to call out in the PR description why we needed to create our own coverage infrastructure, rather than using an off-the-shelf solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor issues overall. Thanks for working through all the feedback!
build-tools/packages/build-cli/src/codeCoverage/getCoverageMetrics.ts
Outdated
Show resolved
Hide resolved
build-tools/packages/build-cli/src/codeCoverage/getCoverageMetrics.ts
Outdated
Show resolved
Hide resolved
logger?.verbose(`Found baseline build with id: ${baselineBuild.id}`); | ||
|
||
return { | ||
build: { ...baselineBuild, id: baselineBuild.id }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a blocker, but I am still a bit confused here. if baselineBuild.id === undefined
, then you throw earlier, so baselineBuild.id must be a defined value by the time you get to this line. So what is this code changing in the data? It seems like it's just setting the id property to itself, which should be a no-op.
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Description
AB#14170
Add code-coverage-tools package to compare code coverage on the PR build.
1.) Before running coverage comparison, code coverage plugin identifies the baseline build for the PR.
2.) Once the baseline build is identified, we download the build artifacts corresponding to the
Code Coverage Report_<Build_Number>
artifact name for this build3.) We then collect the code coverage stats for the PR build and then make the comparison with the baseline.
4.) If the code coverage diff (branch coverage) is more than a percentage point change, then we fail the build for the PR. We also fail the build in case the code coverage for the newly added package is less than 50%.
5.) We post the comment on the PR specifying the code coverage change if any for each package which is modified.
We needed this separate module as we need specifics to do the code coverage comparison like the baseline with which we need to make the comparison and then what we need to compare and then after comparison what comment and in what format we want to report it.
Sample Comment: