-
Notifications
You must be signed in to change notification settings - Fork 14
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
Adjust scripts/tests to generate Go code coverage data #31
Conversation
(I was working on #22 and as I wrote some unit tests for some new code I realized it would be nice if we could get actual code coverage and see how we're doing and found the new Go 1.20+ feature that lets us get even more interesting code coverage results 😄) |
Example output from the end of
https://github.com/docker-library/meta-scripts/pull/31/checks#step:5:354 |
Oh, and the good news is that the vast majority of our code coverage "misses" are the error cases, which are notoriously hard to test correctly. 💪 |
(also, actions/upload-artifact#14 (comment) is very relevant and weep-inducing) |
With Go 1.20+, we can generate coverage information from binaries (not just tests), so our "canonical output" tests can *also* generate appropriate coverage information so we can see more clearly how we're doing and where we can improve. I decided *not* to hook this up to Codecov for now, because I have not been happy with their service, but I did leave a `TODO` comment in the appropriate place for where/how we could implement similar functionality (like their "change in coverage" comments).
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.
Looks great, a couple questions
time "$dir/../builds.sh" --cache "$dir/cache-builds.json" "$dir/sources.json" > "$dir/builds.json" | ||
|
||
# test again, but with "--cache=..." instead of "--cache ..." (which also lets us delete the cache and get slightly better coverage reports at the expense of speed / Hub requests) | ||
time "$dir/../builds.sh" --cache="$dir/cache-builds.json" "$dir/sources.json" > "$dir/builds.json" |
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.
Why would having the equal sign or not change behavior?
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.
😅 because we're currently punting on choosing a flag-parsing library:
Lines 221 to 228 in 96ed6a9
// support "--cache foo.json" and "--cache=foo.json" | |
if sourcesJsonFile == "--cache" && len(os.Args) >= 4 { | |
cacheFile = os.Args[2] | |
sourcesJsonFile = os.Args[3] | |
} else if cf, ok := strings.CutPrefix(sourcesJsonFile, "--cache="); ok && len(os.Args) >= 3 { | |
cacheFile = cf | |
sourcesJsonFile = os.Args[2] | |
} |
Also though, running it a second time should give us slightly different coverage in the case of "totally fresh cache" which is useful for testing locally and has minimal impact in general thanks to our cache file).
rm -f "$dir/../bin/builds" "$dir/../bin/lookup" | ||
|
||
# Go tests | ||
"$dir/../.go-env.sh" go test -cover ./... -args -test.gocoverdir="$GOCOVERDIR" |
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.
Odd that go test
would not pick up the GOCOVERDIR
environment variable.
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.
Agreed, 100%! That "didn't make it into the feature in 1.20", but also doesn't seem to have made it in 1.21 or 1.22 either! (golang/go#51430 (comment))
With Go 1.20+, we can generate coverage information from binaries (not just tests), so our "canonical output" tests can also generate appropriate coverage information so we can see more clearly how we're doing and where we can improve.
I decided not to hook this up to Codecov for now, because I have not been happy with their service, but I did leave a
TODO
comment in the appropriate place for where/how we could implement similar functionality (like their "change in coverage" comments).