fix(keeper): prevent mock race in Test_RegistrySynchronizer1_2_UpkeepReceivedLog#22216
fix(keeper): prevent mock race in Test_RegistrySynchronizer1_2_UpkeepReceivedLog#22216
Conversation
…ReceivedLog The fullSync goroutine (with getActiveUpkeepIDsTime=2) can consume the getUpkeep mock intended for handleUpkeepReceived. Changing .Once() to .Maybe() ensures the log-handler path always has a valid mock response while preserving the WaitForCount(upkeep_registrations, 2) assertion. Fixes CRE-3895 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
👋 Tofel, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate flakiness in the keeper synchronizer test Test_RegistrySynchronizer1_2_UpkeepReceivedLog by relaxing the getUpkeep contract-call mock expectation so concurrent/overlapping calls don’t exhaust a .Once() expectation.
Changes:
- Update the
getUpkeepmock expectation inTest_RegistrySynchronizer1_2_UpkeepReceivedLogfrom.Once()to.Maybe()to avoid mock-consumption races. - Preserve the primary correctness assertion by still requiring
upkeep_registrationsto reach a count of 2.
|
|
||
| registryMock := cltest.NewContractMockReceiver(t, ethMock, keeper.Registry1_2ABI, contractAddress) | ||
| registryMock.MockResponse("getUpkeep", upkeepConfig1_2).Once() | ||
| registryMock.MockResponse("getUpkeep", upkeepConfig1_2).Maybe() |
There was a problem hiding this comment.
The PR description attributes the flake to the fullSync goroutine making an extra getUpkeep call after WaitForCount(...) returns, but this test setup uses SyncInterval = 1000*time.Hour (and the resettable timer isn’t initially started) so a second scheduled full sync shouldn’t be able to run during the test. Consider updating the PR description to reflect the actual source of the extra getUpkeep call (or the current test timing) so future readers aren’t misled.
|
|
||
| registryMock := cltest.NewContractMockReceiver(t, ethMock, keeper.Registry1_2ABI, contractAddress) | ||
| registryMock.MockResponse("getUpkeep", upkeepConfig1_2).Once() | ||
| registryMock.MockResponse("getUpkeep", upkeepConfig1_2).Maybe() |
There was a problem hiding this comment.
Switching this mock to .Maybe() removes any upper/lower bound on how many getUpkeep RPCs are made during the test, which can mask regressions (e.g., unexpected repeated getUpkeep calls). If the goal is to avoid the race while keeping the assertion tight, consider using a bounded expectation (e.g., .Times(n) if the max is known) or a more specific matcher via MockMatchedResponse so only the UpkeepReceived-triggered call is constrained.
| registryMock.MockResponse("getUpkeep", upkeepConfig1_2).Maybe() | |
| registryMock.MockResponse("getUpkeep", upkeepConfig1_2).Once() |
|
Summary
Test_RegistrySynchronizer1_2_UpkeepReceivedLogincore/services/keeper/(CRE-3895)registryMock.MockResponse("getUpkeep", upkeepConfig1_2).Once()→.Maybe()atregistry1_2_synchronizer_test.go:537fullSyncgoroutine (configured withgetActiveUpkeepIDsTime=2) can make a secondgetUpkeepcall afterWaitForCount(upkeep_registrations, 1)returns, consuming the.Once()mock intended for the log handler — leavinghandleUpkeepReceivedwithout a response and causingWaitForCount(upkeep_registrations, 2)to time out.Maybe()makes the mock a permanent fallback; theWaitForCount(..., 2)assertion still fully validates correctnessTest plan
Test_RegistrySynchronizer1_2_UpkeepReceivedLogwith-race -shuffle=on(requiresCL_DATABASE_URL— not runnable locally without a full DB setup)🤖 Generated with Claude Code