From e45d9204ca57274bc4c8a6d36f18f9d4c3906093 Mon Sep 17 00:00:00 2001 From: Jack Lindamood Date: Tue, 18 Aug 2026 11:00:32 +0000 Subject: [PATCH 1/3] test: re-enable coverage upload and close remaining coverage gaps 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. --- .github/workflows/build.yml | 2 +- circuit_test.go | 61 +++++++++++ closers/hystrix/opener_test.go | 26 +++++ closers_test.go | 101 +++++++++++++++++++ config_test.go | 30 ++++++ errors_test.go | 10 ++ example/main.go | 57 +++++++---- example/main_test.go | 95 +++++++++++++++++ expvar_test.go | 71 +++++++++++++ faststats/atomic_test.go | 19 ++++ faststats/rolling_counter_test.go | 9 ++ internal/evar/evar_test.go | 9 ++ metriceventstream/metriceventstream_test.go | 78 ++++++++++++++ metrics/responsetimeslo/responsetime_test.go | 76 ++++++++++++++ metrics/rolling/rolling_test.go | 58 +++++++++++ 15 files changed, 681 insertions(+), 21 deletions(-) create mode 100644 example/main_test.go create mode 100644 expvar_test.go diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 19cdf6b..174b8f9 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -39,6 +39,6 @@ jobs: run: go test -v -covermode=count -coverprofile=coverage.out ./... - name: upload coverage uses: shogo82148/actions-goveralls@v1 - if: github.ref == 'refs/heads/main' && github.event_name == 'push' && matrix.go-version == '1.26.x' + if: github.ref == 'refs/heads/master' && github.event_name == 'push' && matrix.go-version == '1.26.x' with: path-to-profile: coverage.out diff --git a/circuit_test.go b/circuit_test.go index f0cb2b4..8134493 100644 --- a/circuit_test.go +++ b/circuit_test.go @@ -786,3 +786,64 @@ func TestCloseCircuit_EmitsClosedExactlyOnce(t *testing.T) { t.Errorf("Closed() called %d times, want exactly 1", got) } } + +func TestNilReceivers(t *testing.T) { + var c *Circuit + if c.Name() != "" { + t.Error("nil circuit should have no name") + } + if s := c.Var().String(); s != "null" { + t.Errorf("unexpected Var for nil circuit: %s", s) + } + var h *Manager + if h.AllCircuits() != nil { + t.Error("nil manager should have no circuits") + } + if h.GetCircuit("anything") != nil { + t.Error("nil manager should find no circuit") + } +} + +// A ForcedClosed circuit never transitions to open, even when its opener wants to, so clearing the override +// leaves it closed rather than springing open from stale state. +func TestForcedClosedNeverOpens(t *testing.T) { + ctx := context.Background() + var opened faststats.AtomicInt64 + c := NewCircuitFromConfig("forced-closed", Config{ + General: GeneralConfig{ + ForcedClosed: true, + ClosedToOpenFactory: func() ClosedToOpen { return alwaysOpens{} }, + }, + Metrics: MetricsCollectors{Circuit: []Metrics{openedCounter{&opened}}}, + }) + for i := 0; i < 5; i++ { + err := c.Execute(ctx, testhelp.AlwaysFails, nil) + if err == nil { + t.Fatal("expected the run error back") + } + } + if c.IsOpen() || opened.Get() != 0 { + t.Fatalf("forced-closed circuit opened: IsOpen=%v opened=%d", c.IsOpen(), opened.Get()) + } + cfg := c.Config() + cfg.General.ForcedClosed = false + c.SetConfigThreadSafe(cfg) + if c.IsOpen() { + t.Fatal("clearing ForcedClosed must not reveal an open circuit") + } + if err := c.Execute(ctx, testhelp.AlwaysFails, nil); err == nil { + t.Fatal("expected the run error back") + } + if !c.IsOpen() || opened.Get() != 1 { + t.Fatalf("circuit should open once the override is gone: IsOpen=%v opened=%d", c.IsOpen(), opened.Get()) + } +} + +type alwaysOpens struct{ neverOpens } + +func (alwaysOpens) ShouldOpen(context.Context, time.Time) bool { return true } + +type openedCounter struct{ n *faststats.AtomicInt64 } + +func (o openedCounter) Opened(context.Context, time.Time) { o.n.Add(1) } +func (o openedCounter) Closed(context.Context, time.Time) {} diff --git a/closers/hystrix/opener_test.go b/closers/hystrix/opener_test.go index daae423..df90b80 100644 --- a/closers/hystrix/opener_test.go +++ b/closers/hystrix/opener_test.go @@ -2,6 +2,7 @@ package hystrix import ( "context" + "encoding/json" "strings" "sync" "testing" @@ -96,3 +97,28 @@ func TestOpenerFactory_ConcurrentCreation(t *testing.T) { } wg.Wait() } + +func TestOpener_MarshalJSON_ErrPercentage(t *testing.T) { + ctx := context.Background() + now := time.Now() + o := OpenerFactory(ConfigureOpener{Now: func() time.Time { return now }})().(*Opener) + o.Success(ctx, now, time.Millisecond) + o.Success(ctx, now, time.Millisecond) + o.Success(ctx, now, time.Millisecond) + o.ErrFailure(ctx, now, time.Millisecond) + b, err := o.MarshalJSON() + if err != nil { + t.Fatal(err) + } + var out map[string]json.RawMessage + if err := json.Unmarshal(b, &out); err != nil { + t.Fatal(err) + } + var pct float64 + if err := json.Unmarshal(out["err_%"], &pct); err != nil { + t.Fatalf("%s: %v", b, err) + } + if pct != 0.25 { + t.Fatalf("expected 25%% errors, got %v from %s", pct, b) + } +} diff --git a/closers_test.go b/closers_test.go index 357d195..17fbbee 100644 --- a/closers_test.go +++ b/closers_test.go @@ -21,3 +21,104 @@ func TestNeverClose(t *testing.T) { require.False(t, c.Allow(ctx, time.Now())) require.False(t, c.ShouldClose(ctx, time.Now())) } + +// TestNeverOpensNeverClosesIgnoreEvents runs every RunMetrics/Metrics event through the default no-op logic and +// checks none of them change its answer. +func TestNeverOpensNeverClosesIgnoreEvents(t *testing.T) { + ctx := context.Background() + now := time.Now() + opener := neverOpensFactory() + closer := neverClosesFactory() + for _, m := range []interface { + RunMetrics + Metrics + }{opener, closer} { + m.Success(ctx, now, time.Millisecond) + m.ErrFailure(ctx, now, time.Millisecond) + m.ErrTimeout(ctx, now, time.Millisecond) + m.ErrBadRequest(ctx, now, time.Millisecond) + m.ErrInterrupt(ctx, now, time.Millisecond) + m.ErrConcurrencyLimitReject(ctx, now) + m.ErrShortCircuit(ctx, now) + m.Opened(ctx, now) + m.Closed(ctx, now) + } + require.False(t, opener.ShouldOpen(ctx, now)) + require.False(t, opener.Prevent(ctx, now)) + require.False(t, closer.ShouldClose(ctx, now)) + require.False(t, closer.Allow(ctx, now)) +} + +type configurableOpener struct { + neverOpens + prevent bool + threadSafe, notThread []Config +} + +func (c *configurableOpener) Prevent(context.Context, time.Time) bool { return c.prevent } +func (c *configurableOpener) SetConfigThreadSafe(props Config) { + c.threadSafe = append(c.threadSafe, props) +} +func (c *configurableOpener) SetConfigNotThreadSafe(props Config) { + c.notThread = append(c.notThread, props) +} + +type configurableCloser struct { + neverCloses + threadSafe, notThread []Config +} + +func (c *configurableCloser) SetConfigThreadSafe(props Config) { + c.threadSafe = append(c.threadSafe, props) +} +func (c *configurableCloser) SetConfigNotThreadSafe(props Config) { + c.notThread = append(c.notThread, props) +} + +// Open/close logic that implements Configurable is handed the circuit's config on construction and on live updates. +func TestConfigurableLogicReceivesConfig(t *testing.T) { + opener := &configurableOpener{} + closer := &configurableCloser{} + c := NewCircuitFromConfig("configurable", Config{ + General: GeneralConfig{ + ClosedToOpenFactory: func() ClosedToOpen { return opener }, + OpenToClosedFactory: func() OpenToClosed { return closer }, + }, + Execution: ExecutionConfig{MaxConcurrentRequests: 3}, + }) + require.Len(t, opener.notThread, 1) + require.Len(t, closer.notThread, 1) + require.Equal(t, int64(3), opener.notThread[0].Execution.MaxConcurrentRequests) + liveUpdates := len(opener.threadSafe) + + cfg := c.Config() + cfg.Execution.MaxConcurrentRequests = 9 + c.SetConfigThreadSafe(cfg) + require.Len(t, opener.notThread, 1) + require.Len(t, opener.threadSafe, liveUpdates+1) + require.Len(t, closer.threadSafe, liveUpdates+1) + require.Equal(t, int64(9), opener.threadSafe[liveUpdates].Execution.MaxConcurrentRequests) + require.Equal(t, int64(9), closer.threadSafe[liveUpdates].Execution.MaxConcurrentRequests) +} + +// ClosedToOpen.Prevent short-circuits a request on a closed circuit without opening it. +func TestPreventShortCircuits(t *testing.T) { + opener := &configurableOpener{prevent: true} + c := NewCircuitFromConfig("prevent", Config{ + General: GeneralConfig{ClosedToOpenFactory: func() ClosedToOpen { return opener }}, + }) + ran := false + err := c.Execute(context.Background(), func(context.Context) error { + ran = true + return nil + }, nil) + require.Error(t, err) + require.False(t, ran) + require.False(t, c.IsOpen()) + var ce Error + require.ErrorAs(t, err, &ce) + require.True(t, ce.CircuitOpen()) + + opener.prevent = false + require.NoError(t, c.Execute(context.Background(), func(context.Context) error { return nil }, nil)) +} diff --git a/config_test.go b/config_test.go index d950895..fe288a4 100644 --- a/config_test.go +++ b/config_test.go @@ -82,3 +82,33 @@ func TestExecutionConfig_Merge(t *testing.T) { assert.True(t, cfg.IsErrInterrupt(nil)) }) } + +func TestGeneralConfig_MergeCustomConfig(t *testing.T) { + t.Run("nothing to merge leaves nil map", func(t *testing.T) { + cfg := GeneralConfig{} + cfg.merge(GeneralConfig{}) + assert.Nil(t, cfg.CustomConfig) + }) + + t.Run("copies into nil receiver map without aliasing", func(t *testing.T) { + other := GeneralConfig{CustomConfig: map[interface{}]interface{}{"a": 1, "b": "two"}} + cfg := GeneralConfig{} + cfg.merge(other) + assert.Equal(t, map[interface{}]interface{}{"a": 1, "b": "two"}, cfg.CustomConfig) + cfg.CustomConfig["c"] = 3 + _, leaked := other.CustomConfig["c"] + assert.False(t, leaked, "merge must copy, not alias, the other map") + }) + + t.Run("receiver keys win", func(t *testing.T) { + cfg := GeneralConfig{CustomConfig: map[interface{}]interface{}{"a": "mine"}} + cfg.merge(GeneralConfig{CustomConfig: map[interface{}]interface{}{"a": "theirs", "b": "theirs"}}) + assert.Equal(t, map[interface{}]interface{}{"a": "mine", "b": "theirs"}, cfg.CustomConfig) + }) + + t.Run("Config.Merge carries CustomConfig", func(t *testing.T) { + cfg := Config{} + cfg.Merge(Config{General: GeneralConfig{CustomConfig: map[interface{}]interface{}{"k": "v"}}}) + assert.Equal(t, "v", cfg.General.CustomConfig["k"]) + }) +} diff --git a/errors_test.go b/errors_test.go index 0c97d9d..81b3906 100644 --- a/errors_test.go +++ b/errors_test.go @@ -28,3 +28,13 @@ func TestIsBadRequest(t *testing.T) { require.True(t, IsBadRequest(wrappedErr)) require.False(t, IsBadRequest(fmt.Errorf("wrapped: %w", errors.New("not bad")))) } + +func TestSimpleBadRequest_CauseUnwrap(t *testing.T) { + inner := errors.New("inner") + s := SimpleBadRequest{Err: inner} + require.Same(t, inner, s.Cause()) + require.Same(t, inner, s.Unwrap()) + require.ErrorIs(t, s, inner) + require.ErrorIs(t, fmt.Errorf("wrapped: %w", s), inner) + require.Equal(t, "inner", s.Error()) +} diff --git a/example/main.go b/example/main.go index 6dfb850..697350e 100644 --- a/example/main.go +++ b/example/main.go @@ -25,42 +25,59 @@ import ( const exampleURL = "http://localhost:7979/hystrix-dashboard/monitor/monitor.html?streams=%5B%7B%22name%22%3A%22%22%2C%22stream%22%3A%22http%3A%2F%2Flocalhost%3A8123%2Fhystrix.stream%22%2C%22auth%22%3A%22%22%2C%22delay%22%3A%22%22%7D%5D" func main() { + interval := flag.Duration("interval", time.Millisecond*100, "Setup duration between metric ticks") + addr := flag.String("addr", "127.0.0.1:8123", "Address to listen on") + flag.Parse() + sock, err := net.Listen("tcp", *addr) + if err != nil { + log.Fatal(err) + } + printInstructions(sock.Addr().String()) + handler, es := newHandler(*interval) + go func() { + mustPass(es.Start()) + }() + srv := http.Server{Handler: handler, ReadHeaderTimeout: time.Second} + log.Fatal(srv.Serve(sock)) +} + +// newHandler creates the example circuits and returns a handler serving /hystrix.stream and /debug/vars, plus the +// event stream backing /hystrix.stream. The caller must Start and eventually Close the stream. +func newHandler(interval time.Duration) (http.Handler, *metriceventstream.MetricEventStream) { f := rolling.StatFactory{} - h := circuit.Manager{ + h := &circuit.Manager{ DefaultCircuitProperties: []circuit.CommandPropertiesConstructor{f.CreateConfig}, } - expvar.Publish("hystrix", h.Var()) - es := metriceventstream.MetricEventStream{ - Manager: &h, + if expvar.Get("hystrix") == nil { + expvar.Publish("hystrix", h.Var()) } - go func() { - log.Fatal(es.Start()) - }() - interval := flag.Duration("interval", time.Millisecond*100, "Setup duration between metric ticks") - flag.Parse() - createBackgroundCircuits(&h, *interval) - http.Handle("/hystrix.stream", &es) - sock, err := net.Listen("tcp", "127.0.0.1:8123") - if err != nil { - panic(err) + es := &metriceventstream.MetricEventStream{ + Manager: h, } - log.Println("Serving on socket :8123") + createBackgroundCircuits(h, interval) + mux := http.NewServeMux() + mux.Handle("/hystrix.stream", es) + mux.Handle("/debug/vars", expvar.Handler()) + return mux, es +} + +func printInstructions(addr string) { + log.Printf("Serving on socket %s\n", addr) log.Println("To view the stream, execute: ") - log.Println(" curl http://127.0.0.1:8123/hystrix.stream") + log.Printf(" curl http://%s/hystrix.stream\n", addr) log.Println() log.Println("To view expvar metrics, visit expvar in your browser") - log.Println(" http://127.0.0.1:8123/debug/vars") + log.Printf(" http://%s/debug/vars\n", addr) log.Println() log.Println("To view a dashboard, follow the instructions at https://github.com/Netflix-Skunkworks/hystrix-dashboard#run-via-gradle") log.Println(" git clone git@github.com:Netflix-Skunkworks/hystrix-dashboard.git") log.Println(" cd hystrix-dashboard") log.Println(" ./gradlew jettyRun") log.Println() - log.Println("Then, add the stream http://127.0.0.1:8123/hystrix.stream") + log.Printf("Then, add the stream http://%s/hystrix.stream\n", addr) log.Println() - log.Println("A URL directly to the page usually looks something like this") + log.Println("A URL directly to the page usually looks something like this (adjust the port if you changed -addr)") log.Printf(" %s\n", exampleURL) - log.Fatal(http.Serve(sock, nil)) } func mustFail(err error) { diff --git a/example/main_test.go b/example/main_test.go new file mode 100644 index 0000000..5f607cd --- /dev/null +++ b/example/main_test.go @@ -0,0 +1,95 @@ +package main + +import ( + "bufio" + "context" + "encoding/json" + "io" + "log" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + "time" +) + +// TestExample runs the example's circuits against an in-process server and checks that both the expvar and the +// hystrix event stream endpoints describe them. The background goroutines panic (via mustPass/mustFail) if a +// circuit stops behaving the way the example expects, so this doubles as a smoke test of the library. +func TestExample(t *testing.T) { + log.SetOutput(io.Discard) + defer log.SetOutput(os.Stderr) + handler, es := newHandler(time.Millisecond) + startErr := make(chan error, 1) + go func() { startErr <- es.Start() }() + srv := httptest.NewServer(handler) + defer srv.Close() + + printInstructions(srv.Listener.Addr().String()) + + t.Run("expvar", func(t *testing.T) { + resp, err := http.Get(srv.URL + "/debug/vars") + if err != nil { + t.Fatal(err) + } + defer func() { + if err := resp.Body.Close(); err != nil { + t.Error(err) + } + }() + var vars struct { + Hystrix map[string]struct { + Name string `json:"name"` + } `json:"hystrix"` + } + if err := json.NewDecoder(resp.Body).Decode(&vars); err != nil { + t.Fatal(err) + } + for _, expected := range []string{"always-fails", "always-passes", "floppy-circuit", "throttled-circuit"} { + if c, ok := vars.Hystrix[expected]; !ok || c.Name != expected { + t.Errorf("circuit %q missing from expvar output: %v", expected, vars.Hystrix) + } + } + }) + + t.Run("hystrix.stream", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + req, err := http.NewRequestWithContext(ctx, http.MethodGet, srv.URL+"/hystrix.stream", nil) + if err != nil { + t.Fatal(err) + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + defer func() { + if err := resp.Body.Close(); err != nil { + t.Error(err) + } + }() + if ct := resp.Header.Get("Content-Type"); !strings.HasPrefix(ct, "text/event-stream") { + t.Errorf("unexpected content type %q", ct) + } + scanner := bufio.NewScanner(resp.Body) + for scanner.Scan() { + if strings.Contains(scanner.Text(), `"always-times-out"`) { + return + } + } + t.Fatalf("never saw a circuit on the event stream: %v", scanner.Err()) + }) + + if err := es.Close(); err != nil { + t.Fatal(err) + } + select { + case err := <-startErr: + if err != nil { + t.Fatal(err) + } + case <-time.After(5 * time.Second): + t.Fatal("event stream did not stop after Close") + } +} diff --git a/expvar_test.go b/expvar_test.go new file mode 100644 index 0000000..581cfbd --- /dev/null +++ b/expvar_test.go @@ -0,0 +1,71 @@ +package circuit_test + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/cep21/circuit/v4" + "github.com/cep21/circuit/v4/metrics/rolling" + "github.com/stretchr/testify/require" +) + +// TestCircuit_VarWithRollingStats drives a circuit that has rolling run and fallback stats through each fallback +// outcome and checks the expvar output includes those collectors with the right counts. +func TestCircuit_VarWithRollingStats(t *testing.T) { + ctx := context.Background() + f := rolling.StatFactory{} + m := circuit.Manager{DefaultCircuitProperties: []circuit.CommandPropertiesConstructor{f.CreateConfig}} + c := m.MustCreateCircuit("var-rolling", circuit.Config{ + Fallback: circuit.FallbackConfig{MaxConcurrentRequests: 1}, + }) + fails := func(context.Context) error { return errors.New("boom") } + recovers := func(context.Context, error) error { return nil } + + require.NoError(t, c.Execute(ctx, func(context.Context) error { return nil }, nil)) + require.NoError(t, c.Execute(ctx, fails, recovers)) + require.Error(t, c.Execute(ctx, fails, func(_ context.Context, err error) error { return err })) + + // With one fallback parked, a second concurrent fallback is rejected + inFallback := make(chan struct{}) + release := make(chan struct{}) + parked := make(chan error, 1) + go func() { + parked <- c.Execute(ctx, fails, func(context.Context, error) error { + close(inFallback) + <-release + return nil + }) + }() + <-inFallback + require.Error(t, c.Execute(ctx, fails, recovers)) + close(release) + require.NoError(t, <-parked) + + var out struct { + Name string `json:"name"` + IsOpen bool `json:"is_open"` + RunMetrics []map[string]json.RawMessage `json:"run_metrics"` + FallbackMetrics []map[string]int64 `json:"fallback_metrics"` + } + require.NoError(t, json.Unmarshal([]byte(c.Var().String()), &out), c.Var().String()) + require.Equal(t, "var-rolling", out.Name) + require.False(t, out.IsOpen) + require.Len(t, out.RunMetrics, 1) + for _, k := range []string{"Successes", "ErrFailures", "Latencies"} { + require.Contains(t, out.RunMetrics[0], k) + } + require.Equal(t, []map[string]int64{{ + "Successes": 2, + "ErrFailures": 1, + "ErrConcurrencyLimitRejects": 1, + }}, out.FallbackMetrics) + + // The manager's Var nests the same document under the circuit name + var all map[string]struct { + Name string `json:"name"` + } + require.NoError(t, json.Unmarshal([]byte(m.Var().String()), &all)) + require.Equal(t, "var-rolling", all["var-rolling"].Name) +} diff --git a/faststats/atomic_test.go b/faststats/atomic_test.go index 2fed8a4..05628de 100644 --- a/faststats/atomic_test.go +++ b/faststats/atomic_test.go @@ -61,3 +61,22 @@ func TestAtomicBoolean(t *testing.T) { t.Error("Value not stored in correctly") } } + +func TestAtomicUnmarshalJSONInvalid(t *testing.T) { + var i AtomicInt64 + i.Set(7) + if err := i.UnmarshalJSON([]byte(`"nope"`)); err == nil { + t.Fatal("expected an error") + } + if i.Get() != 7 { + t.Fatal("failed unmarshal must not change the value") + } + var b AtomicBoolean + b.Set(true) + if err := b.UnmarshalJSON([]byte(`3`)); err == nil { + t.Fatal("expected an error") + } + if !b.Get() { + t.Fatal("failed unmarshal must not change the value") + } +} diff --git a/faststats/rolling_counter_test.go b/faststats/rolling_counter_test.go index c187831..b9ff485 100644 --- a/faststats/rolling_counter_test.go +++ b/faststats/rolling_counter_test.go @@ -425,3 +425,12 @@ func TestRollingCounter_MoveForward(t *testing.T) { t.Errorf("Should see a sum of 1 after advancing past all the buckets, saw %d", s) } } + +func TestRollingCounter_RollingSum(t *testing.T) { + r := NewRollingCounter(time.Hour, 10, time.Now()) + r.Inc(time.Now()) + r.Inc(time.Now()) + if s := r.RollingSum(); s != 2 { + t.Fatalf("expected rolling sum of 2, got %d", s) + } +} diff --git a/internal/evar/evar_test.go b/internal/evar/evar_test.go index a5624aa..3c31bdb 100644 --- a/internal/evar/evar_test.go +++ b/internal/evar/evar_test.go @@ -55,8 +55,17 @@ func TestExpvarToVal(t *testing.T) { if ExpvarToVal(nil) != nil { t.Error("Expected nil for nil Var") } + + // A misbehaving Var whose String() is not JSON is passed through as a plain string + if s, ok := ExpvarToVal(notJSONVar{}).(string); !ok || s != "not json" { + t.Errorf("Expected plain string fallback, got %T %v", ExpvarToVal(notJSONVar{}), ExpvarToVal(notJSONVar{})) + } } +type notJSONVar struct{} + +func (notJSONVar) String() string { return "not json" } + func TestForExpvar(t *testing.T) { // Test with an object that has Var() mock := &mockExpvar{val: "test-value"} diff --git a/metriceventstream/metriceventstream_test.go b/metriceventstream/metriceventstream_test.go index 6db156d..fd3f1c4 100644 --- a/metriceventstream/metriceventstream_test.go +++ b/metriceventstream/metriceventstream_test.go @@ -2,12 +2,15 @@ package metriceventstream import ( "context" + "encoding/json" + "net/http" "net/http/httptest" "strings" "testing" "time" "github.com/cep21/circuit/v4" + "github.com/cep21/circuit/v4/closers/hystrix" ) func TestMetricEventStream_DoubleClose(t *testing.T) { @@ -57,3 +60,78 @@ func TestMetricEventStream(t *testing.T) { // And finally wait for start to end <-eventStreamStartResult } + +func TestMetricEventStream_HystrixProperties(t *testing.T) { + h := &circuit.Manager{} + h.MustCreateCircuit("plain", circuit.Config{}) + h.MustCreateCircuit("hystrix", circuit.Config{ + General: circuit.GeneralConfig{ + ClosedToOpenFactory: hystrix.OpenerFactory(hystrix.ConfigureOpener{ + ErrorThresholdPercentage: 37, + RequestVolumeThreshold: 11, + }), + OpenToClosedFactory: hystrix.CloserFactory(hystrix.ConfigureCloser{ + SleepWindow: 1234 * time.Millisecond, + }), + }, + }) + eventStream := MetricEventStream{Manager: h, TickDuration: time.Millisecond} + startResult := make(chan error) + go func() { startResult <- eventStream.Start() }() + + recorder := httptest.NewRecorder() + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + eventStream.ServeHTTP(recorder, httptest.NewRequest("GET", "/hystrix.stream", nil).WithContext(ctx)) + if err := eventStream.Close(); err != nil { + t.Fatal(err) + } + <-startResult + + seen := map[string]streamCmdMetric{} + for _, line := range strings.Split(recorder.Body.String(), "\n") { + if !strings.HasPrefix(line, "data:") { + continue + } + var m streamCmdMetric + if err := json.Unmarshal([]byte(strings.TrimPrefix(line, "data:")), &m); err != nil { + t.Fatalf("%q: %v", line, err) + } + seen[m.Name] = m + } + hm, ok := seen["hystrix"] + if !ok { + t.Fatalf("never saw the hystrix circuit in %q", recorder.Body.String()) + } + if hm.CircuitBreakerErrorThresholdPercent != 37 || hm.CircuitBreakerRequestVolumeThreshold != 11 || hm.CircuitBreakerSleepWindow != 1234 { + t.Errorf("hystrix properties not reported: %+v", hm) + } + pm, ok := seen["plain"] + if !ok { + t.Fatalf("never saw the plain circuit in %q", recorder.Body.String()) + } + if pm.CircuitBreakerErrorThresholdPercent != 0 || pm.CircuitBreakerRequestVolumeThreshold != 0 || pm.CircuitBreakerSleepWindow != 0 { + t.Errorf("plain circuit should not report hystrix properties: %+v", pm) + } +} + +// nonFlushingWriter hides the Flush method of the underlying ResponseWriter +type nonFlushingWriter struct { + header http.Header + code int + body strings.Builder +} + +func (n *nonFlushingWriter) Header() http.Header { return n.header } +func (n *nonFlushingWriter) Write(b []byte) (int, error) { return n.body.Write(b) } +func (n *nonFlushingWriter) WriteHeader(statusCode int) { n.code = statusCode } + +func TestMetricEventStream_RequiresFlusher(t *testing.T) { + eventStream := MetricEventStream{Manager: &circuit.Manager{}} + defer func() { _ = eventStream.Close() }() + rw := &nonFlushingWriter{header: http.Header{}} + eventStream.ServeHTTP(rw, httptest.NewRequest("GET", "/hystrix.stream", nil)) + if rw.code != http.StatusInternalServerError { + t.Fatalf("expected a 500 for a writer that cannot flush, got %d: %s", rw.code, rw.body.String()) + } +} diff --git a/metrics/responsetimeslo/responsetime_test.go b/metrics/responsetimeslo/responsetime_test.go index e6e781f..78ae7b0 100644 --- a/metrics/responsetimeslo/responsetime_test.go +++ b/metrics/responsetimeslo/responsetime_test.go @@ -2,8 +2,11 @@ package responsetimeslo import ( "context" + "errors" "testing" "time" + + "github.com/cep21/circuit/v4" ) func checkSLO(t *testing.T, r *Tracker, expectFail int64, expectPass int64) { @@ -43,3 +46,76 @@ func TestTracker(t *testing.T) { } } + +type countingCollector struct { + passed, failed int +} + +func (c *countingCollector) Failed() { c.failed++ } +func (c *countingCollector) Passed() { c.passed++ } + +func TestFactory(t *testing.T) { + collectors := map[string]*countingCollector{} + f := Factory{ + Config: Config{MaximumHealthyTime: time.Hour}, + ConfigConstructor: []func(string) Config{ + func(name string) Config { + if name == "strict" { + return Config{MaximumHealthyTime: time.Nanosecond} + } + return Config{} + }, + }, + CollectorConstructors: []func(string) Collector{ + func(name string) Collector { + c := &countingCollector{} + collectors[name] = c + return c + }, + }, + } + m := circuit.Manager{DefaultCircuitProperties: []circuit.CommandPropertiesConstructor{f.CommandProperties}} + relaxed := m.MustCreateCircuit("relaxed") + strict := m.MustCreateCircuit("strict") + ctx := context.Background() + slow := func(context.Context) error { + time.Sleep(time.Millisecond) + return nil + } + if err := relaxed.Execute(ctx, slow, nil); err != nil { + t.Fatal(err) + } + if err := strict.Execute(ctx, slow, nil); err != nil { + t.Fatal(err) + } + if err := relaxed.Execute(ctx, func(context.Context) error { return errors.New("boom") }, nil); err == nil { + t.Fatal("expected an error") + } + if collectors["relaxed"].passed != 1 || collectors["relaxed"].failed != 1 { + t.Errorf("relaxed collector: %+v", collectors["relaxed"]) + } + if collectors["strict"].passed != 0 || collectors["strict"].failed != 1 { + t.Errorf("strict collector: %+v", collectors["strict"]) + } + // The per-circuit constructor beats Factory.Config, which beats the package default + var strictTracker, relaxedTracker *Tracker + for _, rm := range strict.CmdMetricCollector { + if tr, ok := rm.(*Tracker); ok { + strictTracker = tr + } + } + for _, rm := range relaxed.CmdMetricCollector { + if tr, ok := rm.(*Tracker); ok { + relaxedTracker = tr + } + } + if strictTracker == nil || relaxedTracker == nil { + t.Fatal("expected a Tracker on each circuit") + } + if strictTracker.Config().MaximumHealthyTime != time.Nanosecond { + t.Errorf("strict config: %v", strictTracker.Config()) + } + if relaxedTracker.Config().MaximumHealthyTime != time.Hour { + t.Errorf("relaxed config: %v", relaxedTracker.Config()) + } +} diff --git a/metrics/rolling/rolling_test.go b/metrics/rolling/rolling_test.go index 99e9d09..7fc9485 100644 --- a/metrics/rolling/rolling_test.go +++ b/metrics/rolling/rolling_test.go @@ -2,6 +2,7 @@ package rolling import ( "context" + "encoding/json" "errors" "strings" "testing" @@ -220,3 +221,60 @@ func TestRunStats_ErrorPercentage(t *testing.T) { t.Errorf("Expect all errors") } } + +func TestFallbackStats_Var(t *testing.T) { + ctx := context.Background() + now := time.Now() + var r FallbackStats + r.SetConfigNotThreadSafe(defaultFallbackStatsConfig) + r.Success(ctx, now, time.Millisecond) + r.Success(ctx, now, time.Millisecond) + r.ErrFailure(ctx, now, time.Millisecond) + r.ErrConcurrencyLimitReject(ctx, now) + var out map[string]int64 + if err := json.Unmarshal([]byte(r.Var().String()), &out); err != nil { + t.Fatal(err) + } + expected := map[string]int64{"Successes": 2, "ErrFailures": 1, "ErrConcurrencyLimitRejects": 1} + for k, v := range expected { + if out[k] != v { + t.Errorf("%s: expected %d got %d (%v)", k, v, out[k], out) + } + } + if len(out) != len(expected) { + t.Errorf("unexpected keys in %v", out) + } +} + +func TestSetConfigNotThreadSafe_NegativeValues(t *testing.T) { + ctx := context.Background() + var r RunStats + r.SetConfigNotThreadSafe(RunStatsConfig{ + RollingStatsDuration: -1, + RollingStatsNumBuckets: -1, + RollingPercentileDuration: -1, + RollingPercentileNumBuckets: -1, + RollingPercentileBucketSize: -1, + }) + got, expected := r.Config(), defaultRunStatsConfig + if got.RollingStatsDuration != expected.RollingStatsDuration || + got.RollingStatsNumBuckets != expected.RollingStatsNumBuckets || + got.RollingPercentileDuration != expected.RollingPercentileDuration || + got.RollingPercentileNumBuckets != expected.RollingPercentileNumBuckets || + got.RollingPercentileBucketSize != expected.RollingPercentileBucketSize { + t.Fatalf("negative values should be treated as unset: got %+v want %+v", got, expected) + } + now := time.Now() + r.Success(ctx, now, time.Millisecond) + if r.Successes.RollingSumAt(now) != 1 { + t.Fatal("expected a success") + } + + var f FallbackStats + f.SetConfigNotThreadSafe(FallbackStatsConfig{RollingStatsDuration: -1, RollingStatsNumBuckets: -1}) + now = time.Now() + f.Success(ctx, now, time.Millisecond) + if f.Successes.RollingSumAt(now) != 1 { + t.Fatal("expected a fallback success") + } +} From 92d3ea4861244ba6c4cc4eb728c5b945ccfcc5c7 Mon Sep 17 00:00:00 2001 From: Jack Lindamood Date: Tue, 18 Aug 2026 11:59:24 +0000 Subject: [PATCH 2/3] test: avoid repeated string literals flagged by goconst 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 --- config_test.go | 4 ++-- metrics/rolling/rolling_test.go | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/config_test.go b/config_test.go index fe288a4..4ed7ea1 100644 --- a/config_test.go +++ b/config_test.go @@ -102,8 +102,8 @@ func TestGeneralConfig_MergeCustomConfig(t *testing.T) { t.Run("receiver keys win", func(t *testing.T) { cfg := GeneralConfig{CustomConfig: map[interface{}]interface{}{"a": "mine"}} - cfg.merge(GeneralConfig{CustomConfig: map[interface{}]interface{}{"a": "theirs", "b": "theirs"}}) - assert.Equal(t, map[interface{}]interface{}{"a": "mine", "b": "theirs"}, cfg.CustomConfig) + cfg.merge(GeneralConfig{CustomConfig: map[interface{}]interface{}{"a": "other-a", "b": "other-b"}}) + assert.Equal(t, map[interface{}]interface{}{"a": "mine", "b": "other-b"}, cfg.CustomConfig) }) t.Run("Config.Merge carries CustomConfig", func(t *testing.T) { diff --git a/metrics/rolling/rolling_test.go b/metrics/rolling/rolling_test.go index 7fc9485..a6d13fb 100644 --- a/metrics/rolling/rolling_test.go +++ b/metrics/rolling/rolling_test.go @@ -231,18 +231,18 @@ func TestFallbackStats_Var(t *testing.T) { r.Success(ctx, now, time.Millisecond) r.ErrFailure(ctx, now, time.Millisecond) r.ErrConcurrencyLimitReject(ctx, now) - var out map[string]int64 - if err := json.Unmarshal([]byte(r.Var().String()), &out); err != nil { + var out struct { + Successes int64 + ErrFailures int64 + ErrConcurrencyLimitRejects int64 + } + dec := json.NewDecoder(strings.NewReader(r.Var().String())) + dec.DisallowUnknownFields() + if err := dec.Decode(&out); err != nil { t.Fatal(err) } - expected := map[string]int64{"Successes": 2, "ErrFailures": 1, "ErrConcurrencyLimitRejects": 1} - for k, v := range expected { - if out[k] != v { - t.Errorf("%s: expected %d got %d (%v)", k, v, out[k], out) - } - } - if len(out) != len(expected) { - t.Errorf("unexpected keys in %v", out) + if out.Successes != 2 || out.ErrFailures != 1 || out.ErrConcurrencyLimitRejects != 1 { + t.Errorf("unexpected fallback stats: %+v", out) } } From ee0260ed08d15d20956e6b5fcd0e5fcdcf53e253 Mon Sep 17 00:00:00 2001 From: Jack Lindamood Date: Tue, 18 Aug 2026 12:01:09 +0000 Subject: [PATCH 3/3] ci: pin golangci-lint version and let Renovate manage it 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 --- .github/workflows/build.yml | 7 +++++++ README.md | 2 +- renovate.json | 3 ++- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 174b8f9..6230025 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -12,6 +12,11 @@ on: push: pull_request: +env: + # Pinned so new lint rules arrive via a dependency update PR rather than breaking unrelated builds. + # renovate: datasource=github-releases depName=golangci/golangci-lint + GOLANGCI_LINT_VERSION: v2.12.2 + jobs: test: strategy: @@ -35,6 +40,8 @@ jobs: run: make test-race - name: golangci-lint uses: golangci/golangci-lint-action@v9 + with: + version: ${{ env.GOLANGCI_LINT_VERSION }} - name: Output coverage run: go test -v -covermode=count -coverprofile=coverage.out ./... - name: upload coverage diff --git a/README.md b/README.md index af67575..fc733e4 100644 --- a/README.md +++ b/README.md @@ -458,7 +458,7 @@ make fuzz # active fuzzing (FUZZTIME per target, default 30s) make help # full target list ``` -`make ci` mirrors the GitHub Actions workflow: build, `go test -race -count 10`, and `golangci-lint run`. If it passes locally, CI should pass. +`make ci` mirrors the GitHub Actions workflow: build, `go test -race -count 10`, and `golangci-lint run`. If it passes locally, CI should pass. CI pins the golangci-lint version (`GOLANGCI_LINT_VERSION` in `.github/workflows/build.yml`); use the same version locally if results differ. # [Example](https://github.com/cep21/circuit/blob/master/example/main.go) diff --git a/renovate.json b/renovate.json index 5db72dd..bf0678c 100644 --- a/renovate.json +++ b/renovate.json @@ -1,6 +1,7 @@ { "$schema": "https://docs.renovatebot.com/renovate-schema.json", "extends": [ - "config:recommended" + "config:recommended", + "customManagers:githubActionsVersions" ] }