From 3312d8fcede5a5cc8479e9e89416870f614392e3 Mon Sep 17 00:00:00 2001 From: Terry Tata Date: Sat, 5 Sep 2026 12:01:38 -0700 Subject: [PATCH 1/2] test(verifier): replace recordingChecker with a mockery mock --- .mockery.yaml | 12 +++ verifier/pkg/commit/policy_hook_test.go | 32 +++----- verifier/pkg/policy/mocks/mock_Checker.go | 95 +++++++++++++++++++++++ 3 files changed, 118 insertions(+), 21 deletions(-) create mode 100644 verifier/pkg/policy/mocks/mock_Checker.go diff --git a/.mockery.yaml b/.mockery.yaml index 634a5d46c..12cee992d 100644 --- a/.mockery.yaml +++ b/.mockery.yaml @@ -121,3 +121,15 @@ packages: configs: - dir: ./verifier/internal/mocks outpkg: mocks + + # The Checker mock gets its own package rather than the flat verifier/internal/mocks, because + # both packages that would import it are in-package tests that the flat one cannot serve: + # gate_test.go is package policy, and a mocks package importing policy is a cycle for it; and + # verifier/pkg/commit reaches commit again through the flat package's cctp mock. Its own + # directory imports only policy, so neither cycle exists. + github.com/smartcontractkit/chainlink-ccv/verifier/pkg/policy: + interfaces: + Checker: + configs: + - dir: ./verifier/pkg/policy/mocks + outpkg: mocks diff --git a/verifier/pkg/commit/policy_hook_test.go b/verifier/pkg/commit/policy_hook_test.go index 8b4dd1082..22b480753 100644 --- a/verifier/pkg/commit/policy_hook_test.go +++ b/verifier/pkg/commit/policy_hook_test.go @@ -6,11 +6,13 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/smartcontractkit/chainlink-ccv/protocol" "github.com/smartcontractkit/chainlink-ccv/verifier/pkg/monitoring" "github.com/smartcontractkit/chainlink-ccv/verifier/pkg/policy" + "github.com/smartcontractkit/chainlink-ccv/verifier/pkg/policy/mocks" verifier "github.com/smartcontractkit/chainlink-ccv/verifier/pkg/vtypes" "github.com/smartcontractkit/chainlink-common/pkg/logger" ) @@ -114,19 +116,6 @@ type failingSigner struct { func (s failingSigner) Sign([]byte) ([]byte, error) { return nil, s.err } -// recordingChecker answers every message with the same verdict and counts the calls, so a test -// can tell "the endpoint passed it and the verifier still refused" apart from "the endpoint was -// never asked". -type recordingChecker struct { - verdict policy.Verdict - calls int -} - -func (c *recordingChecker) Evaluate(context.Context, policy.EvaluateRequest) (policy.Verdict, error) { - c.calls++ - return c.verdict, nil -} - // TestPolicyHook_PassCannotBypassVerification is the answer to "are the policy checks purely // additive": they are. A PASS is not an instruction to sign. It only lets the task reach the // commit verifier, which then applies every check it would have applied with no hook configured. @@ -158,7 +147,12 @@ func TestPolicyHook_PassCannotBypassVerification(t *testing.T) { logger.Test(t), monitoring.NewFakeVerifierMonitoring()) require.NoError(t, err) - checker := &recordingChecker{verdict: policy.Verdict{Decision: policy.DecisionPass}} + // The endpoint must have been asked exactly once and answered PASS, or this test proves + // nothing about what a PASS can do: .Once() plus the mock's cleanup assertion pins both + // halves of that. + checker := mocks.NewMockChecker(t) + checker.EXPECT().Evaluate(mock.Anything, mock.Anything). + Return(policy.Verdict{Decision: policy.DecisionPass}, nil).Once() gated, err := policy.NewGatedVerifier( logger.Test(t), "committee-verifier-1", cv, checker, monitoring.NewFakeVerifierMonitoring(), 0, @@ -168,9 +162,6 @@ func TestPolicyHook_PassCannotBypassVerification(t *testing.T) { task := newVerifiableTask(t, configuredSourceChain, destChain, addr, verifierBlob, executorAddr) results := gated.VerifyMessages(t.Context(), []verifier.VerificationTask{task}) - require.Equal(t, 1, checker.calls, - "the endpoint must have been asked and answered PASS, or this proves nothing about what a PASS can do") - require.Len(t, results, 1) assert.Nil(t, results[0].Result, "a PASS must not produce a signature the verifier withheld") require.NotNil(t, results[0].Error) @@ -191,7 +182,9 @@ func TestPolicyHook_PassCannotBypassVerification(t *testing.T) { cv, err := NewCommitVerifier(config, addr, signer, logger.Test(t), monitoring.NewFakeVerifierMonitoring()) require.NoError(t, err) - checker := &recordingChecker{verdict: policy.Verdict{Decision: policy.DecisionPass}} + // No expectation is set on the mock: the mock fails the test if Evaluate is called at + // all, which is how the endpoint skip stays asserted rather than assumed. + checker := mocks.NewMockChecker(t) gated, err := policy.NewGatedVerifier( logger.Test(t), "committee-verifier-1", cv, checker, monitoring.NewFakeVerifierMonitoring(), 0, @@ -201,9 +194,6 @@ func TestPolicyHook_PassCannotBypassVerification(t *testing.T) { task := newVerifiableTask(t, unconfiguredSourceChain, destChain, addr, verifierBlob, executorAddr) results := gated.VerifyMessages(t.Context(), []verifier.VerificationTask{task}) - assert.Zero(t, checker.calls, - "the hook gates messages this verifier would sign, so a task it already rejects costs the operator nothing") - require.Len(t, results, 1) assert.Nil(t, results[0].Result, "a task the verifier rejects is never signed") require.NotNil(t, results[0].Error) diff --git a/verifier/pkg/policy/mocks/mock_Checker.go b/verifier/pkg/policy/mocks/mock_Checker.go new file mode 100644 index 000000000..ff6d0c5ab --- /dev/null +++ b/verifier/pkg/policy/mocks/mock_Checker.go @@ -0,0 +1,95 @@ +// Code generated by mockery v2.53.5. DO NOT EDIT. + +package mocks + +import ( + context "context" + + policy "github.com/smartcontractkit/chainlink-ccv/verifier/pkg/policy" + policyapi "github.com/smartcontractkit/chainlink-ccv/verifier/pkg/policy/internal/policyapi" + mock "github.com/stretchr/testify/mock" +) + +// MockChecker is an autogenerated mock type for the Checker type +type MockChecker struct { + mock.Mock +} + +type MockChecker_Expecter struct { + mock *mock.Mock +} + +func (_m *MockChecker) EXPECT() *MockChecker_Expecter { + return &MockChecker_Expecter{mock: &_m.Mock} +} + +// Evaluate provides a mock function with given fields: ctx, req +func (_m *MockChecker) Evaluate(ctx context.Context, req policyapi.EvaluateRequest) (policy.Verdict, error) { + ret := _m.Called(ctx, req) + + if len(ret) == 0 { + panic("no return value specified for Evaluate") + } + + var r0 policy.Verdict + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, policyapi.EvaluateRequest) (policy.Verdict, error)); ok { + return rf(ctx, req) + } + if rf, ok := ret.Get(0).(func(context.Context, policyapi.EvaluateRequest) policy.Verdict); ok { + r0 = rf(ctx, req) + } else { + r0 = ret.Get(0).(policy.Verdict) + } + + if rf, ok := ret.Get(1).(func(context.Context, policyapi.EvaluateRequest) error); ok { + r1 = rf(ctx, req) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// MockChecker_Evaluate_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Evaluate' +type MockChecker_Evaluate_Call struct { + *mock.Call +} + +// Evaluate is a helper method to define mock.On call +// - ctx context.Context +// - req policyapi.EvaluateRequest +func (_e *MockChecker_Expecter) Evaluate(ctx interface{}, req interface{}) *MockChecker_Evaluate_Call { + return &MockChecker_Evaluate_Call{Call: _e.mock.On("Evaluate", ctx, req)} +} + +func (_c *MockChecker_Evaluate_Call) Run(run func(ctx context.Context, req policyapi.EvaluateRequest)) *MockChecker_Evaluate_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(policyapi.EvaluateRequest)) + }) + return _c +} + +func (_c *MockChecker_Evaluate_Call) Return(_a0 policy.Verdict, _a1 error) *MockChecker_Evaluate_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *MockChecker_Evaluate_Call) RunAndReturn(run func(context.Context, policyapi.EvaluateRequest) (policy.Verdict, error)) *MockChecker_Evaluate_Call { + _c.Call.Return(run) + return _c +} + +// NewMockChecker creates a new instance of MockChecker. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewMockChecker(t interface { + mock.TestingT + Cleanup(func()) +}) *MockChecker { + mock := &MockChecker{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} From 3bb3403d919c5718280bfcc73cebde917d0f39e2 Mon Sep 17 00:00:00 2001 From: Terry Tata Date: Thu, 10 Sep 2026 12:39:40 -0700 Subject: [PATCH 2/2] comments --- .mockery.yaml | 20 +++++++++++++++----- verifier/pkg/commit/policy_hook_test.go | 10 +++++++++- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/.mockery.yaml b/.mockery.yaml index 12cee992d..9ac99fb35 100644 --- a/.mockery.yaml +++ b/.mockery.yaml @@ -122,11 +122,21 @@ packages: - dir: ./verifier/internal/mocks outpkg: mocks - # The Checker mock gets its own package rather than the flat verifier/internal/mocks, because - # both packages that would import it are in-package tests that the flat one cannot serve: - # gate_test.go is package policy, and a mocks package importing policy is a cycle for it; and - # verifier/pkg/commit reaches commit again through the flat package's cctp mock. Its own - # directory imports only policy, so neither cycle exists. + # The Checker mock gets its own package rather than the flat verifier/internal/mocks because + # its consumer, verifier/pkg/commit, cannot import the flat one: that package holds the cctp + # attestation mock, so it depends on verifier/pkg/token/cctp, which depends on commit. A + # commit test importing it closes that loop. This directory imports only policy, so it does + # not. + # + # It cannot serve verifier/pkg/policy's own tests either: gate_test.go is package policy, and + # any mocks package importing policy is a cycle for an in-package test. Those tests use the + # hand-written stubChecker in gate_test.go and should keep doing so. + # + # The generated signature names policy/internal/policyapi, which is where the request type is + # defined. That is not a barrier for callers: policy.EvaluateRequest is a true alias for it, + # so a commit test writes Run(func(_ context.Context, req policy.EvaluateRequest){...}) and it + # type-checks. Rewriting it with mockery's replace-type does not work, because the target is + # an alias and mockery emits an empty type name for it. github.com/smartcontractkit/chainlink-ccv/verifier/pkg/policy: interfaces: Checker: diff --git a/verifier/pkg/commit/policy_hook_test.go b/verifier/pkg/commit/policy_hook_test.go index 22b480753..8941a381c 100644 --- a/verifier/pkg/commit/policy_hook_test.go +++ b/verifier/pkg/commit/policy_hook_test.go @@ -150,8 +150,17 @@ func TestPolicyHook_PassCannotBypassVerification(t *testing.T) { // The endpoint must have been asked exactly once and answered PASS, or this test proves // nothing about what a PASS can do: .Once() plus the mock's cleanup assertion pins both // halves of that. + task := newVerifiableTask(t, configuredSourceChain, destChain, addr, verifierBlob, executorAddr) + checker := mocks.NewMockChecker(t) checker.EXPECT().Evaluate(mock.Anything, mock.Anything). + // Typed rather than untyped mock.Arguments. The generated signature names + // policy/internal/policyapi, which this package cannot import, but + // policy.EvaluateRequest is a true alias for that type, so naming it here + // type-checks. It also pins that the gate asked about the task in hand. + Run(func(_ context.Context, req policy.EvaluateRequest) { + assert.Equal(t, task.MessageID, req.MessageId) + }). Return(policy.Verdict{Decision: policy.DecisionPass}, nil).Once() gated, err := policy.NewGatedVerifier( logger.Test(t), "committee-verifier-1", cv, checker, @@ -159,7 +168,6 @@ func TestPolicyHook_PassCannotBypassVerification(t *testing.T) { ) require.NoError(t, err) - task := newVerifiableTask(t, configuredSourceChain, destChain, addr, verifierBlob, executorAddr) results := gated.VerifyMessages(t.Context(), []verifier.VerificationTask{task}) require.Len(t, results, 1)