-
-
Notifications
You must be signed in to change notification settings - Fork 234
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
fix: merge branch/statement/functionMap's together when merging two coverage reports #617
Changes from 4 commits
eda927f
d95caed
352ee43
5d8ba43
96cde5d
a81826b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I probably should check the spec on that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
@@ -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, { | ||
|
@@ -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', () => { | ||
|
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.
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.
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.
@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.
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.
@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 inv8
format.I suppose the concern is a large codebase using
tap
andnyc
, thenpm
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.