Re-enable coverage upload and close remaining coverage gaps - #161
Merged
Conversation
The Coveralls upload step was gated on refs/heads/main, but the default
branch is master, so coverage has not been published since January 2024.
Point it at master.
Since Go 1.22 the untested example binary counted as 0% and pulled the
module total from ~95% to ~86%. main() is split into newHandler /
printInstructions (plus a new -addr flag) so a smoke test can run the
example circuits against an httptest server and check both /debug/vars
and /hystrix.stream. The background goroutines still panic if a circuit
misbehaves, so this doubles as an integration check.
New unit tests for previously uncovered paths:
- GeneralConfig CustomConfig merging (copy, no aliasing, receiver wins)
- Circuit/Manager Var output with rolling run + fallback stats, including
a throttled fallback
- Configurable open/close logic receiving SetConfig{,Not}ThreadSafe
- ClosedToOpen.Prevent short-circuiting without opening
- ForcedClosed never transitioning to open, then opening once cleared
- nil Circuit/Manager receivers; SimpleBadRequest Cause/Unwrap
- rolling FallbackStats.Var and negative config sanitizing
- responsetimeslo Factory with per-circuit config and collectors
- hystrix Opener err_% in MarshalJSON
- metriceventstream hystrix property reporting and non-flushable writer
- faststats RollingSum and Atomic UnmarshalJSON error paths
- evar fallback for a Var whose String() is not JSON
Total statement coverage goes from 86.4% to ~97%; the root package,
closers/hystrix, metrics/rolling and metrics/responsetimeslo are at 100%.
No library (non-example) source changes.
Newer golangci-lint counts literals in _test.go files toward goconst's per-package total, so the new tests tripped it for the expvar key names already used twice in rolling.go and for a repeated value in config_test.go. Decode into a struct instead of a keyed map and use distinct values. No-Verification-Needed: test-only change
golangci-lint-action installed whatever the latest linter was, so a new goconst behavior broke this branch's CI while local runs on the previous release were clean. Pin the version through GOLANGCI_LINT_VERSION with a renovate marker and enable the customManagers:githubActionsVersions preset so upgrades (and any new findings) show up in their own PR. No-Verification-Needed: CI configuration and README only
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Coverage has not been published to Coveralls since January 2024 because the upload step was gated on
refs/heads/mainwhile the default branch ismaster; this points it atmaster. It also adds tests for the remaining uncovered paths so the number that gets published is meaningful: total statement coverage goes from 86.4% to ~97%, with the root package,closers/hystrix,metrics/rollingandmetrics/responsetimesloat 100%.The biggest single gap was the
examplebinary, which since Go 1.22 counts as 0% and dragged the module total down ~9 points.main()is split intonewHandler/printInstructions(plus an-addrflag) so a smoke test can run the example circuits against anhttptestserver and check/debug/varsand/hystrix.stream; its background goroutines still panic if a circuit misbehaves, so it doubles as an integration check.The remaining tests cover real behavior rather than just lines:
CustomConfigmerging, expvar output with rolling run/fallback stats (including a throttled fallback),Configurablelogic receiving config updates,Preventshort-circuiting,ForcedClosednever opening, SLO factory per-circuit config/collectors, hystrix property reporting on the event stream, and a handful of nil-receiver / error-path cases.No library source changes outside
example/; public API untouched.Test plan
make ci(build,go test -race -count 10 ./..., golangci-lint) passes locally. Built and ranexample -addr 127.0.0.1:8124 -interval 20ms:/hystrix.streamreturns 200text/event-streamwith data events,/debug/varslists all nine circuits, process stays up. Coveralls upload itself can only be confirmed after merge tomaster.