diff --git a/breaker_test.go b/breaker_test.go index b8192d6..3a4f9c0 100644 --- a/breaker_test.go +++ b/breaker_test.go @@ -167,9 +167,7 @@ func TestBreaker_Observe_State(t *testing.T) { }, } for _, tt := range tests { - tt := tt for bName, b := range tt.breakers { - b := b t.Run(bName+": "+tt.name, func(t *testing.T) { t.Parallel() diff --git a/hoglet.go b/hoglet.go index 4aa0d8f..1c41742 100644 --- a/hoglet.go +++ b/hoglet.go @@ -236,7 +236,7 @@ func Wrap[IN, OUT any](c *Circuit, f WrappableFunc[IN, OUT]) WrappableFunc[IN, O obs.Observe(true) panic(err) // let the caller deal with panics } - obs.Observe(c.options.isFailure(err)) + obs.Observe(err != nil && c.options.isFailure(err)) }() return f(ctx, in) @@ -257,7 +257,7 @@ func (c *Circuit) observeCtx(obs Observer, ctx context.Context) { if context.Cause(ctx) == errWrappedFunctionDone { err = nil // ignore internal cancellations; the wrapped function returned already } - obs.Observe(c.options.isFailure(err)) + obs.Observe(err != nil && c.options.isFailure(err)) } // State represents the state of a circuit. diff --git a/hoglet_test.go b/hoglet_test.go index e445a72..315c5e6 100644 --- a/hoglet_test.go +++ b/hoglet_test.go @@ -10,7 +10,7 @@ import ( "github.com/stretchr/testify/require" ) -var sentinel = errors.New("sentinel error") +var errSentinel = errors.New("sentinel error") type noopIn int @@ -26,7 +26,7 @@ func noop(ctx context.Context, in noopIn) (struct{}, error) { case noopInSuccess: return struct{}{}, nil case noopInFailure: - return struct{}{}, sentinel + return struct{}{}, errSentinel default: // noopInPanic panic("boom") } @@ -80,9 +80,9 @@ func TestBreaker_nil_breaker_does_not_open(t *testing.T) { b, err := NewCircuit(nil) require.NoError(t, err) _, err = Wrap(b, noop)(t.Context(), noopInFailure) - assert.Equal(t, sentinel, err) + assert.Equal(t, errSentinel, err) _, err = Wrap(b, noop)(t.Context(), noopInFailure) - assert.Equal(t, sentinel, err) + assert.Equal(t, errSentinel, err) } func TestBreaker_ctx_parameter_not_cancelled(t *testing.T) { @@ -152,7 +152,7 @@ func TestHoglet_Do(t *testing.T) { name: "error opens", calls: []calls{ {arg: noopInSuccess, wantErr: nil}, - {arg: noopInFailure, wantErr: sentinel}, + {arg: noopInFailure, wantErr: errSentinel}, {arg: noopInSuccess, wantErr: ErrCircuitOpen}, }, }, @@ -168,7 +168,7 @@ func TestHoglet_Do(t *testing.T) { name: "success on half-open closes", calls: []calls{ {arg: noopInSuccess, wantErr: nil}, - {arg: noopInFailure, wantErr: sentinel}, + {arg: noopInFailure, wantErr: errSentinel}, {arg: noopInSuccess, wantErr: nil, halfOpen: true}, {arg: noopInSuccess, wantErr: nil}, }, @@ -177,8 +177,8 @@ func TestHoglet_Do(t *testing.T) { name: "failure on half-open keeps open", calls: []calls{ {arg: noopInSuccess, wantErr: nil}, - {arg: noopInFailure, wantErr: sentinel}, - {arg: noopInFailure, wantErr: sentinel, halfOpen: true}, + {arg: noopInFailure, wantErr: errSentinel}, + {arg: noopInFailure, wantErr: errSentinel, halfOpen: true}, {arg: noopInSuccess, wantErr: ErrCircuitOpen}, }, }, @@ -207,11 +207,27 @@ func TestHoglet_Do(t *testing.T) { } } +func TestCircuit_failure_condition_never_called_with_nil_error(t *testing.T) { + conditionCalled := false + condition := func(err error) bool { + conditionCalled = true + require.NotNil(t, err, "failure condition must not be called with nil error") + return true + } + + b, err := NewCircuit(nil, WithFailureCondition(condition)) + require.NoError(t, err) + + _, err = Wrap(b, noop)(t.Context(), noopInSuccess) + require.NoError(t, err) + assert.False(t, conditionCalled, "failure condition should not have been called for a successful call") +} + // maybeAssertPanic is a test-table helper to assert that a function panics or not, depending on the value of wantPanic. func maybeAssertPanic(t *testing.T, f func(), wantPanic any) { wrapped := assert.NotPanics if wantPanic != nil { - wrapped = func(t assert.TestingT, f assert.PanicTestFunc, msgAndArgs ...interface{}) bool { + wrapped = func(t assert.TestingT, f assert.PanicTestFunc, msgAndArgs ...any) bool { return assert.PanicsWithValue(t, wantPanic, f, msgAndArgs...) } } diff --git a/options.go b/options.go index d8cba1d..a530363 100644 --- a/options.go +++ b/options.go @@ -29,9 +29,9 @@ func WithHalfOpenDelay(delay time.Duration) Option { // WithFailureCondition allows specifying a filter function that determines whether an error should open the breaker. // If the provided function returns true, the error is considered a failure and the breaker may open (depending on the // breaker logic). -// The default filter considers all non-nil errors as failures (err != nil). +// Nil errors are always considered successes. The provided function is only called in the non-nil error case. // -// This does not modify the error returned by [Circuit.Call]. It only affects the circuit itself. +// This does not modify the error returned by the wrapped function call. It only affects the circuit itself. func WithFailureCondition(condition func(error) bool) Option { return optionFunc(func(o *options) error { o.isFailure = condition @@ -41,7 +41,7 @@ func WithFailureCondition(condition func(error) bool) Option { // IgnoreContextCanceled is a helper function for [WithFailureCondition] that ignores [context.Canceled] errors. func IgnoreContextCanceled(err error) bool { - return err != nil && !errors.Is(err, context.Canceled) + return !errors.Is(err, context.Canceled) } // WithBreakerMiddleware allows wrapping the [Breaker] via a [BreakerMiddleware].