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.

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

fix: wrong output truecount and falsecount of clover #699

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: wrong output truecount and falsecount of clover #699

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f4925c2
fix: wrong output `truecount` and `falsecount` of clover
KiritaniAyaka Oct 23, 2022
66db410
fix: wrong output of clover for other types
KiritaniAyaka Nov 7, 2022
51ef498
fix: treat `switch` the same as `binary-expr`
KiritaniAyaka Jan 31, 2023
4328d29
chore: add description for clover
KiritaniAyaka Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Expand Up @@ -9,3 +9,4 @@ dist
lerna-debug.log
packages/*/yarn.lock
packages/nyc/
.vscode/*
50 changes: 44 additions & 6 deletions packages/istanbul-reports/lib/clover/index.js
View file
Open in desktop
Expand Up @@ -119,7 +119,7 @@ class CloverReport extends ReportBase {
onDetail(node) {
const fileCoverage = node.getFileCoverage();
const metrics = node.getCoverageSummary();
const branchByLine = fileCoverage.getBranchCoverageByLine();
const branchDetails = getBranchDetails(fileCoverage);

this.xml.openTag('file', {
name: asClassName(node),
Expand All @@ -135,13 +135,35 @@ class CloverReport extends ReportBase {
count,
type: 'stmt'
};
const branchDetail = branchByLine[k];
const branchDetail = branchDetails[k];

if (branchDetail) {
attrs.type = 'cond';
attrs.truecount = branchDetail.covered;
attrs.falsecount = branchDetail.total - branchDetail.covered;
if (!branchDetail || branchDetail.type === 'switch') {
Copy link
Member

Choose a reason for hiding this comment

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

What is the effect of adding a line tag here and returning immediately, will this result in 100% line coverage and no missing branch coverage?

Copy link
Author

Choose a reason for hiding this comment

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

It is from the original code:

if (branchDetail) {
    attrs.type = 'cond';
    attrs.truecount = branchDetail.covered;
    attrs.falsecount = branchDetail.total - branchDetail.covered;
}
this.xml.inlineTag('line', attrs);

but the true case is too complex now, so I move the false case into the brackets and change the condition to !branchDetail.

As to branchDetail.type === 'switch'. Thanks to you, I found a situation as you said:

switch (num) {
    case 0:
    case 1:
        console.log('Hello');
        break;
}

When we use num=1 to test this switch, all lines are covered but branches not.

Maybe we should treat switch the same as binary-expr.

return this.xml.inlineTag('line', attrs);
}

attrs.type = 'cond';
attrs.truecount = 0;
attrs.falsecount = 0;

if (count === 0) {
return this.xml.inlineTag('line', attrs);
}

if (['if', 'cond-expr'].includes(branchDetail.type)) {
Copy link
Member

Choose a reason for hiding this comment

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

This logic is complex enough, that I think it would be good to add a description above each condition, explaining the reasoning for the logic.

attrs.truecount = branchDetail.states[0];
attrs.falsecount = branchDetail.states[1];
} else if (
['binary-expr', 'default-arg'].includes(branchDetail.type)
) {
if (branchDetail.states.every(state => state > 0)) {
attrs.truecount = 1;
attrs.falsecount = 1;
} else {
attrs.truecount = 1;
attrs.falsecount = 0;
Comment on lines +164 to +169
Copy link
Author

Choose a reason for hiding this comment

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

Hi, @bcoe. The problem mentioned in issue #695 looks to make others puzzled now so I think it's needed to push this PR.

I guess this PR stop forward because you may disagree with the practice that assigning hardcode values to make 3rd-party parsers understand if this line was covered or not.

Could we have better practice to resolve this problem?

}
}

this.xml.inlineTag('line', attrs);
});

Expand All @@ -161,4 +183,20 @@ function asClassName(node) {
return node.getRelativeName().replace(/.*[\\/]/, '');
}

function getBranchDetails(fileCoverage) {
const branchMeta = fileCoverage.branchMap;
const branchStats = fileCoverage.b;

const branchDetails = {};

Object.entries(branchMeta).forEach(([index, branch]) => {
branchDetails[branch.loc.start.line] = {
type: branch.type,
states: branchStats[index]
};
});

return branchDetails;
}

module.exports = CloverReport;