core/internal/testutils/evmtest: run service to ensure closure#21397
core/internal/testutils/evmtest: run service to ensure closure#21397
Conversation
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
![trunk-io[bot]](https://avatars.githubusercontent.com/in/120106?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🔴 Test Results: Data Race in Test Logger
Affected failures:
- Workflow Run: Core Tests (go_core_race_tests)
What Broke
The addition of servicetest.Run(t, c) in core/internal/testutils/evmtest/evmtest.go introduced a data race in the test logger.
Proposed Fixes
Address the data race in the test logger by running each chain service within its own subtest, providing an isolated testing.TB instance for logging, and ensure proper service closure by collecting and deferring the execution of closer functions.
- for _, c := range cc.Slice() {
- servicetest.Run(t, c)
- }
+ var closers []func()
+ for i, c := range cc.Slice() {
+ chain := c // Capture loop variable for closure
+ t.Run(fmt.Sprintf("chain-%d", i), func(st *testing.T) {
+ closers = append(closers, servicetest.Run(st, chain))
+ })
+ }
+ t.Cleanup(func() {
+ for _, closer := range closers {
+ closer()
+ }
+ })Autofix Options
You can apply the proposed fixes directly to your branch. Try the following:
- Comment
/trunk stack-fix 6qscLsePto generate a stacked PR with the proposed fixes. - Use MCP in your IDE to fix the issue. Try
Help me fix CI failures from 6qscLsePto get started.
Tip
Get Better Results: This CI job is not uploading test reports. Adding structured test reports enables more precise, test-level analysis with better root cause identification and more targeted fix recommendations.
👉🏻 Learn how to upload test results.
There was a problem hiding this comment.
🔴 Test Results: Unrelated Failure
Affected failures:
- TestUpdateRouterRamps/MCMS_disabled (Workflow Run: Core Tests (go_core_ccip_deployment_tests))
- TestUpdateRouterRamps (Workflow Run: Core Tests (go_core_ccip_deployment_tests))
- Workflow Run: Integration Tests
What Broke
These failures appear unrelated to the changes in the PR. Some failures are due to insufficient detail in CI logs, providing only generic 'exit code 1' errors without specific messages or stack traces. Other failures stem from improper service lifecycle management in the test environment, such as simulated EVM chain services not being properly started or managed, or the Chainlink node application not being managed by servicetest.Run, leading to network port resource leaks and 'address already in use' errors.
Autofix Options
You can use our MCP server to get AI assistance with debugging and fixing these failures.
- Use MCP in your IDE to debug the issue. Try
Help me fix CI failures from gUd37lCyto get started.
There was a problem hiding this comment.
🔴 Test Results: EVM Chain Service Setup
Affected tests:
- TestUpdateRouterRamps/MCMS_disabled_but_enforced (Workflow Run: Core Tests (go_core_ccip_deployment_tests))
What Broke
The PR's change to run EVM chains as services inadvertently enables MCMS enforcement in the test environment, leading to a test failure that expects MCMS to be disabled.
Proposed Fixes
Introduce a SkipServiceRun option in evmtest.TestChainOpts and modify evmtest.NewLegacyChains to conditionally skip servicetest.Run. This allows tests expecting MCMS to be disabled to opt out of the service startup that implicitly enables it.
\tTxManager txmgr.TxManager
\tKeyStore keystore.Eth
\tMailMon *mailbox.Monitor
+ \tSkipServiceRun bool
}
// NewLegacyChains returns a simple chain collection with one chain and \trequire.NoError(t, err)
- \tfor _, c := range cc.Slice() {
- \t\tservicetest.Run(t, c)
- \t}
+ \tif (!testopts.SkipServiceRun) {
+ \t\tfor _, c := range cc.Slice() {
+ \t\t\tservicetest.Run(t, c)
+ \t\t}
+ \t}
\treturn cc.NewLegacyChains()
}Autofix Options
You can apply the proposed fixes directly to your branch. Try the following:
- Comment
/trunk stack-fix J9Sw6OTqto generate a stacked PR with the proposed fixes. - Use MCP in your IDE to fix the issue. Try
Help me fix CI failures from J9Sw6OTqto get started.
There was a problem hiding this comment.
🔴 Test Results: EVM Service Lifecycle
Affected failures:
- TestDelegate_ServicesListenerHandleLog/Log_has_wrong_jobID (Workflow Run: Core Tests (go_core_tests))
- Workflow Run: Run CCIP integration In Memory Tests For PR / smoke/ccip/ccip_messaging_test.go:Test_CCIPMessaging_Solana2EVM_LOOPP
- Workflow Run: Run CCIP integration In Memory Tests For PR / smoke/ccip/ccip_token_transfer_test.go:*_LOOPP
What Broke
These failures are caused by incorrect lifecycle management of EVM chain services within evmtest.NewLegacyChains. The explicit running and closing of chain services via servicetest.Run in this utility function occurs prematurely or interferes with test expectations. This leads to various issues, including preventing proper service setup for mocks, causing premature shutdowns and timeouts in integration tests, or blocking tests indefinitely.
Proposed Fixes
Remove the immediate starting and stopping of chain services from evmtest.NewLegacyChains. For tests requiring explicit service management, run services within the specific test setup, such as NewDirectRequestUniverseWithConfig in delegate_test.go.
- for _, c := range cc.Slice() {
- servicetest.Run(t, c);
- } )
+ for _, c := range legacyChains.Slice() {
+ servicetest.Run(t, c);
+ }Autofix Options
You can apply the proposed fixes directly to your branch. Try the following:
- Comment
/trunk stack-fix sAYnNP2Fto generate a stacked PR with the proposed fixes. - Use MCP in your IDE to fix the issue. Try
Help me fix CI failures from sAYnNP2Fto get started.
There was a problem hiding this comment.
🔴 Test Results: Insufficient Client Mocks
Affected tests:
- TestDelegate_ServicesListenerHandleLog/Log_is_a_CancelOracleRequest_with_a_matching_run (Workflow Run: Core Tests (go_core_tests))
- TestDelegate_ServicesListenerHandleLog/Log_is_not_consumed,_as_it's_too_young (Workflow Run: Core Tests (go_core_tests))
- TestDelegate_ServicesListenerHandleLog/Log_is_an_OracleRequest (Workflow Run: Core Tests (go_core_tests))
- TestDelegate_ServicesListenerHandleLog/Log_is_a_CancelOracleRequest_with_no_matching_run (Workflow Run: Core Tests (go_core_tests))
What Broke
These tests fail because the NullClient used in their setup lacks the necessary mock functionality or has incorrect configuration to properly simulate blockchain behavior. Specifically, it fails to provide mocks for ConfiguredChainID and HeadByNumber to allow log listeners to process cancellation events, handle logs, or meet minimum confirmation requirements, leading to timeouts or incorrect test outcomes.
Proposed Fixes
Replace client.NewNullClient with cltest.NewEthMocksWithStartupAssertions and add mocks for ConfiguredChainID and HeadByNumber to provide the necessary mock functionality and simulate blockchain advancements for log processing.
- ethClient := client.NewNullClient(big.NewInt(evmtest.NullClientChainID), logger.TestLogger(t))
+ ethClient := cltest.NewEthMocksWithStartupAssertions(t)
+ ethClient.On("ConfiguredChainID").Return(big.NewInt(evmtest.NullClientChainID)).Maybe()
+ ethClient.On("HeadByNumber", mock.Anything, mock.Anything).Return(cltest.Head(1), nil).Maybe()- ethClient := client.NewNullClient(big.NewInt(evmtest.NullClientChainID), logger.TestLogger(t))
+ var currentBlock int64 = 0
+ ethClient := cltest.NewEthMocksWithStartupAssertions(t)
+ ethClient.On("ConfiguredChainID").Return(big.NewInt(evmtest.NullClientChainID)).Maybe()
+ ethClient.On("HeadByNumber", mock.Anything, mock.Anything).Return(func(_ context.Context, _ *big.Int) (*evmtypes.Head, error) {
+ currentBlock++
+ return cltest.Head(currentBlock), nil
+ }).Maybe()Autofix Options
You can apply the proposed fixes directly to your branch. Try the following:
- Comment
/trunk stack-fix lnM6wVGzto generate a stacked PR with the proposed fixes. - Use MCP in your IDE to fix the issue. Try
Help me fix CI failures from lnM6wVGzto get started.
CORA - Pending Reviewers
Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
| t.Parallel() | ||
| ctx := testutils.Context(t) | ||
| ethClient := cltest.NewEthMocksWithStartupAssertions(t) | ||
| ethClient.On("NonceAt", mock.Anything, mock.Anything, mock.Anything).Return(uint64(0), nil).Once() |
There was a problem hiding this comment.
Every instance of .On("NonceAt" is just returning 0 for anything, so we can register that once in the caller as .Maybe(). We don't count the number of calls anymore, but that is an implementation details anyways.
|
2 participants
This is an attempt to fix a long time leaky test goroutine, that manifests as a race through the test logger from the evm balance monitor
Worker. Everything appears to be wired correctly up to this point, suggesting that a caller is not closing the chains. I checked a few of them, and many do not even Start the chains, so I'm hoping we can just callservicetest.Runfrom one place to make it foolproof.Requires:
TODO
servicetest.OnStartClose(mock)- or a broader set including Ready(), HealthReport(), etc.servicetest.MockLifecycle(mockService)