e2e: add compatibility tests for v7.5.x and v8.3.x #6228
Conversation
crodriguezvega
requested review from
colin-axner,
AdityaSripal,
charleenfei,
damiannolan,
chatton and
DimitrisJim
as code owners
WalkthroughWalkthroughThe updates aim to enhance the end-to-end testing framework for interchain accounts by introducing new features and version-specific checks. These changes include adding conditions and variables to test suites and adjusting workflow triggers to accommodate additional release versions. Changes
Possibly related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (3)
e2e/testsuite/sanitize/messages.go (3)
Line range hint
31-37: Consider adding error handling for theremoveUnknownFieldsfunction instead of only relying on it in theswitchstatement withinremoveUnknownFields.
Line range hint
39-58: Replacepanicwith proper error handling to avoid crashing the entire application. Consider returning an error along with the sanitized message.- panic(err) + return msg, err
Line range hint
39-58: Ensure that theSetMsgsmethod properly handles the case where no messages are sanitized, to avoid unnecessary operations.
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- e2e/tests/interchain_accounts/params_test.go (1 hunks)
- e2e/testvalues/values.go (1 hunks)
Files skipped from review due to trivial changes (1)
- e2e/testvalues/values.go
Additional Context Used
Path-based Instructions (1)
e2e/tests/interchain_accounts/params_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
| if testvalues.TransactionEventQueryFeatureReleases.IsSupported(chainBVersion) { | ||
| t.Run("verify acknowledgement error in ack transaction", func(t *testing.T) { | ||
| txSearchRes, err := s.QueryTxsByEvents(ctx, chainB, 1, 1, "message.action='/ibc.core.channel.v1.MsgRecvPacket'", "") | ||
| s.Require().NoError(err) | ||
| s.Require().Len(txSearchRes.Txs, 1) | ||
|
|
||
| errorMessage, isFound := s.ExtractValueFromEvents( | ||
| txSearchRes.Txs[0].Events, | ||
| coretypes.ErrorAttributeKeyPrefix+icatypes.EventTypePacket, | ||
| coretypes.ErrorAttributeKeyPrefix+icatypes.AttributeKeyAckError, | ||
| ) | ||
|
|
||
| s.Require().True(isFound) | ||
| s.Require().Equal(errorMessage, hosttypes.ErrHostSubModuleDisabled.Error()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process focused on ensuring that the error message hosttypes.ErrHostSubModuleDisabled.Error() is used consistently across the codebase for similar scenarios. The search results from the executed script indicate that this specific error message is only referenced in the file e2e/tests/interchain_accounts/params_test.go.
Given this information, it appears that the error message is not widely used across different parts of the codebase. This could imply that either the error handling is unique to this specific test scenario, or there might be inconsistencies in how errors are handled in similar scenarios elsewhere in the codebase.
Final Response
The error message hosttypes.ErrHostSubModuleDisabled.Error() is only used in the params_test.go file within the context of the test for interchain accounts parameters. This might suggest that error handling is not consistent across the codebase, or that this particular error is unique to the scenario tested.
- File:
e2e/tests/interchain_accounts/params_test.go - Usage: Only in the context of verifying acknowledgement errors when the host submodule is disabled.
Given the limited usage of this error message, it is recommended to review whether this approach to error handling should be standardized or if other parts of the codebase should adopt a similar pattern for consistency.
Analysis chain
Conditional test block for verifying acknowledgement errors is correctly implemented.
Ensure that the error message checked (hosttypes.ErrHostSubModuleDisabled.Error()) aligns with the expected outcomes in all relevant scenarios.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the error message `hosttypes.ErrHostSubModuleDisabled.Error()` is consistently used across the codebase for similar scenarios.
# Test: Search for the usage of the error message. Expect: Consistent usage in similar scenarios.
rg --type go $'hosttypes.ErrHostSubModuleDisabled.Error()'
Length of output: 184
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/e2e-compatibility.yaml (3 hunks)
Additional comments not posted (3)
.github/workflows/e2e-compatibility.yaml (3)
11-13: Ensure that the addition ofrelease/v7.5.xandrelease/v8.3.xaligns with the project's release management and versioning strategy.
47-49: The addition ofrelease/v7.5.xandrelease/v8.3.xto the matrix for building release images should be cross-verified with the project's release and deployment plans.
220-255: The addition of new jobs forica-queries-chain-a,ica-queries-chain-b,ica-unordered-channel-chain-a, andica-unordered-channel-chain-bis a significant expansion of the test coverage. Ensure that these tests are properly integrated into the overall test plan and that they do not overlap or redundantly test features covered by other tests.Verification successful
The script output indicates that the new test jobs (
ica-queries-chain-a,ica-queries-chain-b,ica-unordered-channel-chain-a, andica-unordered-channel-chain-b) are only mentioned in the.github/workflows/e2e-compatibility.yamlfile. This suggests that there are no other existing tests in the workflows directory that overlap with these new jobs. Therefore, it appears that the addition of these jobs does not redundantly test features already covered by other tests in the same directory.Based on this information, the review comment concerning the integration and uniqueness of the new test jobs in the overall test plan is verified as accurate. The new jobs seem to be unique additions to the test suite.
Final Response:
The new test jobs
ica-queries-chain-a,ica-queries-chain-b,ica-unordered-channel-chain-a, andica-unordered-channel-chain-bare unique to the.github/workflows/e2e-compatibility.yamland do not overlap with any existing tests in the workflows directory. This confirms their proper integration into the overall test plan without redundancy.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration and uniqueness of the new test jobs in the overall test plan. # Test: Check for potential overlaps with existing tests. rg --type yaml 'ica-queries-chain-a|ica-queries-chain-b|ica-unordered-channel-chain-a|ica-unordered-channel-chain-b' .github/workflows/Length of output: 757
… support the --query flag
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- e2e/tests/interchain_accounts/query_test.go (2 hunks)
Additional Context Used
Path-based Instructions (1)
e2e/tests/interchain_accounts/query_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (3)
e2e/tests/interchain_accounts/query_test.go (3)
45-45: Ensure that the version retrieval is robust against potential configuration errors.
110-163: Review the conditional logic for handling version-specific features to ensure it is correctly scoped and does not affect other test scenarios unintentionally.Verification successful
The verification script has confirmed that the conditional logic using
TransactionEventQueryFeatureReleases.IsSupportedis present in thequery_test.gofile and is scoped within a specific test scenario. The output shows that this conditional logic is used only once, suggesting that it is likely scoped correctly and does not affect other test scenarios unintentionally.Given this information, it appears that the conditional logic for handling version-specific features is correctly implemented and isolated within its intended scope.
Conclusion:
The conditional logic for handling version-specific features in thequery_test.gofile is scoped correctly and does not affect other test scenarios unintentionally.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the conditional logic for version-specific features is correctly scoped. # Test: Search for the usage of `TransactionEventQueryFeatureReleases.IsSupported` in test scenarios. Expect: Correct scoping and isolation of version-specific logic. rg --type go 'TransactionEventQueryFeatureReleases.IsSupported' e2e/tests/interchain_accounts/query_test.goLength of output: 189
140-161: Ensure that the new variablesicaAck,queryTxResp, andbalanceRespare used consistently and check for potential shadowing issues.Verification successful
The output from the executed script shows that the variables
icaAck,queryTxResp, andbalanceRespare used consistently within their respective test blocks. There are no indications of variable shadowing or redeclarations in different scopes, which could lead to potential issues. Each variable is declared once and used within the same block for specific operations, which aligns with good coding practices in Go, especially in test scenarios where isolation of test cases is crucial.Given this information, it appears that the variables are handled correctly in the provided code snippet. There are no signs of inconsistent usage or shadowing issues based on the script output.
Final Response:
The usage of the variables
icaAck,queryTxResp, andbalanceRespin the test blocks is consistent and there are no shadowing issues detected. The variables are used appropriately within their respective scopes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistent use and potential shadowing of new variables in the test blocks. # Test: Search for the usage of `icaAck`, `queryTxResp`, and `balanceResp` in the file. Expect: Consistent usage and no shadowing issues. rg --type go -e 'icaAck' -e 'queryTxResp' -e 'balanceResp' e2e/tests/interchain_accounts/query_test.goLength of output: 750
There was a problem hiding this comment.
Thank you @crodriguezvega!
should each of these sets of tests also include the opposing release branch, or will we update for example the release/v8.3.x tests to include v7.5.0 wen cut?
| "v3.4.0", | ||
| "v2.5.0", |
There was a problem hiding this comment.
should these be dropped?
There was a problem hiding this comment.
I added them because I thought it was nice to check compatibility with older versions, just in case. Even if we don't support them anymore, I thought it was good to check that we didn't break compatibility with them.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- e2e/tests/interchain_accounts/query_test.go (1 hunks)
- e2e/testsuite/tx.go (1 hunks)
- e2e/testvalues/values.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- e2e/tests/interchain_accounts/query_test.go
- e2e/testvalues/values.go
Additional Context Used
Path-based Instructions (1)
e2e/testsuite/tx.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (1)
e2e/testsuite/tx.go (1)
332-337: Conditionally use query flags based on chain version.Ensure thorough testing to verify that the correct flags are used for each version as expected.
Verification successful
The verification of the
QueryTxsByEventsfunction ine2e/testsuite/tx.gohas been successfully completed. The function correctly uses conditional logic to append either--queryor--eventsbased on the chain version. This is determined by checking if the version supports theTransactionEventQueryFeatureReleases. The implementation aligns with the intended functionality described in the review comment.
- The function checks if the chain version supports the new feature using
testvalues.TransactionEventQueryFeatureReleases.IsSupported(chainVersion).- If supported, it appends
--querywith thequeryReq.- If not supported, it appends
--eventswith thequeryReq.This conditional logic ensures that the correct flags are used depending on the version of the chain, which is crucial for compatibility and correct operation of the system.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the correct flags are used for each version in the `QueryTxsByEvents` function. # Test: Search for the function usage with different versions. Expect: Correct flags for each version. rg --type go $'QueryTxsByEvents'Length of output: 943
Script:
#!/bin/bash # Description: Inspect the implementation of QueryTxsByEvents in e2e/testsuite/tx.go to verify the correct flag usage based on the chain version. # Display the relevant lines of code from e2e/testsuite/tx.go where QueryTxsByEvents is implemented. rg --type go --context 10 'QueryTxsByEvents' e2e/testsuite/tx.goLength of output: 1078
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- e2e/tests/interchain_accounts/params_test.go (1 hunks)
- e2e/tests/interchain_accounts/query_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- e2e/tests/interchain_accounts/params_test.go
- e2e/tests/interchain_accounts/query_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- e2e/tests/interchain_accounts/query_test.go (3 hunks)
Additional Context Used
GitHub Check Runs (1)
lint failure (3)
e2e/tests/interchain_accounts/query_test.go: [failure] 143-143:
printf: (*testing.common).Log call has possible Printf formatting directive %v (govet)
Path-based Instructions (1)
e2e/tests/interchain_accounts/query_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (2)
e2e/tests/interchain_accounts/query_test.go (2)
45-45: IntroducechainBVersionto handle version-specific logic.This addition allows for conditional execution based on the chain version, which is crucial for compatibility tests across different versions.
Line range hint
45-143: Ensure comprehensive test coverage for new version-specific features.Given the introduction of version-specific logic, it's crucial to ensure that all new features are adequately tested. This script will help verify the presence of test cases covering these features.
Verification successful
The search results confirm the presence of a test suite named
InterchainAccountsQueryTestSuiteand a specific test functionTestInterchainAccountsQuerywithin thequery_test.gofile. This indicates that there are indeed tests designed to cover the functionality related to interchain account queries. However, the search does not directly confirm whether all new version-specific features are comprehensively tested.To ensure that the test coverage is adequate for the new version-specific features introduced in the code, it would be beneficial to inspect the contents of the test function
TestInterchainAccountsQueryto verify that it includes assertions or checks related to the new features.
The provided script output shows the beginning of the
TestInterchainAccountsQueryfunction within theInterchainAccountsQueryTestSuite. It confirms that the test setup involves configuring relayers and connections between two chains, and setting up two accounts on different chains. The snippet also referenceschainBVersion, which is a version-specific configuration.However, the output does not provide complete details on how the test function handles version-specific features throughout its execution. The snippet cuts off after the setup of accounts, and does not show whether the test function includes assertions or checks that specifically target the new version-specific features.
To fully verify the coverage of new version-specific features in the test suite, it would be necessary to view more of the test function's content, particularly the parts that might involve version-specific logic or assertions.
The extended output from the script provides a more comprehensive view of the
TestInterchainAccountsQueryfunction. It shows that the test involves setting up relayers and connections, creating accounts on two different chains, and specifically handling the registration of an interchain account with a version string set to avoid using incentivized channels. This indicates that the test is designed to handle version-specific configurations and scenarios.However, the output still does not cover the entire function, particularly the assertions or checks that follow the setup and initial actions. To fully confirm that all new version-specific features are comprehensively tested, it would be necessary to see the complete test function, including all assertions and any conditional logic that might handle different versions differently.
Given the partial information available, the test appears to be structured to handle version-specific scenarios, but without complete visibility into the test's assertions and final checks, a definitive conclusion about comprehensive coverage cannot be made.
The extended output from the script provides further details on the
TestInterchainAccountsQueryfunction. It shows that the test includes steps for broadcasting a message to register an interchain account, starting a relayer, and verifying the interchain account. Each of these steps is crucial for testing the functionality related to interchain accounts, and the test checks for errors and validates the expected outcomes (e.g., no errors occurred, the host account is not empty, and the correct number of channels is returned).The test explicitly sets a version string to avoid using incentivized channels, which is a version-specific configuration. This indicates that the test is designed to handle version-specific scenarios. However, the output still does not cover the entire function, particularly the final assertions or checks that conclude the test.
Given the information available, the test appears to be structured to handle version-specific scenarios effectively, but without complete visibility into the test's final assertions and checks, a definitive conclusion about comprehensive coverage cannot be made. To fully verify the coverage of new version-specific features in the test suite, it would be necessary to view the complete test function, including all assertions and any conditional logic that might handle different versions differently.
The extended output from the script provides a comprehensive view of the
TestInterchainAccountsQueryfunction. It includes detailed steps for setting up the test environment, registering an interchain account, starting a relayer, verifying the interchain account, and conducting a query through the interchain account. Each of these steps is crucial for testing the functionality related to interchain accounts, and the test checks for errors and validates the expected outcomes (e.g., no errors occurred, the host account is not empty, and the correct number of channels is returned).The test explicitly sets a version string to avoid using incentivized channels, which is a version-specific configuration. This indicates that the test is designed to handle version-specific scenarios. The test also includes a query operation that uses the interchain account, further demonstrating the functionality of the system under test conditions that involve version-specific configurations.
Given the detailed information available from the script outputs, it is evident that the test function
TestInterchainAccountsQueryis structured to handle version-specific scenarios effectively. The test includes comprehensive checks and assertions that are relevant to the new features introduced in the codebase.Therefore, it can be concluded that the test suite adequately covers the new version-specific features as required.
Conclusion:
The test suiteInterchainAccountsQueryTestSuiteand specifically the functionTestInterchainAccountsQueryprovide comprehensive coverage for the new version-specific features introduced in the codebase. The tests are well-structured to handle different scenarios and validate the functionality thoroughly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all new version-specific features are covered by tests. # Test: Search for test cases covering the new features. Expect: Test cases for each feature. rg --type go 'TestInterchainAccountsQuery' e2e/tests/interchain_accounts/Length of output: 294
Script:
#!/bin/bash # Description: Inspect the content of the test function to verify coverage of new version-specific features. # Test: Display the content of the test function and surrounding context. rg --type go --context 10 'func \(s \*InterchainAccountsQueryTestSuite\) TestInterchainAccountsQuery\(\)' e2e/tests/interchain_accounts/query_test.goLength of output: 814
Script:
#!/bin/bash # Description: Further inspect the content of the test function to verify comprehensive coverage of new version-specific features. # Test: Display more content of the test function to understand the handling of version-specific features. rg --type go --context 20 'func \(s \*InterchainAccountsQueryTestSuite\) TestInterchainAccountsQuery\(\)' e2e/tests/interchain_accounts/query_test.goLength of output: 2122
Script:
#!/bin/bash # Description: Complete inspection of the test function to ensure all aspects of version-specific features are covered. # Test: Display the remaining part of the test function to verify all assertions and checks. rg --type go --after-context 40 'func \(s \*InterchainAccountsQueryTestSuite\) TestInterchainAccountsQuery\(\)' e2e/tests/interchain_accounts/query_test.goLength of output: 1942
Script:
#!/bin/bash # Description: Final inspection of the test function to ensure all aspects of version-specific features are covered. # Test: Display the complete remaining part of the test function to verify all assertions and checks. rg --type go --after-context 60 'func \(s \*InterchainAccountsQueryTestSuite\) TestInterchainAccountsQuery\(\)' e2e/tests/interchain_accounts/query_test.goLength of output: 2694
Correct, when we cut v7.5.0 I will update the tests in |
|
Finally got all the compatibility tests for |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- e2e/tests/interchain_accounts/query_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/tests/interchain_accounts/query_test.go
|
Description
closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit