Conversation
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM — this change effectively disables two E2E subtests, which can hide regressions unless there’s an alternative signal/coverage.
This PR aims to reduce CI flakiness by skipping VRF BHS “full” E2E test paths in the devenv test suite.
Changes:
- Skip the VRFv2 BHS full E2E subtest that waits ~256 blocks then funds and expects fulfillment.
- Skip the VRFv2Plus BHS full E2E subtest that waits ~256 blocks then funds and expects fulfillment.
Targeted areas for scrupulous human review:
- Whether the skips should be CI-only (or opt-in) so the E2E scenarios remain runnable for debugging/regression verification.
- Whether the PR description should be updated to remove/reflect the LogPoller runner change claim.
Suggested reviewers (per CODEOWNERS): @smartcontractkit/devex-tooling (for /devenv/)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| devenv/tests/vrfv2plus/bhs_test.go | Unconditionally skips the “complete E2E fund later and fulfill” subtest. |
| devenv/tests/vrfv2/bhs_test.go | Unconditionally skips the “complete E2E” subtest that waits 256 blocks then funds. |
| }) | ||
|
|
||
| t.Run("BHS complete E2E fund later and fulfill", func(t *testing.T) { | ||
| t.Skip("This test is flaky on CI. Originally it only run on live testnets. Owners should work on fixing it.") |
There was a problem hiding this comment.
Skip message grammar: "Originally it only run on live testnets" should be "Originally it only ran on live testnets" (or similar).
| t.Skip("This test is flaky on CI. Originally it only run on live testnets. Owners should work on fixing it.") | |
| t.Skip("This test is flaky on CI. Originally it only ran on live testnets. Owners should work on fixing it.") |
| t.Skip("This test is flaky on CI. Originally it only run on live testnets. Owners should work on fixing it.") | ||
| // Failure reason: blockhash not found in the store. |
There was a problem hiding this comment.
t.Skip is unconditional, but the message says the test is flaky on CI. As written, it will also skip for local/manual runs, which prevents running this E2E scenario when debugging. Consider gating the skip on a CI signal (e.g., os.Getenv("CI")) or requiring an explicit env var to enable the long/flaky path.
| require.NoError(t, err, "failed to connect to Chainlink nodes") | ||
|
|
||
| t.Run("BHS Job with complete E2E - wait 256 blocks to see if Rand Request is fulfilled", func(t *testing.T) { | ||
| t.Skip("This test is flaky on CI. Originally it only run on live testnets. Owners should work on fixing it.") |
There was a problem hiding this comment.
Skip message grammar: "Originally it only run on live testnets" should be "Originally it only ran on live testnets" (or similar).
| t.Skip("This test is flaky on CI. Originally it only run on live testnets. Owners should work on fixing it.") | |
| t.Skip("This test is flaky on CI. Originally it only ran on live testnets. Owners should work on fixing it.") |
| t.Run("BHS Job with complete E2E - wait 256 blocks to see if Rand Request is fulfilled", func(t *testing.T) { | ||
| t.Skip("This test is flaky on CI. Originally it only run on live testnets. Owners should work on fixing it.") |
There was a problem hiding this comment.
PR description mentions "use a stronger runner for LogPoller tests" but this PR only changes VRF BHS tests. If LogPoller changes are no longer part of this PR, please update the description (or include the missing changes) to avoid confusion for reviewers/release notes.
| t.Skip("This test is flaky on CI. Originally it only run on live testnets. Owners should work on fixing it.") | ||
| // Failure reason: blockhash not found in the store. |
There was a problem hiding this comment.
t.Skip is unconditional, but the message says the test is flaky on CI. As written, it will also skip for local/manual runs (including on live testnets), which makes it hard to reproduce or debug. Consider gating the skip on a CI signal (e.g., os.Getenv("CI")/GITHUB_ACTIONS) or using an explicit opt-in env var to run the E2E path.
|
| }) | ||
|
|
||
| t.Run("BHS complete E2E fund later and fulfill", func(t *testing.T) { | ||
| t.Skip("This test is flaky on CI. Originally it only run on live testnets. Owners should work on fixing it.") |
There was a problem hiding this comment.
Can we just mark this as flaky in Trunk instead of direct skip?
There was a problem hiding this comment.
why would we do that? original was also skipped.
Why? These tests were flaky and originals weren't running in CI anyway.
Plus use a stronger runner for LogPoller tests to make sure CL node doesn't crash at the end.