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: merge branch/statement/functionMap's together when merging two coverage reports #617

Merged
merged 6 commits into from
Sep 13, 2021
Merged

fix: merge branch/statement/functionMap's together when merging two coverage reports #617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eda927f
commutative file coverage merge
JustinChristensen Jul 3, 2021
d95caed
stylistic tweaks
JustinChristensen Jul 12, 2021
352ee43
restore all
JustinChristensen Jul 12, 2021
5d8ba43
fix test
JustinChristensen Jul 12, 2021
96cde5d
Merge branch 'master' into 322-commutative-file-coverage-merge
bcoe Sep 13, 2021
a81826b
Merge branch 'master' into 322-commutative-file-coverage-merge
bcoe Sep 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 72 additions & 18 deletions packages/istanbul-lib-coverage/lib/file-coverage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,47 @@ function assertValidObject(obj) {
}
}

const keyFromLoc = ({ start, end }) =>
`${start.line}|${start.column}|${end.line}|${end.column}`;

const mergeProp = (aHits, aMap, bHits, bMap, itemKey = keyFromLoc) => {
const aItems = Object.values(aHits).reduce((items, itemHits, i) => {
const item = aMap[i];
items[itemKey(item)] = [itemHits, item];
return items;
}, {});

Object.values(bHits).forEach((bItemHits, i) => {
const bItem = bMap[i];
const k = itemKey(bItem);

if (aItems[k]) {
const aPair = aItems[k];
if (bItemHits.forEach) {
// should this throw an exception if aPair[0] is not an array?
bItemHits.forEach((hits, h) => {
if (aPair[0][h] !== undefined) aPair[0][h] += hits;
else aPair[0][h] = hits;
});
} else {
aPair[0] += bItemHits;
}
} else {
aItems[k] = [bItemHits, bItem];
}
});

const hits = {};
const map = {};

Object.values(aItems).forEach(([itemHits, item], i) => {
hits[i] = itemHits;
map[i] = item;
});

return [hits, map];
};

/**
* provides a read-only view of coverage for a single file.
* The deep structure of this object is documented elsewhere. It has the following
Expand Down Expand Up @@ -166,24 +207,37 @@ class FileCoverage {
return;
}

Object.entries(other.s).forEach(([k, v]) => {
this.data.s[k] += v;
});
Object.entries(other.f).forEach(([k, v]) => {
this.data.f[k] += v;
});
Object.entries(other.b).forEach(([k, v]) => {
let i;
const retArray = this.data.b[k];
/* istanbul ignore if: is this even possible? */
if (!retArray) {
this.data.b[k] = v;
return;
}
for (i = 0; i < retArray.length; i += 1) {
retArray[i] += v[i];
}
});
let [hits, map] = mergeProp(
this.s,
this.statementMap,
other.s,
other.statementMap
);
this.data.s = hits;
this.data.statementMap = map;

const keyFromLocProp = x => keyFromLoc(x.loc);
const keyFromLocationsProp = x => keyFromLoc(x.locations[0]);

[hits, map] = mergeProp(
Copy link
Member

Choose a reason for hiding this comment

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

One slight concern I have about this change is the number of additional operations that will be performed on each coverage report as we iterate over all these branch coverage operations.

Maybe there's no way around it, if this is necessary for correct merging behavior, but it would be good to find a large codebase to test against.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bcoe The previous incarnation looped through each of these properties, including the branch hit counters, for just the "other" object. This new algorithm also needs to iterate through the properties for "this" object too. So in the worst case I would expect this to be potentially twice as slow, but that's a low price to pay for correctness.

Copy link
Member

Choose a reason for hiding this comment

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

@JustinChristensen I'm pretty sure on large codebases using c8, it won't ultimately merge many coverage files because the merge happens at prior to processing when the output is in v8 format.

I suppose the concern is a large codebase using tap and nyc, the npm CLI might be a good repo to test with -- if it works with npm, I'd be pretty confident we don't have too huge of a performance hit.

this.f,
this.fnMap,
other.f,
other.fnMap,
keyFromLocProp
);
this.data.f = hits;
this.data.fnMap = map;

[hits, map] = mergeProp(
this.b,
this.branchMap,
other.b,
other.branchMap,
keyFromLocationsProp
);
this.data.b = hits;
this.data.branchMap = map;
}

computeSimpleTotals(property) {
Expand Down
40 changes: 20 additions & 20 deletions packages/istanbul-lib-coverage/test/file.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,36 +153,36 @@ describe('base coverage', () => {
const template = new FileCoverage({
path: '/path/to/file',
statementMap: {
1: loc(1, 1, 1, 100),
2: loc(2, 1, 2, 50),
3: loc(2, 51, 2, 100),
4: loc(2, 101, 3, 100)
0: loc(1, 1, 1, 100),
Copy link
Member

Choose a reason for hiding this comment

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

It seems a bit odd to me that this new logic would have switched tests to being 0 offset vs., 1 offset.

Is this expected?

Copy link
Contributor Author

@JustinChristensen JustinChristensen Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@bcoe

The previous incarnation of merge preserved the object keys while zipping up the hit counters. As those keys are just ascending integers, and not a kind of identifier, it didn't seem particularly important to preserve them. Unless that behavior is documented somewhere, I suspect that the test was written with that unimportant implementation detail in mind, and not to validate an API's guarantees.

So my implementation just iterates through the hit counters in the order of Object.values, which I believe is now insertion order, for both the mappings and the hit counters. If that is now guaranteed to be insertion order then it should preserve the key correspondence between the mappings and the hit counters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I probably should check the spec on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://tc39.es/ecma262/multipage/ordinary-and-exotic-objects-behaviours.html#sec-ordinaryownpropertykeys

Yup.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good 👍

1: loc(2, 1, 2, 50),
2: loc(2, 51, 2, 100),
3: loc(2, 101, 3, 100)
},
fnMap: {
1: {
0: {
name: 'foobar',
line: 1,
loc: loc(1, 1, 1, 50)
}
},
branchMap: {
1: {
0: {
type: 'if',
line: 2,
locations: [loc(2, 1, 2, 20), loc(2, 50, 2, 100)]
}
},
s: {
0: 0,
1: 0,
2: 0,
3: 0,
4: 0
3: 0
},
f: {
1: 0
0: 0
},
b: {
1: [0, 0]
0: [0, 0]
}
});
const clone = function(obj) {
Expand All @@ -192,13 +192,13 @@ describe('base coverage', () => {
const c2 = new FileCoverage(clone(template));
let summary;

c1.s[1] = 1;
c1.f[1] = 1;
c1.b[1][0] = 1;
c1.s[0] = 1;
c1.f[0] = 1;
c1.b[0][0] = 1;

c2.s[2] = 1;
c2.f[1] = 1;
c2.b[1][1] = 2;
c2.s[1] = 1;
c2.f[0] = 1;
c2.b[0][1] = 2;
summary = c1.toSummary();
assert.ok(summary instanceof CoverageSummary);
assert.deepEqual(summary.statements, {
Expand Down Expand Up @@ -253,11 +253,11 @@ describe('base coverage', () => {
pct: 100
});

assert.equal(c1.s[0], 1);
assert.equal(c1.s[1], 1);
assert.equal(c1.s[2], 1);
assert.equal(c1.f[1], 2);
assert.equal(c1.b[1][0], 1);
assert.equal(c1.b[1][1], 2);
assert.equal(c1.f[0], 2);
assert.equal(c1.b[0][0], 1);
assert.equal(c1.b[0][1], 2);
});

it('drops all data during merges', () => {
Expand Down