fix(rfq-relayer): e2e test flakes #3075
Conversation
WalkthroughThe changes enhance contract deployment and approval processes within the Changes
Sequence Diagram(s)sequenceDiagram
participant IntegrationSuite
participant Backend
participant Contract
IntegrationSuite->>Backend: Initiate contract deployment
Backend-->>Contract: Deploy contract
Contract-->>Backend: Contract deployed
Backend-->>IntegrationSuite: Confirm deployment
IntegrationSuite->>IntegrationSuite: Call waitForContractDeployment
IntegrationSuite->>Backend: Check contract code
Backend-->>IntegrationSuite: Return contract code
IntegrationSuite->>Backend: Proceed with approval
Tip We have updated our review workflow to use the Anthropic's Claude family of models. Please share any feedback in the discussion post on our Discord. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3075 +/- ##
===================================================
+ Coverage 24.88692% 29.86118% +4.97426%
===================================================
Files 184 450 +266
Lines 11275 29319 +18044
Branches 82 82
===================================================
+ Hits 2806 8755 +5949
- Misses 8214 19735 +11521
- Partials 255 829 +574
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/e2e/setup_test.go (2 hunks)
Additional comments not posted (3)
services/rfq/e2e/setup_test.go (3)
209-209: Increase retry durationThe retry duration has been increased from 15 seconds to 30 seconds in the
setupCCTPfunction. This change improves reliability in scenarios where contract deployment may take longer than initially expected.The code changes are approved.
221-232: LGTM!The
waitForContractDeploymentfunction is correctly implemented with proper error handling and retry logic.The code changes are approved.
234-249: LGTM!The
Approvefunction is more robust with better error handling. The changes improve the clarity and reliability of the contract interaction logic.The code changes are approved.
Deploying sanguine-fe with
|
| Latest commit: |
9c523c6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f1829e56.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-rfq-ci-flake.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- services/rfq/e2e/setup_test.go (2 hunks)
- services/rfq/relayer/service/relayer.go (1 hunks)
Additional comments not posted (6)
services/rfq/relayer/service/relayer.go (2)
104-104: Enhanced error reporting in contract creationThe error message now includes the address of the FastBridge contract, providing more context for debugging.
The code changes are approved.
108-108: Enhanced error reporting in deploy block retrievalThe error message now includes the address of the FastBridge contract, providing more context for debugging.
The code changes are approved.
services/rfq/e2e/setup_test.go (4)
209-209: Increased retry durationThe retry duration has been increased from 15 seconds to 30 seconds, which can improve reliability in scenarios where contract deployment may take longer than initially expected.
The code changes are approved.
221-225: LGTM!The function correctly checks if a contract is deployed at a specified address by querying the backend for the contract's code.
The code changes are approved.
227-238: LGTM!The function correctly encapsulates the logic for checking if a contract is deployed at a specified address by querying the backend for the contract's code. The retry mechanism enhances reliability.
The code changes are approved.
240-255: LGTM!The
Approvefunction has been modified to use the newwaitForContractDeploymentfunction, ensuring that the contract is deployed before proceeding with the approval process. This improves reliability and error handling.The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/e2e/setup_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/e2e/setup_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- ethergo/backends/anvil/anvil.go (1 hunks)
Additional comments not posted (1)
ethergo/backends/anvil/anvil.go (1)
132-132: LGTM! But verify the impact of the increased timeout.The increase in
pool.MaxWaitto ten minutes can improve the resilience of the connection process. However, ensure that this does not lead to excessively long waits in scenarios where a shorter timeout would be more appropriate.The code changes are approved.
Run the following script to verify the impact of the increased timeout:
Verification successful
No adverse production impact from increased timeout.
The
NewAnvilBackendfunction's increased timeout is used only in test files, and theSetMaxWaitTimefunction provides flexibility to adjust it if necessary. This ensures that the change is unlikely to negatively affect production environments.
- The
NewAnvilBackendfunction is not used in any non-test files.- The
MaxWaitproperty is configurable viaSetMaxWaitTime.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the increased timeout. # Test: Search for the usage of `NewAnvilBackend`. Expect: No adverse effects due to the increased timeout. rg --type go -A 5 $'NewAnvilBackend'Length of output: 4322
Script:
#!/bin/bash # Search for usage of `NewAnvilBackend` in non-test files to check for production impact. rg --type go --invert-match '_test.go' -A 5 $'NewAnvilBackend' # Search for tests that might be evaluating timeout behavior. rg --type go 'MaxWait' -A 5Length of output: 2507
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- agents/testutil/simulated_backends_suite.go (4 hunks)
- ethergo/backends/anvil/anvil.go (12 hunks)
- ethergo/submitter/suite_test.go (2 hunks)
- services/rfq/e2e/setup_test.go (3 hunks)
Additional comments not posted (12)
ethergo/submitter/suite_test.go (1)
118-120: LGTM!The code changes improve error handling by capturing any errors encountered during the creation of the
AnvilBackendinstance and asserting that no errors occurred. This enhances the robustness of the code and prevents potential runtime issues if the backend initialization fails.ethergo/backends/anvil/anvil.go (2)
Line range hint
68-210: LGTM!The changes to error handling and return types significantly enhance the robustness and clarity of the
NewAnvilBackendfunction. Errors are now properly propagated and handled, and the increasedMaxWaitproperty improves reliability by allowing more time for connection retries.
Line range hint
225-283: LGTM!The changes to error handling and return types significantly enhance the robustness and clarity of the
setupOtterscanfunction. Errors are now properly propagated and handled.services/rfq/e2e/setup_test.go (6)
114-121: LGTM!The increased retry duration from 15 seconds to 30 seconds is a good change to improve reliability in case the backend creation takes longer. The error handling and nil check after the retry loop look correct.
128-135: LGTM!The increased retry duration from 15 seconds to 30 seconds is a good change to improve reliability in case the backend creation takes longer. The error handling and nil check after the retry loop look correct.
223-224: LGTM!The increased retry duration from 15 seconds to 30 seconds is a good change to improve reliability in case the CCTP setup takes longer. The error handling after the retry loop looks correct.
235-247: LGTM!The new
waitForContractDeploymentfunction is a good addition to encapsulate the logic for waiting for a contract deployment. It improves code readability and maintainability. The retry duration of 30 seconds is reasonable to handle potential delays in contract deployment. The error handling and messaging look correct.
249-264: LGTM!The modifications to the
Approvefunction improve its reliability and error handling. Using thewaitForContractDeploymentfunction ensures that the contract is deployed before proceeding with the approval process. The improved error handling usingi.Require().NoErrorenhances the robustness of the function by validating the success of each step. The overall flow of the function looks correct and more resilient to potential issues related to contract deployment timing.
Line range hint
358-359: LGTM!The increased retry duration from 15 seconds to 30 seconds for adding the relayer role is a good change to improve reliability. It provides more time for the operation to succeed in case of any delays or transient issues. The change is consistent with the other retry duration increases in the file.
agents/testutil/simulated_backends_suite.go (3)
416-416: LGTM!Declaring the
errvariable at the beginning of the function is a good practice to allow reusing it in multiple goroutines.
423-424: Improved error handling.Capturing the error returned by
anvil.NewAnvilBackendand asserting that it is nil usinga.Nil(err)enhances the robustness of the backend setup process. This ensures that any errors encountered during backend initialization are properly handled, preventing potential runtime issues.
Line range hint
481-487: Improved error handling.Capturing the error returned by
a.TestDeployManager.LoadHarnessContractsOnChainsand fatally failing the test if the error is non-nil ensures that the test is aborted if there are any issues during the loading of harness contracts on chains. This prevents the test from proceeding with an invalid state.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- ethergo/backends/anvil/anvil.go (12 hunks)
- ethergo/backends/anvil/suite_test.go (2 hunks)
Additional comments not posted (4)
ethergo/backends/anvil/suite_test.go (2)
48-49: LGTM!The changes improve error handling in backend initialization by:
- Capturing the error returned by
anvil.NewAnvilBackend.- Checking that the error is nil using
Nil(a.T(), err).This ensures that any errors encountered during the backend initialization are properly handled and the backend is successfully created before proceeding.
Line range hint
37-37: Verify the impact of theSetupSuitefunction signature change.The list of alterations indicates an implicit change to the
SetupSuitefunction signature fromfunc (a *AnvilSuite) SetupSuite()tofunc (a *AnvilSuite) SetupSuite() error.Ensure that:
- The
SetupSuitefunction returns an error when appropriate.- The returned error is handled correctly by the test suite.
- The change does not break any existing tests.
Run the following script to verify the impact of the function signature change:
ethergo/backends/anvil/anvil.go (2)
Line range hint
68-210: LGTM!The changes to error handling and return types significantly enhance the robustness and clarity of the function's operation, ensuring that errors are properly propagated and handled. The increased
MaxWaitproperty improves the timeout behavior during connection retries.
Line range hint
225-283: LGTM!The changes to error handling and return types significantly enhance the robustness and clarity of the function's operation, ensuring that errors are properly propagated and handled. The improved error handling for context cancellation errors during container pull is a good addition.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- ethergo/backends/anvil/anvil.go (12 hunks)
- packages/rest-api/package.json (1 hunks)
- services/rfq/e2e/setup_test.go (3 hunks)
- services/rfq/relayer/relapi/server_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- packages/rest-api/package.json
Additional context used
GitHub Check: codecov/patch
ethergo/backends/anvil/anvil.go
[warning] 73-73: ethergo/backends/anvil/anvil.go#L73
Added line #L73 was not covered by tests
[warning] 80-81: ethergo/backends/anvil/anvil.go#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 102-103: ethergo/backends/anvil/anvil.go#L102-L103
Added lines #L102 - L103 were not covered by tests
[warning] 125-126: ethergo/backends/anvil/anvil.go#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 134-135: ethergo/backends/anvil/anvil.go#L134-L135
Added lines #L134 - L135 were not covered by tests
[warning] 156-156: ethergo/backends/anvil/anvil.go#L156
Added line #L156 was not covered by tests
[warning] 160-161: ethergo/backends/anvil/anvil.go#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 171-172: ethergo/backends/anvil/anvil.go#L171-L172
Added lines #L171 - L172 were not covered by tests
[warning] 185-186: ethergo/backends/anvil/anvil.go#L185-L186
Added lines #L185 - L186 were not covered by tests
[warning] 198-199: ethergo/backends/anvil/anvil.go#L198-L199
Added lines #L198 - L199 were not covered by tests
[warning] 249-249: ethergo/backends/anvil/anvil.go#L249
Added line #L249 was not covered by tests
[warning] 256-257: ethergo/backends/anvil/anvil.go#L256-L257
Added lines #L256 - L257 were not covered by tests
[warning] 283-283: ethergo/backends/anvil/anvil.go#L283
Added line #L283 was not covered by tests
Additional comments not posted (7)
services/rfq/relayer/relapi/server_test.go (2)
79-79: LGTM!The addition of the
Senderfield to theexpectedResultstructure enhances the completeness of the test by ensuring that the sender's details are validated alongside other transaction attributes.
122-122: LGTM!The addition of the
Senderfield to theexpectedResultstructure enhances the completeness of the test by ensuring that the sender's details are validated alongside other transaction attributes.ethergo/backends/anvil/anvil.go (2)
68-68: LGTM!The change to the
NewAnvilBackendfunction signature to return(*Backend, error)is a good practice for improving error handling. This allows the function to communicate errors more effectively.
225-225: LGTM!The change to the
setupOtterscanfunction signature to return(string, error)is a good practice for improving error handling. This allows the function to communicate errors along with the string result.services/rfq/e2e/setup_test.go (3)
223-224: LGTM!Increasing the retry duration from 15 seconds to 30 seconds allows more time for the CCTP contract setup to complete, which can improve the reliability of the tests.
235-247: LGTM!The new
waitForContractDeploymentfunction improves the reliability of the tests by ensuring that a contract is deployed before proceeding. The retry logic handles transient failures and the function is well-structured.
249-264: LGTM!The changes to the
Approvefunction improve its reliability and error handling:
- Using the
waitForContractDeploymentfunction ensures that the contract is deployed before proceeding with the approval process.- Checking for errors and failing the test improves the reliability and debuggability of the tests.
The code segment is well-structured and follows best practices.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- ethergo/backends/anvil/anvil.go (12 hunks)
Additional context used
GitHub Check: codecov/patch
ethergo/backends/anvil/anvil.go
[warning] 73-73: ethergo/backends/anvil/anvil.go#L73
Added line #L73 was not covered by tests
[warning] 80-81: ethergo/backends/anvil/anvil.go#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 102-103: ethergo/backends/anvil/anvil.go#L102-L103
Added lines #L102 - L103 were not covered by tests
[warning] 125-126: ethergo/backends/anvil/anvil.go#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 134-135: ethergo/backends/anvil/anvil.go#L134-L135
Added lines #L134 - L135 were not covered by tests
[warning] 156-156: ethergo/backends/anvil/anvil.go#L156
Added line #L156 was not covered by tests
[warning] 160-161: ethergo/backends/anvil/anvil.go#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 170-171: ethergo/backends/anvil/anvil.go#L170-L171
Added lines #L170 - L171 were not covered by tests
[warning] 184-185: ethergo/backends/anvil/anvil.go#L184-L185
Added lines #L184 - L185 were not covered by tests
[warning] 197-198: ethergo/backends/anvil/anvil.go#L197-L198
Added lines #L197 - L198 were not covered by tests
[warning] 248-248: ethergo/backends/anvil/anvil.go#L248
Added line #L248 was not covered by tests
[warning] 255-256: ethergo/backends/anvil/anvil.go#L255-L256
Added lines #L255 - L256 were not covered by tests
[warning] 282-282: ethergo/backends/anvil/anvil.go#L282
Added line #L282 was not covered by tests
Additional comments not posted (2)
ethergo/backends/anvil/anvil.go (2)
Line range hint
68-209: Improved error handling logic.The error handling enhancements using conditional checks and formatted error messages improve the robustness of the
NewAnvilBackendfunction by properly propagating and handling errors. The error messages provide clear context about the failures.Tools
GitHub Check: codecov/patch
[warning] 156-156: ethergo/backends/anvil/anvil.go#L156
Added line #L156 was not covered by tests
[warning] 160-161: ethergo/backends/anvil/anvil.go#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 170-171: ethergo/backends/anvil/anvil.go#L170-L171
Added lines #L170 - L171 were not covered by tests
Line range hint
224-282: Improved error handling logic.The error handling enhancements using conditional checks and formatted error messages improve the robustness of the
setupOtterscanfunction by properly propagating and handling errors. The error messages provide clear context about the failures.Tools
GitHub Check: codecov/patch
[warning] 248-248: ethergo/backends/anvil/anvil.go#L248
Added line #L248 was not covered by tests
[warning] 255-256: ethergo/backends/anvil/anvil.go#L255-L256
Added lines #L255 - L256 were not covered by tests
| if err != nil { | ||
| require.Nil(t, err) | ||
| return nil, fmt.Errorf("failed to create docker pool: %w", err) |
There was a problem hiding this comment.
Add test coverage for error handling.
Static analysis hints indicate that the added lines for error handling are not covered by tests:
- Line 73
- Lines 80-81
- Lines 102-103
- Lines 134-135
- Line 156
- Lines 160-161
- Lines 170-171
- Lines 184-185
- Lines 197-198
To ensure the reliability of the error handling logic, please add appropriate test cases that cover these lines. This will help catch any potential issues and maintain the stability of the codebase.
If you need any assistance in writing the test cases, please let me know. I'd be happy to help!
Also applies to: 80-81, 102-103, 134-135, 156-156, 160-161, 170-171, 184-185, 197-198
Tools
GitHub Check: codecov/patch
[warning] 73-73: ethergo/backends/anvil/anvil.go#L73
Added line #L73 was not covered by tests
| if !errors.Is(err, context.Canceled) { | ||
| require.Nil(tb, err) | ||
| if err != nil && !errors.Is(err, context.Canceled) { | ||
| return "", fmt.Errorf("failed to run otterscan container: %w", err) |
There was a problem hiding this comment.
Add test coverage for error handling.
Static analysis hints indicate that the added lines for error handling are not covered by tests:
- Line 248
- Lines 255-256
- Line 282
To ensure the reliability of the error handling logic, please add appropriate test cases that cover these lines. This will help catch any potential issues and maintain the stability of the codebase.
If you need any assistance in writing the test cases, please let me know. I'd be happy to help!
Also applies to: 255-256, 282-282
Tools
GitHub Check: codecov/patch
[warning] 248-248: ethergo/backends/anvil/anvil.go#L248
Added line #L248 was not covered by tests
Bundle ReportChanges will decrease total bundle size by 145.67kB ⬇️
|
Summary by CodeRabbit