chore: pass knuu as an argument to the testnet.New #3952
Conversation
mojtaba-esk
requested review from
rootulp and
staheri14
and removed request for
a team
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to several end-to-end test files to integrate the Knuu package for improved test execution management. Key changes include the initialization of Knuu instances with unique identifiers, the introduction of a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
test/e2e/simple.go (1)
25-30: Initialize Knuu with appropriate optionsThe Knuu instance is initialized correctly with the specified options:
Scope: Provides isolation between test runs.ProxyEnabled: Allows for network proxying if required.Consider whether you need to uncomment and set the
Timeoutoption based on your test requirements.If timeouts are a concern for longer-running tests, you might uncomment the
Timeoutoption:// if the tests timeout, pass the timeout option - // Timeout: 120 * time.Minute, + Timeout: 120 * time.Minute,test/e2e/major_upgrade_v2.go (2)
21-21: Consider declaringtestNameas a constantSince
testNameis a value that does not change, declaring it as a constant improves code readability and conveys its immutability.Apply this diff to declare
testNameas a constant:+const testName = "MajorUpgradeToV2" var ( - testName = "MajorUpgradeToV2" numNodes = 4 upgradeHeight = int64(10) )
33-34: Consider removing or uncommenting theTimeoutoptionThe
Timeoutoption is currently commented out. If it's not needed, consider removing it to keep the code clean. If it might be used in the future, perhaps add a comment explaining under what circumstances it should be enabled.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- test/e2e/major_upgrade_v2.go (1 hunks)
- test/e2e/minor_version_compatibility.go (2 hunks)
- test/e2e/simple.go (1 hunks)
- test/e2e/testnet/testnet.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (16)
test/e2e/testnet/testnet.go (4)
33-39: LGTM: Well-structuredTestnetOptionsstructThe introduction of the
TestnetOptionsstruct is a good design choice. It encapsulates all the parameters previously passed to theNewfunction, improving flexibility and making future additions easier. The struct is well-organized with clear field names.
41-49: LGTM: ImprovedNewfunction withTestnetOptionsThe changes to the
Newfunction are well-implemented. The new signature simplifies the function call by accepting a singleTestnetOptionsparameter, which aligns with the PR objective of passingknuuas an argument. The function body correctly initializes theTestnetstruct using fields from theoptsparameter.
Line range hint
1-1: Summary: Changes align well with PR objectivesThe modifications in this file, including the introduction of
TestnetOptions, updates to theNewfunction, and removal of theKnuumethod, align well with the PR objective of passingknuuas an argument to the testnet constructor. These changes enhance the flexibility of the testnet constructor and allow for better customization of testnet configurations.The code structure has been improved, making it easier to add new options in the future. However, please ensure that all instances of
t.Knuu()in the codebase are updated to directly access theknuufield of theTestnetstruct.
Line range hint
1-1: Verify impact of removing theKnuumethodThe removal of the
Knuumethod from theTestnetstruct simplifies the code and reduces unnecessary abstraction. However, this change might impact existing code that uses theKnuumethod.Please run the following script to check for any remaining usage of the
Knuumethod:If the script returns any results, those occurrences should be updated to directly access the
knuufield of theTestnetstruct.test/e2e/simple.go (7)
12-12: Import Knuu package for testnet integrationThe addition of the import
"github.com/celestiaorg/knuu/pkg/knuu"is appropriate for integrating Knuu into the testnet. This allows for enhanced testnet management and flexibility.
19-19: Define 'testName' as a constantDefining
testNameas a constant improves code readability and maintainability. This makes it easier to reference and modify the test name if needed.
24-24: Generate a unique identifier for Knuu scopeCreating a unique
identifierusingtestNameand the current timestamp ensures that each test run operates in a distinct scope. This is a good practice to prevent conflicts between concurrent test executions.
31-31: Proper error handling after Knuu initializationUsing
testnet.NoError("failed to initialize Knuu", err)ensures that any errors during Knuu initialization are appropriately handled and reported. This enhances the robustness of the test setup.
32-32: Handle stop signals for Knuu gracefullyCalling
kn.HandleStopSignal(ctx)allows the Knuu instance to handle termination signals properly, ensuring resources are cleaned up and processes are stopped gracefully.
33-33: Log Knuu initialization detailsLogging the initialization details of Knuu, including the
scopeandtestName, provides valuable information for debugging and auditing test runs.
35-40: Pass Knuu instance to TestnetOptionsIntegrating the Knuu instance into
testnet.Newby passing it throughtestnet.TestnetOptionsenhances the testnet's flexibility and control. This change aligns with the PR's objective to passknuuas an argument totestnet.New.test/e2e/minor_version_compatibility.go (4)
18-18: Importing Knuu package: Looks goodThe addition of the Knuu package import is necessary for integrating Knuu into the test setup.
22-25: Constants declaration improves code clarityDefining
testNameandnumNodesas constants enhances readability and maintainability.
51-53: Initialization and logging of Knuu instance: ApprovedHandling the stop signal and logging the initialization details ensures proper resource management and traceability.
54-59: Passing Knuu instance to TestnetOptionsIncluding the
Knuuinstance inTestnetOptionsallows for flexible test network configuration and aligns with the objective of passing Knuu as an argument.test/e2e/major_upgrade_v2.go (1)
15-15: Importing Knuu package is appropriateThe addition of the Knuu package import is necessary for the new functionality introduced.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
test/e2e/simple.go (1)
24-33: LGTM: Knuu initialization added with proper error handling and logging.The Knuu initialization is well-implemented with a unique identifier, proper error handling, and informative logging. This addition enhances the test setup and provides better control over the test environment.
Consider reviewing the commented-out timeout option. If it's necessary for some scenarios, you might want to make it configurable or document why it's commented out.
test/e2e/testnet/testnet.go (1)
Potential Impact of Removing the
KnuuMethodThe removal of the
Knuumethod from theTestnetstruct may affect existing code that references it. Please review the following locations to ensure that the removal does not introduce issues:
test/e2e/simple.go
- Line 1:
testnet.NoError("failed to initialize Knuu", err)- Line 2:
logger.Println("Knuu initialized", "scope", kn.Scope, "testName", testName)- Line 3:
Knuu: kn,
test/e2e/major_upgrade_v2.go
- Line 1:
testnet.NoError("failed to initialize Knuu", err)- Line 2:
logger.Println("Knuu initialized", "scope", kn.Scope, "testName", testName)- Line 3:
Knuu: kn,
test/e2e/testnet/txsimNode.go
- Line 1:
knuu *knuu.Knuu,
test/e2e/testnet/testnet.go
- Line 1:
knuu *knuu.Knuu- Line 2:
Knuu *knuu.Knuu- Line 3:
knuu: opts.Knuu,
test/e2e/testnet/node.go
- Line 1:
kn *knuu.Knuu,
test/e2e/minor_version_compatibility.go
- Line 1:
testnet.NoError("failed to initialize Knuu", err)- Line 2:
logger.Println("Knuu initialized", "scope", kn.Scope, "testName", testName)- Line 3:
Knuu: kn,🔗 Analysis chain
Line range hint
1-1: Verify the impact of removing the Knuu methodThe
Knuumethod has been removed from theTestnetstruct. While this change aligns with the new approach of passing knuu as an argument, please verify that this removal doesn't negatively impact any existing code that might rely on this method. Consider adding a comment explaining the rationale behind this removal if it's intentional.To check for potential impacts, you can run the following command:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of the Knuu method rg '\bKnuu\(\)' --type goLength of output: 27
Script:
#!/bin/bash # Search for any remaining usage of the Knuu identifier in Go files rg 'Knuu' --type goLength of output: 958
test/e2e/major_upgrade_v2.go (1)
21-21: Avoid hardcoding the test nameWhile defining
testNameas"MajorUpgradeToV2"works, consider deriving it dynamically from the function name or using a constant to prevent potential discrepancies if the function name changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- test/e2e/major_upgrade_v2.go (1 hunks)
- test/e2e/minor_version_compatibility.go (2 hunks)
- test/e2e/simple.go (1 hunks)
- test/e2e/testnet/testnet.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (17)
test/e2e/simple.go (3)
12-12: LGTM: Knuu package import added.The import for the Knuu package is correctly added and is necessary for the subsequent Knuu initialization in the function.
19-20: LGTM: Test name constant added.The addition of the
testNameconstant is a good practice. It improves code readability and maintainability by centralizing the test name definition.
Line range hint
1-91: Summary: E2ESimple function successfully updated to incorporate Knuu.The changes to the
E2ESimplefunction effectively integrate the Knuu package for improved test execution management. Key improvements include:
- Proper initialization of a Knuu instance with a unique identifier.
- Enhanced testnet creation using the new
TestnetOptionsstructure.- Improved logging for better visibility during test execution.
These modifications align well with the PR objectives, enhancing the flexibility and control of the testnet setup while maintaining the core logic of the existing test.
test/e2e/testnet/testnet.go (2)
33-39: LGTM: New TestnetOptions struct improves code organizationThe introduction of the
TestnetOptionsstruct is a good improvement. It encapsulates all the parameters previously passed to theNewfunction, making the code more organized and easier to maintain. This change also aligns with the PR objective of passing knuu as an argument.
41-49: LGTM: New function signature improves flexibility and meets PR objectiveThe changes to the
Newfunction are well-implemented. The new signature, which takes a singleTestnetOptionsparameter, simplifies function calls and improves flexibility for future additions. The function body correctly uses the fields from theTestnetOptionsstruct, including the initialization of theknuufield, which meets the PR objective of passing knuu as an argument.test/e2e/minor_version_compatibility.go (5)
18-18: Added import for Knuu packageThe addition of
github.com/celestiaorg/knuu/pkg/knuuimport integrates Knuu functionalities into the test environment.
22-25: Constants 'testName' and 'numNodes' added for clarityDefining
testNameandnumNodesas constants improves code readability and maintainability by clearly identifying the test's purpose and the number of nodes used.
42-43: Unique identifier for Knuu scopeConstructing the
identifierusing the test name and current timestamp ensures a unique scope for each test run, preventing potential conflicts in test environments.
51-51: Ensure graceful shutdown with 'HandleStopSignal'Calling
kn.HandleStopSignal(ctx)ensures that the Knuu instance handles stop signals appropriately, allowing for a clean and graceful shutdown of the test environment.
54-59: Updated 'testnet.New' call to include Knuu instanceIncorporating the
Knuuinstance intotestnet.Newenhances the flexibility and modularity of the testnet setup, allowing for better management and customization of the test environment.test/e2e/major_upgrade_v2.go (7)
15-15: Importing Knuu packageThe import of
"github.com/celestiaorg/knuu/pkg/knuu"correctly allows the use of the Knuu package for managing test execution.
29-35: Initializing Knuu instance with unique identifierThe Knuu instance is initialized with a unique identifier, which is essential for isolating test environments. The options set, including
ProxyEnabled, are appropriate.
36-36: Error handling after Knuu initializationThe error check using
testnet.NoErrorensures that any issues during Knuu initialization are properly handled.
38-38: Handling stop signals for graceful shutdownUsing
kn.HandleStopSignal(ctx)allows the test to respond gracefully to termination signals, improving test robustness.
39-39: Enhanced logging with Knuu initialization detailsLogging the initialization of Knuu with details about the scope and test name improves traceability and debugging capability.
42-47: Updating testnet creation to include Knuu instancePassing the Knuu instance within
testnet.TestnetOptionswhen creating the testnet ensures that the testnet is aware of the Knuu environment.
42-47: Verify consistency oftestnet.Newusage across the codebaseWith the updated signature of
testnet.Newto acceptTestnetOptions, ensure all other invocations oftestnet.Neware updated accordingly to prevent potential errors.Run the following script to identify all usages of
testnet.New:Review each occurrence to confirm that they pass
TestnetOptionswith the necessary parameters, including the Knuu instance if required.✅ Verification successful
All
testnet.Newinvocations are consistent with the updated signature including theKnuuparameter. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to testnet.New that need to be reviewed. rg --type go --no-heading --line-number 'testnet\.New\(ctx,'Length of output: 333
Script:
#!/bin/bash # Description: Verify that all testnet.New calls include the Knuu parameter in TestnetOptions. ast-grep --lang go --pattern 'testnet\.New($_, testnet\.TestnetOptions{ $$$, Knuu: $_, $$$ })' --invertLength of output: 305
Script:
#!/bin/bash # Description: Verify that all testnet.New calls include the Knuu parameter in TestnetOptions. rg --type go --no-heading --line-number 'testnet\.New\(ctx,\s*testnet\.TestnetOptions\{' | grep -v 'Knuu:'Length of output: 377
Script:
#!/bin/bash # Description: Verify that all testnet.New calls include the Knuu parameter in TestnetOptions. rg --type go --no-heading --line-number 'testnet\.New\(ctx,\s*testnet\.TestnetOptions\{' | grep -v 'Knuu\s*:'Length of output: 382
Script:
#!/bin/bash # Description: Identify all testnet.New calls and check for the presence of the Knuu parameter. # Step 1: Extract all testnet.New calls with their TestnetOptions ast-grep --lang go --pattern 'testnet.New(ctx, testnet.TestnetOptions{ $$$ })' > all_testnet_new_calls.txt # Step 2: Identify calls that do not include the Knuu parameter grep -L 'Knuu:' all_testnet_new_calls.txtLength of output: 122
mojtaba-esk
requested review from
a team and
ninabarbakadze
and removed request for
a team
Thanks for pointing it out. now it is fixed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
test/e2e/major_upgrade_v2.go (1)
29-39: LGTM: Knuu initialization implemented correctlyThe Knuu initialization has been implemented effectively:
- A unique identifier is created for each test run.
- Knuu options are set appropriately, including proxy enablement.
- Error handling is implemented correctly.
- The logging statement has been updated to use Printf as suggested.
Consider uncommenting and configuring the Timeout option if extended test durations are anticipated.
test/e2e/testnet/testnet.go (2)
41-49: LGTM: Improved New function with OptionsThe refactoring of the
Newfunction to use theOptionsstruct simplifies the function signature and improves readability. The changes correctly utilize the fields from theoptsparameter, including theknuufield, which aligns with the PR objective.Consider adding basic validation for required fields in the
Optionsstruct to provide more informative error messages. For example:if opts.Knuu == nil { return nil, errors.New("knuu is required in Options") }
Remaining usages of the
Knuumethod foundThe removal of the
Knuumethod from theTestnetstruct is not fully completed. The following files still contain usages ofKnuu:
test/e2e/simple.gotest/e2e/testnet/txsimNode.gotest/e2e/testnet/node.gotest/e2e/major_upgrade_v2.gotest/e2e/minor_version_compatibility.gotest/e2e/benchmark/benchmark.goPlease update these instances to use the new
knuufield from theTestnetstruct directly.🔗 Analysis chain
Line range hint
1-1: Verify impact of removing Knuu methodThe removal of the
Knuumethod from theTestnetstruct aligns with the new approach of passingknuuas an argument in theOptionsstruct. However, it's crucial to ensure that all previous usages of this method have been appropriately updated throughout the codebase.Please run the following script to check for any remaining usages of the
Knuumethod:If the script returns any results, those occurrences should be updated to use the new
knuufield from theTestnetstruct directly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usages of the Knuu method rg --type go 'Knuu\(\)' --glob '!test/e2e/testnet/testnet.go'Length of output: 63
Script:
#!/bin/bash # Search for any remaining usages of Knuu in the codebase excluding testnet.go rg --type go 'Knuu\b' --glob '!test/e2e/testnet/testnet.go'Length of output: 933
test/e2e/benchmark/benchmark.go (2)
36-37: Clarify Context Usage inHandleStopSignalThe comment indicates that
context.Background()is used to allowHandleStopSignalto function after this function returns. It might be clearer to explain why a new background context is appropriate here, especially sincectxwas created earlier. Consistency in context usage can help prevent confusion.
39-40: Enhance Logging for Better VisibilityThe log message
"Knuu initialized with scope %s"provides useful information. Consider including additional context if available, such as the identifier or any relevant configuration details, to aid in debugging and monitoring.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- test/e2e/benchmark/benchmark.go (2 hunks)
- test/e2e/major_upgrade_v2.go (1 hunks)
- test/e2e/minor_version_compatibility.go (2 hunks)
- test/e2e/simple.go (1 hunks)
- test/e2e/testnet/testnet.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/minor_version_compatibility.go
- test/e2e/simple.go
🧰 Additional context used
🔇 Additional comments (6)
test/e2e/major_upgrade_v2.go (4)
15-15: LGTM: Knuu package import added correctlyThe import for the Knuu package has been added appropriately, aligning with the existing import structure and supporting the new test management features.
21-21: LGTM: Test name variable addedThe
testNamevariable has been appropriately declared and initialized, providing a clear identifier for the test which enhances readability and maintainability.
55-55: LGTM: Logging statement updated as suggestedThe logging statement has been correctly updated to use
Printf, allowing for proper formatting of the log message with variables. This change addresses the suggestion made in a previous review comment.
Line range hint
19-147: Overall changes improve test management while preserving core logicThe modifications to the
MajorUpgradeToV2function successfully integrate Knuu for enhanced test management and update the testnet creation process. These changes provide better control over the test environment and improve code clarity without altering the core upgrade verification logic.Key improvements:
- Integration of Knuu for test management
- Use of structured options for testnet creation
- Improved logging
These changes enhance the maintainability and flexibility of the test setup while preserving its fundamental functionality.
test/e2e/testnet/testnet.go (1)
33-39: LGTM: Well-structured Options structThe introduction of the
Optionsstruct is a good design choice. It encapsulates related parameters, making the code more maintainable and extensible. This change aligns well with the PR objective of passingknuuas an argument and improves overall code organization.test/e2e/benchmark/benchmark.go (1)
41-48: Verify Integration ofKnuuwithtestnet.NewThe
testnet.Newfunction now includes theKnuuoption:testNet, err := testnet.New(testnet.Options{ // ... Knuu: kn, })Ensure that the
testnet.Optionsstruct and thetestnet.Newfunction are updated to accept and utilize theKnuuinstance correctly. Verify that all downstream components handle theKnuufield as expected.To confirm, you can check the implementation of
testnet.Optionsandtestnet.Newto ensure they accommodate the newKnuuparameter.
test/e2e/benchmark/benchmark.go
Outdated
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| identifier := fmt.Sprintf("%s_%s", name, time.Now().Format("20060102_150405")) |
There was a problem hiding this comment.
[optional] this could extract a constant for the time format and reuse that in other places
| func (t *Testnet) Knuu() *knuu.Knuu { | ||
| return t.knuu | ||
| } | ||
|
|
There was a problem hiding this comment.
[question] is this getter no longer helpful?
There was a problem hiding this comment.
Not really as the user passes the knuu object, so they can use it on their end the way they need.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
test/e2e/benchmark/benchmark.go (1)
45-52: LGTM: Improved testnet creation with Options struct and error handling.The use of an Options struct for testnet creation is a good improvement. It enhances code readability and makes it easier to add or modify options in the future. The addition of error handling is also a positive change.
Consider wrapping the error with additional context:
-testnet.NoError("failed to create testnet", err) +testnet.NoError("failed to create testnet", fmt.Errorf("creating testnet with Knuu: %w", err))This change would provide more context about where the error occurred, making debugging easier.
test/e2e/testnet/testnet.go (2)
41-49: Approved: SimplifiedNewfunction withOptionsstructThe updated
Newfunction signature using theOptionsstruct simplifies the API and improves flexibility. The function body correctly utilizes the fields from theOptionsstruct.Consider adding basic validation for the
Optionsstruct fields, such as checking for nil values where appropriate, to improve error handling and provide more informative error messages to the user.
Line range hint
62-65: Removal ofKnuumethod aligns with new designThe removal of the
Knuumethod is consistent with the new approach where the user passes the knuu object directly. This change improves encapsulation and gives users more control over how they use the knuu object.Consider updating the package documentation to reflect this change and provide guidance on how users should now interact with the knuu object.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- test/e2e/benchmark/benchmark.go (2 hunks)
- test/e2e/main.go (1 hunks)
- test/e2e/major_upgrade_v2.go (1 hunks)
- test/e2e/major_upgrade_v3.go (1 hunks)
- test/e2e/minor_version_compatibility.go (2 hunks)
- test/e2e/simple.go (1 hunks)
- test/e2e/testnet/testnet.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/main.go
- test/e2e/major_upgrade_v2.go
- test/e2e/minor_version_compatibility.go
- test/e2e/simple.go
🧰 Additional context used
🔇 Additional comments (10)
test/e2e/benchmark/benchmark.go (2)
15-16: LGTM: New import for Knuu package.The addition of the Knuu package import is appropriate for the new functionality being introduced in this file.
18-20: LGTM: Time format constant added.The addition of the
timeFormatconstant is a good practice. It ensures consistency in time formatting across the package and improves maintainability.test/e2e/testnet/testnet.go (2)
33-39: Excellent addition of theOptionsstruct!The introduction of the
Optionsstruct is a great improvement. It enhances the extensibility and readability of theNewfunction by grouping related parameters. This change will make it easier to add new options in the future without modifying the function signature.
33-49: Overall: Significant improvement in package designThe changes introduced in this PR represent a substantial improvement in the testnet package's design. The new
Optionsstruct and the updatedNewfunction signature enhance flexibility and extensibility while maintaining the core functionality.To ensure a smooth transition, please verify that all other parts of the codebase that use this package have been updated to work with the new API. Run the following script to check for any remaining usages of the old
Newfunction signature:If any results are found, they will need to be updated to use the new
Optionsstruct.✅ Verification successful
Verification Successful: No Remaining Old
NewFunction UsagesAll usages of the old
Newfunction signature have been successfully updated. No remaining instances of the outdatedNewfunction are found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old New function usage rg --type go 'New\s*\(\s*ctx\s*,\s*name\s*,\s*seed\s*,\s*grafana\s*,\s*chainID\s*,\s*genesisModifiers' --glob '!test/e2e/testnet/testnet.go'Length of output: 142
Script:
#!/bin/bash # Search for any usage of the New function in Go files excluding testnet.go rg --type go 'New\s*\(' --glob '!test/e2e/testnet/testnet.go'Length of output: 8982
test/e2e/major_upgrade_v3.go (6)
13-13: Importing theknuupackageThe import statement for
github.com/celestiaorg/knuu/pkg/knuucorrectly adds the necessary package for Knuu integration.
18-21: Initialization of new variables for test configurationThe addition of
testName,numNodes, and the change ofupgradeHeightV3toint64(10)enhances the configurability of the test. This allows for more flexibility in test scenarios.
27-31: Initialization of Knuu instance with context and optionsThe Knuu instance
knis properly initialized with the context and options, includingScopeandProxyEnabled. Error handling after initialization ensures robustness.
33-34: Handling stop signals and logging Knuu initializationUsing
kn.HandleStopSignal(ctx)allows the application to gracefully handle termination signals. Logging the initialization of Knuu with scope enhances observability.
37-41: Updatingtestnet.Newto usetestnet.OptionsstructRefactoring the
testnet.Newfunction to use thetestnet.Optionsstruct improves code clarity and future maintainability. PassingKnuu: knintegrates the Knuu instance into the testnet setup effectively.
20-20: Verify the change ofupgradeHeightV3from 20 to 10The
upgradeHeightV3has been modified fromint64(20)toint64(10). Please ensure that this change aligns with your testing requirements and that the upgrade occurs at the intended block height.To confirm that the upgrade height is correctly set and does not affect other tests, you can verify its usage:
✅ Verification successful
upgradeHeightV3successfully updated to10The change from
int64(20)toint64(10)has been verified across the codebase. No remaining instances ofupgradeHeightV3set toint64(20)were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `upgradeHeightV3` is correctly set to 10 and is used appropriately. # Test: Search for the usage of `upgradeHeightV3` in the codebase. # Expect: Occurrences where `upgradeHeightV3` is set to 10. rg --type go 'upgradeHeightV3' -A 5Length of output: 712
Script:
#!/bin/bash # Description: Check for any occurrences where `upgradeHeightV3` is still set to 20. # Test: Search for assignments of `upgradeHeightV3` to `int64(20)`. # Expect: No matches should be found. rg --type go 'upgradeHeightV3\s*=\s*int64\(20\)'Length of output: 50
test/e2e/testnet/testnet.go
Outdated
|
|
||
| func New(opts Options) (*Testnet, error) { |
There was a problem hiding this comment.
nit: I prefer the use of Options for arguments to a constructor which are in fact optional. All fields here can have working defaults except for the Knuu instance which is mandatory thus I'd prefer
| func New(opts Options) (*Testnet, error) { | |
| func New(knuu *knuu.Knuu, opts Options) (*Testnet, error) { |
so now it's more explicit or better still if you go with
type Option func (*Testnet)
func WithSeed(seed int64) Option {
return func (t *Testnet) {
t.seed = seed
}
}
func New(knuu *knuu.Knuu, options Option...) *TestnetThere was a problem hiding this comment.
yes the term "option" is kinda counter intuitive here. The main reason for using it to reduce the number of arguments to the constructor.
Function option could be another way of using it; it is subjective, but I think option alone is cleaner than than the function option way.
we could extract knuu out of that option thing. a question might arise here: do we have default values for the rest of options if we extract knuu out? if so, I will do that.
There was a problem hiding this comment.
Here's a guess at defaults for the options:
Seed int64 => 42
Grafana *GrafanaInfo => &GrafanaInfo{}
ChainID string => test-chain-id
GenesisModifiers []genesis.Modifier => []genesis.Modifier{}
There was a problem hiding this comment.
ok thanks Rootul, then let's do it
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- test/e2e/testnet/testnet.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
test/e2e/testnet/testnet.go (3)
33-39: LGTM: New Options struct improves code organizationThe introduction of the
Optionsstruct is a good improvement. It encapsulates related parameters, making the code more maintainable and easier to extend in the future. This change also aligns with the PR objective of passingKnuuas an argument.
41-49: LGTM: Validate method enhances error handlingThe
Validatemethod is a valuable addition. It ensures that critical fields (ChainIDandKnuu) are provided before creating aTestnet, which will help prevent potential issues downstream. The error messages are clear and informative.
51-62: LGTM: New function refactored to use Options structThe refactoring of the
Newfunction to use theOptionsstruct is a good improvement. It simplifies the function signature, enhances error handling by validating options upfront, and makes theTestnetinitialization more concise. This change aligns well with the PR objective of enhancing the flexibility of the testnet constructor.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
test/e2e/major_upgrade_v3.go (1)
26-34: LGTM: Knuu initialization improves test managementThe Knuu initialization with a unique identifier enhances test isolation. The error handling and logging additions are good practices.
Consider wrapping the error handling in a helper function for consistency:
func handleError(msg string, err error) { if err != nil { log.Fatalf("%s: %v", msg, err) } } // Usage handleError("failed to initialize Knuu", err)test/e2e/benchmark/benchmark.go (1)
45-51: LGTM: Testnet creation with Knuu and improved error handlingThe changes to the testnet creation, including the use of the Knuu instance and additional options, align well with the PR objectives. The error handling for testnet creation is good, but could be slightly improved for consistency.
Consider updating the error handling to match the style used for Knuu initialization:
-testnet.NoError("failed to create testnet", err) +if err != nil { + return nil, fmt.Errorf("failed to create testnet: %w", err) +}This change would provide more consistent error handling throughout the function.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- test/e2e/benchmark/benchmark.go (2 hunks)
- test/e2e/benchmark/throughput.go (0 hunks)
- test/e2e/main.go (1 hunks)
- test/e2e/major_upgrade_v2.go (1 hunks)
- test/e2e/major_upgrade_v3.go (1 hunks)
- test/e2e/minor_version_compatibility.go (2 hunks)
- test/e2e/simple.go (1 hunks)
- test/e2e/testnet/testnet.go (3 hunks)
💤 Files with no reviewable changes (1)
- test/e2e/benchmark/throughput.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/main.go
- test/e2e/major_upgrade_v2.go
- test/e2e/minor_version_compatibility.go
- test/e2e/simple.go
🧰 Additional context used
🔇 Additional comments (9)
test/e2e/major_upgrade_v3.go (4)
13-13: LGTM: Knuu package import addedThe addition of the Knuu package import is consistent with the changes in the function body and supports the new test execution management approach.
Line range hint
39-114: LGTM: Overall changes improve test management while preserving core logicThe modifications to incorporate Knuu improve test execution management without altering the core upgrade test logic. The unchanged parts of the function remain consistent with the new changes.
As a final verification step, please run the following script to ensure all Knuu-related changes are consistent throughout the file:
#!/bin/bash # Check for any remaining references to the old testnet creation method # and verify Knuu usage consistency rg --type go 'testnet\.New\(' test/e2e/ rg --type go 'knuu\.' test/e2e/
37-37: LGTM: Testnet creation now uses KnuuThe change aligns with the PR objective of passing Knuu as an argument to
testnet.New. The use oftestnet.Optionsmight provide more flexibility in testnet configuration.Please verify that all necessary options are being passed to
testnet.New. Run the following script to check thetestnet.Optionsstruct definition and usage:
17-21: Verify the impact of reducing upgradeHeightV3The addition of
testNameimproves code readability. However, theupgradeHeightV3has been reduced from 20 to 10.Please confirm that this change doesn't negatively impact the test scenario. Run the following script to check for any related configurations or dependencies:
test/e2e/benchmark/benchmark.go (3)
15-20: LGTM: New import and constant addedThe addition of the Knuu package import and the
timeFormatconstant are appropriate for the new functionality. The constant improves code readability and maintainability.
40-43: LGTM: Stop signal handling and logging addedThe addition of stop signal handling using a background context is correct and ensures proper functionality even after the function returns. The logging statement provides useful information about the Knuu initialization, which is good for debugging and monitoring.
28-38:⚠️ Potential issueRemove deferred context cancellation
The creation of a context with cancellation and the initialization of the Knuu instance look good. However, the deferred cancellation of the context on line 29 could lead to premature termination of Knuu operations, especially since
HandleStopSignalis called with a background context later.ctx, cancel := context.WithCancel(context.Background()) -defer cancel()The rest of the changes, including the unique identifier generation and Knuu initialization, are well-implemented.
test/e2e/testnet/testnet.go (2)
23-26: ConstantsDefaultSeedandDefaultChainIDare appropriately defined.Defining
DefaultSeedandDefaultChainIDenhances code maintainability and provides clear default values for theTestnetconfiguration.
46-54:Newfunction signature is well-structured and follows best practices.Passing
knuuas a mandatory parameter and grouping optional parameters into theOptionsstruct enhances the readability and usability of theNewfunction.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
test/e2e/testnet/testnet.go (1)
38-43: LGTM: Options struct improves parameter management.The
Optionsstruct enhances code organization and flexibility. However, consider adding theKnuufield to this struct as suggested in a previous review comment.Consider adding the
Knuufield to theOptionsstruct:type Options struct { Seed int64 Grafana *GrafanaInfo ChainID string GenesisModifiers []genesis.Modifier + Knuu *knuu.Knuu }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- test/e2e/testnet/testnet.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
test/e2e/testnet/testnet.go (2)
23-26: LGTM: Default constants added for seed and chain ID.The addition of
DefaultSeedandDefaultChainIDconstants improves code readability and maintainability by centralizing default values.
45-54: LGTM: New function signature improves parameter handling.The updated
Newfunction effectively uses theOptionsstruct and separates theknuuparameter. ThesetDefaultscall ensures proper initialization of default values.
| // context.Background() is used to allow the stopSignal to be functioning even after this function returns | ||
| kn.HandleStopSignal(context.Background()) | ||
|
|
||
| log.Printf("Knuu initialized with scope %s", kn.Scope) |
There was a problem hiding this comment.
[nit] I would move this log statement immediately beneath the Knuu initialization command (and error handling):
kn, err := knuu.New(ctx, knuu.Options{
Scope: identifier,
ProxyEnabled: true,
})
if err != nil {
return nil, err
}
log.Printf("Knuu initialized with scope %s", kn.Scope)| testnet.NoError("failed to initialize Knuu", err) | ||
|
|
||
| kn.HandleStopSignal(ctx) | ||
| logger.Printf("Knuu initialized with scope %s", kn.Scope) |
There was a problem hiding this comment.
[nit] same nit about moving this immediately after knuu initialization.
There was a problem hiding this comment.
it is after knuu initialization. knuu.New() does the initialization and if it does not fail (error check), then we handle the stop signal.
or you mean removing that empty line?
There was a problem hiding this comment.
I mean moving the log statement.
kn, err := knuu.New(ctx, knuu.Options{
Scope: scope,
ProxyEnabled: true,
})
testnet.NoError("failed to initialize Knuu", err)
+ logger.Printf("Knuu initialized with scope %s", kn.Scope)
kn.HandleStopSignal(ctx)
- logger.Printf("Knuu initialized with scope %s", kn.Scope)| testnet.NoError("failed to initialize Knuu", err) | ||
|
|
||
| kn.HandleStopSignal(ctx) | ||
| logger.Printf("Knuu initialized with scope %s", kn.Scope) |
There was a problem hiding this comment.
[nit] same nit about moving this up
|
Oops nvm I can't push to your fork until you add me as a collaborator. cc: @mojtaba-esk |
|
Approving to unblock. I can address nits in a follow-up if this merges before I get permission to push my commit that addresses nits. |
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
test/e2e/major_upgrade_v2.go (1)
20-22: LGTM: Test configuration variables addedThe addition of these variables improves code readability and maintainability. The values seem appropriate for the test scenario.
Consider using inline variable declarations for consistency with the codebase style:
testName := "MajorUpgradeToV2" numNodes := 4 upgradeHeight := int64(10)test/e2e/benchmark/benchmark.go (2)
18-18: Consider removing the const blockThe
timeFormatconstant is correctly defined and used in theNewBenchmarkTestfunction. However, as suggested in a previous review, since there's only one constant, you can remove theconstblock:const timeFormat = "20060102_150405"This change would slightly improve readability.
38-41: LGTM: Stop signal handling and loggingThe stop signal handling with a background context is correctly implemented. The log statement provides good observability for the Knuu initialization.
Consider moving the log statement immediately after the Knuu initialization for better code flow:
kn, err := knuu.New(ctx, knuu.Options{ Scope: scope, ProxyEnabled: true, }) if err != nil { return nil, err } log.Printf("Knuu initialized with scope %s", kn.Scope) // context.Background() is used to allow the stopSignal to be functional even after this function returns kn.HandleStopSignal(context.Background())This change would improve the logical grouping of related operations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- test/e2e/benchmark/benchmark.go (1 hunks)
- test/e2e/major_upgrade_v2.go (1 hunks)
- test/e2e/major_upgrade_v3.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/major_upgrade_v3.go
🧰 Additional context used
🔇 Additional comments (5)
test/e2e/major_upgrade_v2.go (3)
15-15: LGTM: Knuu package import addedThe addition of the Knuu package import is appropriate for the changes made in this file.
38-38: LGTM: Testnet creation updatedThe update to use
testnet.Optionsstructure improves the clarity of the testnet creation process. The Knuu instance is correctly passed as an argument, and the previously undefinedseedparameter is no longer used.
46-46: LGTM: Logging statement updatedThe logging statement has been improved to include the test name, enhancing the clarity of the log output. The
Printffunction is correctly used for formatting.test/e2e/benchmark/benchmark.go (2)
15-15: LGTM: Knuu package import addedThe addition of the Knuu package import is consistent with the changes in the
NewBenchmarkTestfunction and is correctly implemented.
43-48: LGTM: Enhanced testnet creation with KnuuThe updated testnet creation using
testnet.Newwith Knuu and additional options is well-implemented. This change aligns perfectly with the PR objective of enhancing the flexibility of the testnet constructor by allowing the "knuu" parameter to be passed as an argument.The error handling for testnet creation is also correctly implemented, improving the robustness of the function.
These modifications successfully achieve the goal of providing users with more control over the "knuu" options, thereby improving the overall usability and efficiency of the testnet setup.
| const ( | ||
| testName = "MinorVersionCompatibility" | ||
| numNodes = 4 | ||
| ) |
There was a problem hiding this comment.
| const ( | |
| testName = "MinorVersionCompatibility" | |
| numNodes = 4 | |
| ) | |
| testName := "MinorVersionCompatibility" | |
| numNodes = 4 |
| identifier := fmt.Sprintf("%s_%s", testName, time.Now().Format(timeFormat)) | ||
| kn, err := knuu.New(ctx, knuu.Options{ | ||
| Scope: identifier, | ||
| ProxyEnabled: true, | ||
| }) |
There was a problem hiding this comment.
| identifier := fmt.Sprintf("%s_%s", testName, time.Now().Format(timeFormat)) | |
| kn, err := knuu.New(ctx, knuu.Options{ | |
| Scope: identifier, | |
| ProxyEnabled: true, | |
| }) | |
| scope := fmt.Sprintf("%s_%s", testName, time.Now().Format(timeFormat)) | |
| kn, err := knuu.New(ctx, knuu.Options{ | |
| Scope: scope, | |
| ProxyEnabled: true, | |
| }) |
mojtaba-esk
deleted the
mojtaba/pass-knuu-as-an-argument-to-the-testnet.New
branch
|
@mojtaba-esk for future reference, this was an api breaking PR, so we should put |
|
also, as a broad question, why were we able to merge this PR when only a single core app engineer approved? |
As pointed out before in one of the comments, if we pass knuu as an argument to the testnet constructor, user will have more flexibility in playing with knuu options and can optimize the resource consumption based on the test requirements.
This PR attempts to achieve that goal.