Handle multiple coverage files#483
Conversation
| testFiles.set("/file/123", "datastringhere"); | ||
| testFiles.set("/file/111", "datastring111"); | ||
| testFiles.set("/file/222", "datastring222"); | ||
| testFiles.set("/file/123", "samestringhere"); |
There was a problem hiding this comment.
As this is a map it only ever has 3 entries, making the test meaningless :(
There was a problem hiding this comment.
Whoops hah! Darn Ryan from many ages ago 😅.
There was a problem hiding this comment.
I'm leaving this one as draft as I'm not too sure how you've setup the data structures here. Please let me know if there are downstream consequences that I've missed. I couldn't see any.
This is looking good to me so far, downstream wise I don't think they account for duplicate coverage lines and more just focus on parsing test out into a single file format.
I did leave one question in the coverageparser though and how we might decide between different conflicts.
| testFiles.set("/file/123", "datastringhere"); | ||
| testFiles.set("/file/111", "datastring111"); | ||
| testFiles.set("/file/222", "datastring222"); | ||
| testFiles.set("/file/123", "samestringhere"); |
There was a problem hiding this comment.
Whoops hah! Darn Ryan from many ages ago 😅.
| const sections = new Map<string, Section>(); | ||
| const addToSectionsMap = async (section: Section) => { | ||
| sections.set(section.title + "::" + section.file, section); | ||
| sections.set([coverageFilename, section.title, section.file].join("::"), section); |
There was a problem hiding this comment.
I am curious how this will decide between two conflicting coverage files / lines? Do we just need to assume the "best case" scenario and pick the coverage line that is more positive?
There was a problem hiding this comment.
The answer is: this breaks at least one thing downstream (Displayed coverage %).
ryanluker
changed the title
| @@ -1,10058 +0,0 @@ | |||
| TN: | |||
There was a problem hiding this comment.
@ryanluker er.... why was this file so long?
There was a problem hiding this comment.
It was to test out "large coverage" files / big code repositories.
I ended up just copy and pasting a bunch of jiggerish to make it big enough to detecting performance regressions.
| switch (coverageFile.type) { | ||
| case CoverageType.CLOVER: | ||
| coverage = await this.xmlExtractClover(fileName, fileContent); | ||
| await this.xmlExtractClover( |
There was a problem hiding this comment.
[F] Each of these extract methods now mutate the original coverages Map.
There was a problem hiding this comment.
Gotcha that makes sense to allow for the multiple coverage files scenario.
| return; | ||
| } | ||
|
|
||
| const mergedSection = this.mergeSections(existingSection, section); |
There was a problem hiding this comment.
[F] If we already have a section with the current title and file, then we merge the line and branch information as they are used downstream.
| }); | ||
| } | ||
|
|
||
| private mergeSections(existingSection: Section, section: Section): Section { |
There was a problem hiding this comment.
[F] If you look at 66df5d7 you can see how I initially worked this out by updating the node example project.
| FNDA:1,test | ||
| DA:1,1 | ||
| DA:2,1 | ||
| DA:3,1 |
There was a problem hiding this comment.
Line 3 is only covered by the integration test in this example.
mattseddon
changed the title
| import { OutputChannel } from "vscode"; | ||
|
|
||
| import {CoverageFile, CoverageType} from "./coveragefile"; | ||
| import { CoverageFile, CoverageType } from "./coveragefile"; |
There was a problem hiding this comment.
Yeahhh this file hasn't been touched in a while, didn't even have the latest lintings 😓.
| }; | ||
| } | ||
|
|
||
| private mergeLineCoverage( |
There was a problem hiding this comment.
This merging logic looks correct to me, nice work 🦾.
ryanluker
left a comment
ryanluker
left a comment
There was a problem hiding this comment.
Very nice work! I will add this against the 2.14.0 release.
2 participants
Closes #481
cc @ryanluker - I'm leaving this one as draft as I'm not too sure how you've setup the data structures here. Please let me know if there are downstream consequences that I've missed. I couldn't see any.This PR gives the extension the ability to handle multiple coverage files.
I have left some comments inline.