Skip to content

Commit

Permalink
Remove line rate comparison for code coverage as that is not much use…
Browse files Browse the repository at this point in the history
…ful due to comments (#23389)

## Description


[AB#23400](https://dev.azure.com/fluidframework/internal/_workitems/edit/23400)
Based on the feedback and analysis we are removing line rate comparison
for code coverage as that is not much useful due to comments increasing
and decreasing it based on whether the comments was in covered code or
non-covered code.

---------

Co-authored-by: Jatin Garg <jatingarg@Jatins-MacBook-Pro-2.local>
  • Loading branch information
jatgarg and Jatin Garg authored Dec 23, 2024
1 parent 6e42677 commit f6bf3a8
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ const codeCoverageComparisonIgnoreList: string[] = [
];

/**
* Type for the code coverage report generated by comparing the baseline and pr code coverage
* Type for the code coverage report generated by comparing the baseline and pr code coverage. We are noting both line and branch coverage
* here but as part of the code coverage comparison check, we are only using branch coverage.
*/
export interface CodeCoverageComparison {
/**
Expand Down Expand Up @@ -159,8 +160,7 @@ export function getPackagesWithCodeCoverageChanges(

// Find existing packages that have reported a change in coverage for the current PR
const existingPackagesWithCoverageChange = codeCoverageComparisonData.filter(
(codeCoverageReport) =>
codeCoverageReport.branchCoverageDiff !== 0 || codeCoverageReport.lineCoverageDiff !== 0,
(codeCoverageReport) => codeCoverageReport.branchCoverageDiff !== 0,
);
logger?.verbose(
`Found ${existingPackagesWithCoverageChange.length} packages with code coverage changes`,
Expand All @@ -184,8 +184,7 @@ export function isCodeCoverageCriteriaPassed(
const { codeCoverageComparisonForNewPackages, codeCoverageComparisonForExistingPackages } =
codeCoverageChangeForPackages;
const packagesWithNotableRegressions = codeCoverageComparisonForExistingPackages.filter(
(codeCoverageReport: CodeCoverageComparison) =>
codeCoverageReport.branchCoverageDiff < -1 || codeCoverageReport.lineCoverageDiff < -1,
(codeCoverageReport: CodeCoverageComparison) => codeCoverageReport.branchCoverageDiff < -1,
);

logger?.verbose(
Expand All @@ -194,8 +193,7 @@ export function isCodeCoverageCriteriaPassed(

// Code coverage for the newly added package should be less than 50% to fail.
const newPackagesWithNotableRegressions = codeCoverageComparisonForNewPackages.filter(
(codeCoverageReport) =>
codeCoverageReport.branchCoverageInPr < 50 || codeCoverageReport.lineCoverageInPr < 50,
(codeCoverageReport) => codeCoverageReport.branchCoverageInPr < 50,
);

logger?.verbose(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,8 @@ const getCodeCoverageSummary = (
const summary = codeCoverageComparisonReport
.sort(
(report1, report2) =>
// Sort the diff summary of packages based on the total coverage diff(line coverage + branch coverage)
report1.branchCoverageDiff +
report1.lineCoverageDiff -
(report2.branchCoverageDiff + report2.lineCoverageDiff),
// Sort the diff summary of packages based on the total coverage diff(branch coverage)
report1.branchCoverageDiff - report2.branchCoverageDiff,
)
.map((coverageReport) => getCodeCoverageSummaryForPackages(coverageReport))
.reduce((prev, current) => prev + current);
Expand All @@ -87,9 +85,9 @@ const getCodeCoverageSummary = (
const getCodeCoverageSummaryForPackages = (coverageReport: CodeCoverageComparison): string => {
const metrics = codeCoverageDetailsHeader + getMetricRows(coverageReport);

return `<details><summary><b>${getGlyphForHtml(coverageReport.branchCoverageDiff + coverageReport.lineCoverageDiff)} ${
return `<details><summary><b>${getGlyphForHtml(coverageReport.branchCoverageDiff)} ${
coverageReport.packagePath
}:</b> <br> Line Coverage Change: ${formatDiff(coverageReport.lineCoverageDiff)}&nbsp;&nbsp;&nbsp;&nbsp;Branch Coverage Change: ${formatDiff(
}:</b> <br> &nbsp;Branch Coverage Change: ${formatDiff(
coverageReport.branchCoverageDiff,
)}</summary>${metrics}</table></details>`;
};
Expand All @@ -114,23 +112,14 @@ const formatDiff = (coverageDiff: number): string => {
};

const getMetricRows = (codeCoverageComparisonReport: CodeCoverageComparison): string => {
const glyphForLineCoverage = getGlyphForHtml(codeCoverageComparisonReport.lineCoverageDiff);
const glyphForBranchCoverage = getGlyphForHtml(
codeCoverageComparisonReport.branchCoverageDiff,
);

return (
`<tr>
return `<tr>
<td>Branch Coverage</td>
<td>${codeCoverageComparisonReport.branchCoverageInBaseline.toFixed(2)}%</td>
<td>${codeCoverageComparisonReport.branchCoverageInPr.toFixed(2)}%</td>
<td>${glyphForBranchCoverage} ${formatDiff(codeCoverageComparisonReport.branchCoverageDiff)}</td>
</tr>` +
`<tr>
<td>Line Coverage</td>
<td>${codeCoverageComparisonReport.lineCoverageInBaseline.toFixed(2)}%</td>
<td>${codeCoverageComparisonReport.lineCoverageInPr.toFixed(2)}%</td>
<td>${glyphForLineCoverage} ${formatDiff(codeCoverageComparisonReport.lineCoverageDiff)}</td>
</tr>`
);
</tr>`;
};
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export default class ReportCodeCoverageCommand extends BaseCommand<

const summaryFooterOnFailure =
"### What to do if the code coverage check fails:\n" +
"- Ideally, add more tests to increase the code coverage for the package(s) whose code-coverage regressed.\n" +
"- Ideally, add more tests to increase the code coverage for the package(s) whose code-coverage regressed. Currently, we are only checking branch coverage for the code coverage check.\n" +
"- If a regression is causing the build to fail and is due to removal of tests, removal of code with lots of tests or any other valid reason, there is a checkbox further up in this comment that determines if the code coverage check should fail the build or not. You can check the box and trigger the build again. The test coverage analysis will still be done, but it will not fail the build if a regression is detected.\n" +
"- Unchecking the checkbox and triggering another build should go back to failing the build if a test-coverage regression is detected.\n\n" +
"- You can check which lines are covered or not covered by your tests with these steps:\n" +
Expand Down

0 comments on commit f6bf3a8

Please sign in to comment.