CI: set -coverpkg when calling go test in nightly builds#3185
CI: set -coverpkg when calling go test in nightly builds#3185algorandskiy merged 30 commits intoalgorand:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3185 +/- ##
==========================================
- Coverage 47.90% 47.75% -0.15%
==========================================
Files 662 645 -17
Lines 87991 87795 -196
==========================================
- Hits 42149 41930 -219
- Misses 43085 43118 +33
+ Partials 2757 2747 -10 ☔ View full report in Codecov by Sentry. |
|
There is some anecdotal evidence that this change makes the time.Sleep() timing assumptions in the metrics tests flakier.. the metrics tests are failing on all three platforms. Update: fixed by removing ledger/testing from the coverpkg list, it had a call to crypto.GenerateSignatureSecrets in its init() function that was bumping a metric, and we don't need to track coverage for packages that are only used by tests. |
This reverts commit 22c1ed6.
|
Well, the tests pass now, and coverage reports more accurately reflect what is being tested, but collecting atomic coverage reports from all the lines in these previously-uncounted go-algorand package import dependencies made the {amd64,arm64,mac_amd64}_test jobs get ~50% slower, from ~22 minutes to ~34 minutes. So without more parallelism that would make the test jobs the new slowest jobs, previously they were taking roughly about the same amount of time as the expect tests (~25 minutes). |
|
Can revisit later |
cce
changed the title
cce
changed the title
Port the -coverpkg nightly coverage changes from the old CircleCI config to the GitHub Actions ci-nightly.yml workflow. Remove the now-deleted .circleci/config.yml. Pass "$@" through in upload_coverage.sh and add the full_coverage codecov flag to nightly coverage uploads.
Add a workflow_dispatch trigger to ci-pr.yml with a full_coverage option. When enabled, the test job passes -coverpkg to go test to measure coverage across all packages, and tags the codecov upload with the full_coverage flag. Also rename the codecov flag from "nightly" to "full_coverage" to match the actual upload tag.
Export the previously unexported makeCounter/makeGauge constructors so callers outside the metrics package can create metrics without auto-registering on the default registry. Use these in tests and in metricsTracker to avoid the Deregister(nil)+Register(registry) pattern. Add a registry field to metricsTracker so loadFromDisk and close operate on the correct registry, fixing TestMetricsReload under -coverpkg without needing t.Skip().
algorandskiy
left a comment
algorandskiy
left a comment
There was a problem hiding this comment.
LGTM, please fix reviewdog
There was a problem hiding this comment.
Pull request overview
This PR enhances Go test coverage reporting by adding support for cross-package coverage measurement using the -coverpkg flag. The changes enable more accurate coverage reporting by tracking coverage across all packages, not just within the same package as the tests.
Changes:
- Added
-coverpkgflag to nightly CI builds for comprehensive coverage reporting - Introduced optional registry parameter to metrics system to support test isolation
- Refactored internal metric creation functions to be publicly exported for better testability
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ci-nightly.yml |
Added -coverpkg flag with filtered package list for full coverage measurement in nightly builds |
.github/workflows/ci-pr.yml |
Added optional full_coverage workflow input parameter with conditional -coverpkg flag |
.codecov.yml |
Configured full_coverage flag as separate, non-joined coverage reporting stream |
Makefile |
Enhanced cover target with -coverpkg support and package filtering logic |
scripts/travis/upload_coverage.sh |
Modified to pass through additional arguments to codecov script |
util/metrics/serviceCommon.go |
Added optional registry field to ServiceConfig for test isolation |
util/metrics/reporter.go |
Updated to use optional registry when available |
util/metrics/counter.go |
Renamed makeCounter to MakeCounterUnregistered and made it public |
util/metrics/gauge.go |
Renamed makeGauge to MakeGaugeUnregistered and made it public |
util/metrics/prometheus.go |
Updated to use new public unregistered metric constructors |
util/metrics/opencensus.go |
Updated to use new public unregistered metric constructors |
util/metrics/registry_test.go |
Refactored to use non-default registry for test isolation |
util/metrics/counter_test.go |
Updated all test functions to use non-default registry |
util/metrics/gauge_test.go |
Updated to use non-default registry for test isolation |
ledger/metrics.go |
Added registry field and updated to use unregistered constructors |
ledger/metrics_test.go |
Updated to use non-default registry to avoid cross-package interference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
If I'm honest, I have no idea what any of the metrics changes do.
I also didn't follow what actually happens after this change. Nightly coverage is calculated, but where does it show up? It seems like CI coverage is only useful if it's attached to the code that caused the changes. Where would I go to see the nightly coverage, and who will go look at it repeatedly to see how it changes?
I will update the PR description — they were touching global counter state and now that multiple packages are being compiled together, it was causing test assertions to fail that assumed that counter state was empty.
After this change you would go to the codecov website to see the nightly coverage — I need to get this merged so that we can start to see it show up as a base for comparison — and then could trigger coverage diffs on PRs by triggering a special Github Actions workflow to run it against your PR. |
You mentioned that in stand-up and I didn't have a chance to understand why, seemed like compiling together is not a problem. But reading comments above, it seems the issue is
okay! |
Summary
By default, Go test with -coverprofile enabled only measures the coverage of files in the same package as the tests. Providing -coverpkg provides go test with additional names of packages for which you'd like it to measure coverage. Today we don't see the true coverage of source files called from tests in other packages.
Test Plan
This should not impact test correctness, but slows down tests due to the extra line counting required to measure coverage across more packages.
Due to the performance impact I updated the PR to only do "full coverage" reporting for nightly builds and am using the codecov flags feature to have these nightly builds be separately categorized by Codecov.
https://docs.codecov.com/docs/flags#hide-builds-eg-nightly-builds