swift-test-codecov
swift-test-codecov copied to clipboard
Feature: Optionally read JSON output of previous run and display coverage deltas
I wanted to unit test these changes and as I mentioned in #15 you can't do that in a command line executable. I could have moved all of the new logic to SwiftTestCodecovLib
, but IMHO that package is for coverage output parsing and this was mostly display logic, so I created SwiftTestCodecovLogic
, and everything in it is 100% covered. Other than that I don't feel strongly about this in any way, so if you want it as part of SwiftTestCodecovLib
to keep the structure simpler, that's fine too.
I changed Aggregate
by only adding fields, and most of them are optional. It does add the coveredProperty
property to the default export, but I set that up so it'll still read old output without that field, it just assumes the coveredProperty
is lines
. Seemed like a reasonable compromise...
Because I added fields to Aggregate
, on a delta run, the JSON output will have the delta data in it. I chose to keep this because I thought it would be useful if the caller wanted to parse the JSON into HTML or something. Assuming normal JSON parsing the additional fields should just be ignored. I guess the only downside to this is a larger JSON output, which might be a problem for a larger project. I could add another option to not export deltas in JSON or maybe a different print format, but both seemed complicated because they only apply if the delta file is provided and it's all bordering on YAGNI.
Updated --help
output is in the README.
Example Output
(Non-delta output is the same, but here for reference.)
Minimal w/ Delta
$ swift-test-codecov .build/debug/codecov/CoverageExample.json --print-format minimal --base-json-file base-coverage.json
89.66% (+10.34%)
Minimal w/o Delta
$ swift-test-codecov .build/debug/codecov/CoverageExample.json --print-format minimal
89.66%
Table w/ Delta
$ swift-test-codecov .build/debug/codecov/CoverageExample.json --print-format table --base-json-file base-coverage.json
Overall Coverage: 89.66% (+10.34%)
File Coverage Coverage Change
----------------- -------- ---------------
Added.swift 100.00% 100.00%
Deleted.swift (Removed)
DeltaLess.swift 62.50% -37.50%
DeltaMore.swift 100.00% +54.55%
NoChange.swift 100.00% -
=-=-=-=-=-=-=-=-=
Table w/o Delta
$ swift-test-codecov .build/debug/codecov/CoverageExample.json --print-format table
Overall Coverage: 89.66%
File Coverage
----------------- --------
Added.swift 100.00%
DeltaLess.swift 62.50%
DeltaMore.swift 100.00%
NoChange.swift 100.00%
=-=-=-=-=-=-=-=-=
Numeric Delta
$ swift-test-codecov .build/debug/codecov/CoverageExample.json --print-format numeric --base-json-file base-coverage.json
This returns the delta and not the total coverage. I don't have a particular use case for this, so no real opinion other than if you wanted the total coverage you could just run it again without the delta file. 🤷♂️
10.344827586206895
Numeric
$ swift-test-codecov .build/debug/codecov/CoverageExample.json --print-format numeric
89.65517241379311
JSON w/ Delta
$ swift-test-codecov .build/debug/codecov/CoverageExample.json --print-format json --base-json-file base-coverage.json
(Formatted and reordered for clarity)
{
"coveragePerFile": {
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/NoChange.swift": {
"count": 5,
"covered": 5,
"percent": 100
},
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/DeltaMore.swift": {
"count": 11,
"covered": 11,
"percent": 100
},
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/DeltaLess.swift": {
"count": 8,
"covered": 5,
"percent": 62.5
},
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/Added.swift": {
"count": 5,
"covered": 5,
"percent": 100
}
},
"coverageDeltaPerFile": {
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/NoChange.swift": {
"noChange": {}
},
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/DeltaMore.swift": {
"delta": {
"coverageChange": {
"count": 0,
"covered": 6,
"percent": 54.54545454545455
}
}
},
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/DeltaLess.swift": {
"delta": {
"coverageChange": {
"count": 0,
"covered": -3,
"percent": -37.5
}
}
},
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/Deleted.swift": {
"fileRemoved": {}
},
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/Added.swift": {
"fileAdded": {
"newCoverage": {
"count": 5,
"covered": 5,
"percent": 100
}
}
}
},
"totalCount": 29,
"totalCountDelta": 0,
"overallCoverage": 0.896551724137931,
"overallCoverageDelta": 0.10344827586206895,
"coveredProperty": "lines"
}
JSON w/o Delta
$ swift-test-codecov .build/debug/codecov/CoverageExample.json --print-format json
(Formatted and reordered for clarity)
{
"coveragePerFile": {
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/DeltaMore.swift": {
"count": 11,
"covered": 11,
"percent": 100
},
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/NoChange.swift": {
"count": 5,
"covered": 5,
"percent": 100
},
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/DeltaLess.swift": {
"count": 8,
"covered": 5,
"percent": 62.5
},
"/Users/edelabar/Code/projects/CoverageExample/Sources/CoverageExample/Added.swift": {
"count": 5,
"covered": 5,
"percent": 100
}
},
"totalCount": 29,
"overallCoverage": 0.896551724137931,
"coveredProperty": "lines"
}
So, my second question for discussion:
Would you mind moving the properties of Aggregate
that only apply when there is a delta comparison into a new struct along with the hash of files -> deltas? That way there isn't just an implied correlation between all the various delta properties being available or nil
, there is a codified guarantee that the whole delta substruct is either there or nil
.
Would you mind moving the properties of Aggregate that only apply when there is a delta comparison into a new struct along with the hash of files -> deltas? That way there isn't just an implied correlation between all the various delta properties being available or nil, there is a codified guarantee that the whole delta substruct is either there or nil.
@mattpolzin Good call, I like that!
Also, I just ran into an interesting problem in my usage, see this commit that I just pushed: bba5945
I assume this isn't a problem? We have a package that currently doesn't have tests, but we still want to generate a coverage report for it to call that out. If it returns an error it kills the GitHub action, which prevents the report from being generated, so this commit makes 0% a valid case. (I assume this was just an overlooked edge case?)
We have a package that currently doesn't have tests, but we still want to generate a coverage report for it to call that out. If it returns an error it kills the GitHub action, which prevents the report from being generated, so this commit makes 0% a valid case. (I assume this was just an overlooked edge case?)
Yep, that code used to be buggy. Definitely should be greater-than-or-equal-to the minimum.
I don't recall where, but earlier you noticed this tool couldn't analyze its own test coverage; I've fixed that with https://github.com/mattpolzin/swift-test-codecov/pull/18.
Regarding JSON output: I think it is nice to put the delta in there when it is calculated. I have a problem with Swift's default encoding of enums with associated values, though. I find it pretty terrible. As much as it sucks to write custom encoding/decoding for such a silly reason, I propose this (your PR except I've implemented my own encoding/decoding for the delta: https://github.com/mattpolzin/swift-test-codecov/compare/deltas-rev-json#diff-1dea3a6793522ec9553550f8266868f16714199bbce4e42db782b9dd0c0c19f3R29-R82
I'm about to merge a PR that will create a merge conflict with this one ever-so-slightly (and I am snagging your bug fix for only checking that the coverage is greater-than-or-equal-to the minimum).
I'm still very interested in seeing the work in this PR get merged, but I imagine life happened or priorities shifted and that's no problem at all!
@mattpolzin Sorry to fall of the face of the earth on this one! Things got a little crazy at the day job, but gonna try and get this over the end zone in the next day or two.