tests: add option to dump genesis files in E2E tests #6100
Conversation
WalkthroughThe overarching change across the end-to-end (E2E) test suite involves adding support for dumping genesis files for debugging purposes. This is achieved by introducing a new configuration method, Changes
Assessment against linked issues
Poem
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 (
|
e2e/scripts/run-e2e.sh
Outdated
| @@ -59,7 +59,7 @@ _verify_dependencies | |||
|
|
|||
| # if the dev configs directory is present, enable fzf completion to select a test config file to use. | |||
| if [[ -d "dev-configs" ]]; then | |||
| export E2E_CONFIG_PATH="$(pwd)/dev-configs/$(_select_test_config)" | |||
| export E2E_CONFIG_PATH="/workspaces/ibc-go/e2e/dev-configs/config.yaml" | |||
There was a problem hiding this comment.
This needs to be reverted before review. I had to do it as I don't have fzf available on github codespaces
bznein
changed the title
bznein
requested review from
colin-axner,
AdityaSripal,
crodriguezvega,
charleenfei,
damiannolan,
chatton and
DimitrisJim
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (20)
- e2e/tests/core/02-client/client_test.go (1 hunks)
- e2e/tests/core/03-connection/connection_test.go (1 hunks)
- e2e/tests/interchain_accounts/base_test.go (1 hunks)
- e2e/tests/interchain_accounts/gov_test.go (1 hunks)
- e2e/tests/interchain_accounts/groups_test.go (1 hunks)
- e2e/tests/interchain_accounts/incentivized_test.go (1 hunks)
- e2e/tests/interchain_accounts/localhost_test.go (1 hunks)
- e2e/tests/interchain_accounts/params_test.go (1 hunks)
- e2e/tests/interchain_accounts/query_test.go (1 hunks)
- e2e/tests/interchain_accounts/upgrades_test.go (1 hunks)
- e2e/tests/transfer/authz_test.go (1 hunks)
- e2e/tests/transfer/base_test.go (1 hunks)
- e2e/tests/transfer/incentivized_test.go (1 hunks)
- e2e/tests/transfer/localhost_test.go (1 hunks)
- e2e/tests/transfer/upgrades_test.go (1 hunks)
- e2e/tests/upgrades/genesis_test.go (1 hunks)
- e2e/tests/upgrades/upgrade_test.go (1 hunks)
- e2e/testsuite/diagnostics/diagnostics.go (2 hunks)
- e2e/testsuite/testconfig.go (7 hunks)
- e2e/testsuite/testsuite.go (2 hunks)
Additional comments not posted (24)
e2e/tests/interchain_accounts/gov_test.go (1)
31-33: Ensure theConfigureGenesisDebugExportmethod is implemented correctly and adheres to the intended debug export configuration for Genesis files.e2e/tests/core/03-connection/connection_test.go (1)
28-30: Ensure theConfigureGenesisDebugExportmethod is correctly implemented to enable Genesis file debugging as intended.e2e/tests/interchain_accounts/query_test.go (1)
29-31: Confirm that theConfigureGenesisDebugExportmethod is implemented as expected to facilitate Genesis file debugging.e2e/testsuite/diagnostics/diagnostics.go (1)
165-166: The renaming ofgetE2EDirtoGetE2EDirimproves consistency and clarity in naming conventions. Ensure that all references to this function have been updated accordingly.e2e/tests/transfer/localhost_test.go (1)
23-25: Verify that theConfigureGenesisDebugExportmethod is correctly implemented to enable Genesis file debugging for theLocalhostTransferTestSuite.e2e/tests/interchain_accounts/groups_test.go (1)
62-64: Ensure theConfigureGenesisDebugExportmethod is implemented correctly for theInterchainAccountsGroupsTestSuiteto facilitate Genesis file debugging.e2e/tests/upgrades/genesis_test.go (1)
32-34: Confirm that theConfigureGenesisDebugExportmethod is correctly implemented for theGenesisTestSuiteto enable Genesis file debugging.e2e/tests/interchain_accounts/params_test.go (1)
34-36: LGTM! Ensure that the integration and functionality of the Genesis file dumping feature are verified through additional testing.e2e/tests/transfer/authz_test.go (1)
26-28: LGTM! The addition ofConfigureGenesisDebugExportbefore running the test suite is a good practice for setting up test configurations, especially for debugging purposes.e2e/tests/interchain_accounts/upgrades_test.go (1)
32-34: LGTM! The addition ofConfigureGenesisDebugExportbefore running the test suite is a good practice for setting up test configurations, especially for debugging purposes.e2e/tests/interchain_accounts/incentivized_test.go (1)
31-33: LGTM! The addition ofConfigureGenesisDebugExportbefore running the test suite is a good practice for setting up test configurations, especially for debugging purposes.e2e/tests/transfer/upgrades_test.go (1)
24-26: The addition ofConfigureGenesisDebugExportbefore running the test suite is appropriate for enabling Genesis file dumping for debugging purposes. Ensure that this new setup integrates well with the existing test suite and does not introduce any side effects.e2e/tests/interchain_accounts/localhost_test.go (1)
33-35: The addition ofConfigureGenesisDebugExportbefore running the test suite is appropriate for enabling Genesis file dumping for debugging purposes. Ensure that this new setup integrates well with the existing test suite and does not introduce any side effects.e2e/tests/transfer/base_test.go (1)
28-30: LGTM! The addition ofConfigureGenesisDebugExportbefore running the test suite ensures that Genesis file dumping is configured correctly for debugging purposes.e2e/tests/interchain_accounts/base_test.go (1)
39-41: LGTM! The addition ofConfigureGenesisDebugExportbefore running the test suite ensures that Genesis file dumping is configured correctly for debugging purposes.e2e/testsuite/testsuite.go (3)
7-10: The import statements foros,path, andtestingare correctly added to support the new functionality introduced in this PR.
73-112: The logic and flow within theConfigureGenesisDebugExportfunction are correctly implemented to configure the environment for Genesis file export based on the test configuration.
102-111: The setting of environment variablesEXPORT_GENESIS_FILE_PATHandEXPORT_GENESIS_CHAINis correctly implemented. Ensure that the usage and purpose of these environment variables are well-documented for maintainability and clarity for other developers.e2e/tests/core/02-client/client_test.go (1)
47-49: The addition ofConfigureGenesisDebugExportbefore running the test suite is appropriate for enabling Genesis file dumping for debugging. Ensure that this function does not inadvertently alter test behavior when Genesis file dumping is not enabled.e2e/testsuite/testconfig.go (3)
71-77: The addition ofdefaultGenesisExportPathanddefaultChainNamesprovides sensible defaults for the Genesis file dumping feature and chain naming, respectively. These are straightforward and support the new functionality well.
173-200: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [176-260]
The addition of
GetChainIndex,validateGenesisDebugConfig,GetChainName, andGetGenesisChainNamemethods enhances the test suite's configuration capabilities, especially for the new Genesis file dumping feature. Ensure that appropriate error handling is implemented forGetChainIndexto manage cases where an invalid chain name is provided.
280-302: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [271-292]
The introduction of a
Namefield in theChainConfigstruct and the creation of theGenesisDebugConfigstruct are key enhancements for supporting the Genesis file dumping feature. It's recommended to document the expected format or constraints for theExportFilePathinGenesisDebugConfigto guide users in configuring this feature correctly.e2e/tests/transfer/incentivized_test.go (1)
35-36: The addition of theConfigureGenesisDebugExportmethod call before running the test suite is correctly placed and aligns with the PR's objectives to enable Genesis file dumping for debugging purposes.e2e/tests/upgrades/upgrade_test.go (1)
50-52: Ensure that the behavior of the test suite remains consistent when Genesis file dumping is not enabled or when the environment variables controlling this feature are not set.
e2e/testsuite/testsuite.go
Outdated
| if exportPath == "" { | ||
| e2eDir, err := diagnostics.GetE2EDir(t) | ||
| if err != nil { | ||
| s.Fail("can't get e2edir: %w", err) |
There was a problem hiding this comment.
Consider using s.Require().NoError(err) instead of s.Fail for error handling to immediately halt the test execution upon encountering an error, ensuring cleaner test failures and avoiding potential subsequent errors.
- s.Fail("can't get e2edir: %w", err)
+ s.Require().NoError(err, "can't get e2edir")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| s.Fail("can't get e2edir: %w", err) | |
| s.Require().NoError(err, "can't get e2edir") |
There was a problem hiding this comment.
Thank you kind rabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- e2e/testsuite/testconfig.go (7 hunks)
- e2e/testsuite/testsuite.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- e2e/testsuite/testconfig.go
- e2e/testsuite/testsuite.go
e2e/testsuite/testsuite.go
Outdated
|
|
||
| // If no path is provided, use the default (e2e/diagnostics/genesis.json). | ||
| if exportPath == "" { | ||
| e2eDir, err := diagnostics.GetE2EDir(s.T()) |
There was a problem hiding this comment.
Might be worth moving this to an e2e/internal instead of diagnostics? Since it's being used elsewhere now, I think either is probably fine though.
| exportPath = gopath.Join(wd, exportPath) | ||
| } | ||
|
|
||
| t.Setenv("EXPORT_GENESIS_FILE_PATH", exportPath) |
There was a problem hiding this comment.
maybe add a link to interchaintest code for where this is used
e2e/testsuite/testconfig.go
Outdated
| @@ -68,8 +68,14 @@ const ( | |||
| // defaultConfigFileName is the default filename for the config file that can be used to configure | |||
| // e2e tests. See sample.config.yaml as an example for what this should look like. | |||
| defaultConfigFileName = ".ibc-go-e2e-config.yaml" | |||
|
|
|||
| // defaultGenesisExportPath is the default path to which Genesis debug files will be exported to. | |||
| defaultGenesisExportPath = "diagnostics/genesis.json" | |||
There was a problem hiding this comment.
if we end up moving the GetE2EPath to internal, maybe we can move this constant there too.
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (4)
- e2e/internal/directories/directories.go (1 hunks)
- e2e/testsuite/diagnostics/diagnostics.go (3 hunks)
- e2e/testsuite/testconfig.go (7 hunks)
- e2e/testsuite/testsuite.go (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- e2e/testsuite/diagnostics/diagnostics.go
- e2e/testsuite/testconfig.go
- e2e/testsuite/testsuite.go
Additional comments not posted (1)
e2e/internal/directories/directories.go (1)
14-15: Introduce a configurable path for Genesis file export.While having a default path (
diagnostics/genesis.json) for Genesis file exports is a good practice, it might be beneficial to allow this path to be configurable through environment variables or test configuration. This would provide flexibility in specifying the output directory based on different testing environments or requirements.
e2e/testsuite/testsuite.go
Outdated
| @@ -4,6 +4,8 @@ import ( | |||
| "context" | |||
| "errors" | |||
| "fmt" | |||
| "os" | |||
| gopath "path" | |||
There was a problem hiding this comment.
any reason why an alias was chosen here?
There was a problem hiding this comment.
Good catch and yes! The reason is that at line 175 we have a function that uses a variable named path, and this made the linter sad :(
Of course, now that I think about it, I could have easily just changed the variable instead, which is probably cleaner. WDYT?
There was a problem hiding this comment.
No super strong opinion but I'd go with changing the var name personally. Prefer to keep the stdlib imports as unaliased as possible 😄
Head branch was pushed to by a user without write access
e2e/testsuite/testsuite.go
Outdated
| @@ -67,6 +70,49 @@ func newPath(chainA, chainB ibc.Chain) pathPair { | |||
| } | |||
| } | |||
|
|
|||
| func (s *E2ETestSuite) SetupTest() { | |||
| s.ConfigureGenesisDebugExport() | |||
There was a problem hiding this comment.
this basically means we'll run these per test yea? Is this expected behavior?
There was a problem hiding this comment.
yeah, basically just a way to make sure that env vars get set for each specific test based on the config.
e2e/testsuite/testsuite.go
Outdated
| } | ||
|
|
||
| // ConfigureGenesisDebugExport sets, if needed, env variables to enable exporting of Genesis debug files. | ||
| func (s *E2ETestSuite) ConfigureGenesisDebugExport() { |
There was a problem hiding this comment.
looking now, I think we can make this unexported actually, should never have as need for this outside of this testsuite.
Co-authored-by: DimitrisJim <[email protected]>
Co-authored-by: Nikolas De Giorgis <[email protected]> Co-authored-by: Cian Hatton <[email protected]> Co-authored-by: DimitrisJim <[email protected]>
Description
Adds new fields in the e2e config to enable dumping of Genesis files.
closes: #6032
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
getE2EDirtoGetE2EDirindiagnostics.gofor consistent naming.testconfig.go.diagnostics.gofor better clarity.