swift-test-codecov icon indicating copy to clipboard operation
swift-test-codecov copied to clipboard

Feature: Optionally read JSON output of previous run and display coverage deltas

Open edelabar opened this issue 2 years ago • 7 comments

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"
}

edelabar avatar Dec 15 '22 18:12 edelabar

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.

mattpolzin avatar Dec 16 '22 04:12 mattpolzin

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?)

edelabar avatar Dec 16 '22 18:12 edelabar

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.

mattpolzin avatar Dec 17 '22 04:12 mattpolzin

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.

mattpolzin avatar Dec 18 '22 00:12 mattpolzin

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

mattpolzin avatar Dec 18 '22 00:12 mattpolzin

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 avatar Jan 18 '23 04:01 mattpolzin

@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.

edelabar avatar Mar 07 '23 18:03 edelabar