Conversation
product-security-plaid-production
Bot
requested review from
pavel-raykov and
timothyF95
|
👋 kalverra, 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
Risk Rating: LOW (test-only changes; no production logic touched)
Fixes two flaky integration tests by adding retry logic around EVM log filtering where the simulated backend may not surface events immediately.
Changes:
- Add
require.Eventuallyretry loop when filteringRandomWordsRequestedlogs in VRF v2 integration helper logic. - Replace a
gomega.Eventually(...).Should(Equal(...))assertion withrequire.Eventuallyin Flux Monitor hibernation mode integration test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/services/vrf/v2/integration_v2_test.go | Retry log filtering for RandomWordsRequested to reduce flakiness on simulated backend. |
| core/services/fluxmonitorv2/integrations_test.go | Use require.Eventually to retry reading FlagRaised logs to reduce flakiness. |
| require.Eventually(t, func() bool { | ||
| iter, err := coordinator.FilterRandomWordsRequested(nil, nil, []*big.Int{subID}, nil) | ||
| if err != nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
FilterRandomWordsRequested returns a RandomWordsRequestedIterator with a Close() method; inside this require.Eventually loop a new iterator is created on each poll but never closed. This can leak resources/goroutines across retries and make the test flaky. Ensure the iterator is closed on every attempt (including early-return paths), and handle any Close() error appropriately.
| require.Eventually(t, func() bool { | |
| iter, err := coordinator.FilterRandomWordsRequested(nil, nil, []*big.Int{subID}, nil) | |
| if err != nil { | |
| return false | |
| } | |
| require.Eventually(t, func() (ok bool) { | |
| iter, err := coordinator.FilterRandomWordsRequested(nil, nil, []*big.Int{subID}, nil) | |
| if err != nil { | |
| return false | |
| } | |
| defer func() { | |
| if closeErr := iter.Close(); closeErr != nil { | |
| t.Logf("failed to close RandomWordsRequested iterator: %v", closeErr) | |
| ok = false | |
| } | |
| }() |
| var batch []v22.RandomWordsRequested | ||
| for iter.Next() { | ||
| batch = append(batch, iter.Event()) | ||
| } |
There was a problem hiding this comment.
The iterator's Error() is not checked after the for iter.Next() loop. If iteration stops due to an underlying error, this code can accept a partial batch and proceed as though it succeeded. Check iter.Error() (and treat non-nil as a failed attempt) before assigning events / returning true.
| } | |
| } | |
| if err := iter.Error(); err != nil { | |
| return false | |
| } |
There was a problem hiding this comment.
Ah true error check was missing
|
|
Closed in favor of #22324 |
I tested out the ai skil at #22125 in some simple cases. It found and fixed 2 flakes: