Skip to content
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

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

tianon
Copy link
Member

@tianon tianon commented Mar 14, 2024

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

@tianon tianon requested a review from yosifkit as a code owner March 14, 2024 20:07
@tianon
Copy link
Member Author

tianon commented Mar 14, 2024

(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 😄)

@tianon
Copy link
Member Author

tianon commented Mar 14, 2024

Example output from the end of .test/test.sh:

.../cmd/builds/main.go:57:             resolveIndex               84.0%
.../cmd/builds/main.go:101:            resolveArchIndex           84.2%
.../cmd/builds/main.go:148:            loadCacheFromFile          76.0%
.../cmd/builds/main.go:190:            saveCacheToFile            78.6%
.../cmd/builds/main.go:215:            main                       80.7%
.../cmd/lookup/main.go:14:             main                       76.9%
.../om/om.go:17:                       Keys                       100.0%
.../om/om.go:21:                       Get                        100.0%
.../om/om.go:27:                       Set                        100.0%
.../om/om.go:38:                       UnmarshalJSON              72.7%
.../om/om.go:75:                       MarshalJSON                73.3%
.../registry/cache.go:19:              RegistryCache              100.0%
.../registry/cache.go:42:              cacheKeyDigest             100.0%
.../registry/cache.go:46:              cacheKeyTag                100.0%
.../registry/cache.go:51:              getBlob                    75.0%
.../registry/cache.go:92:              GetBlob                    100.0%
.../registry/cache.go:96:              GetManifest                100.0%
.../registry/cache.go:100:             GetTag                     69.6%
.../registry/client.go:17:             Client                     53.1%
.../registry/client.go:135:            EntryForRegistry           72.7%
.../registry/rate-limits.go:23:        Do                         50.0%
.../registry/read-helpers.go:15:       readJSONHelper             60.9%
.../registry/ref.go:18:                ParseRefNormalized         87.5%
.../registry/synthesize-index.go:17:   SynthesizeIndex            74.1%
.../registry/synthesize-index.go:141:  setRefAnnotation           100.0%
.../registry/synthesize-index.go:151:  normalizeManifestPlatform  84.6%
total:                                 (statements)               74.3%

https://github.com/docker-library/meta-scripts/pull/31/checks#step:5:354

@tianon
Copy link
Member Author

tianon commented Mar 14, 2024

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

@tianon
Copy link
Member Author

tianon commented Mar 14, 2024

(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).
Copy link
Collaborator

@whalelines whalelines left a 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"
Copy link
Collaborator

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?

Copy link
Member Author

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:

meta-scripts/builds.go

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"
Copy link
Collaborator

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.

Copy link
Member Author

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

@yosifkit yosifkit merged commit cf871c9 into docker-library:main Mar 20, 2024
1 check passed
@yosifkit yosifkit deleted the coverage branch March 20, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants