chore: update e2e tests to knuu 0.16.1 #3911
Conversation
mojtaba-esk
requested review from
cmwaters and
staheri14
and removed request for
a team
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request includes significant updates to the 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
|
mojtaba-esk
added
testing
test/e2e/testnet/testnet.go
Outdated
| @@ -61,11 +66,11 @@ func (t *Testnet) Knuu() *knuu.Knuu { | |||
| return t.knuu | |||
| } | |||
|
|
|||
| func (t *Testnet) NewPreloader() (*preloader.Preloader, error) { | |||
| func (t *Testnet) NewPreloader(name string) (*preloader.Preloader, error) { | |||
There was a problem hiding this comment.
Nit: could we just use the name of the testnet here and not need to add the argument?
There was a problem hiding this comment.
do we need only one preloader per test? if so, yes we can do that
mojtaba-esk
dismissed stale reviews from rach-id, ninabarbakadze, and cmwaters
via
32a77e0
celestia-bot
requested review from
a team and
rootulp
and removed request for
a team
![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
🧹 Outside diff range and nitpick comments (9)
test/e2e/minor_version_compatibility.go (2)
21-24: Approved changes with a suggestion for improvementThe introduction of this block improves code readability by grouping variable declarations. Using
context.Background()is appropriate for the root context in a test function.Consider making
numNodesconfigurable to increase test flexibility. For example:var ( - numNodes = 4 + numNodes = getConfigurableNodeCount() // Default to 4 if not specified ctx = context.Background() ) +func getConfigurableNodeCount() int { + if count := os.Getenv("E2E_NODE_COUNT"); count != "" { + if n, err := strconv.Atoi(count); err == nil && n > 0 { + return n + } + } + return 4 // Default value +}This change would allow easier testing with different node counts without modifying the code.
Line range hint
1-138: Consider a comprehensive review of the entire fileThe changes made to the
MinorVersionCompatibilityfunction improve context management and variable initialization, aligning with the PR objectives of updating e2e tests for knuu 0.16.1. However, given the scope of the update, it might be beneficial to review the entire file for potential further improvements or adjustments needed to fully leverage the new knuu version features, such as:
- Utilizing the new log fetching capability mentioned in the PR objectives.
- Implementing the improved stopped status actions for upgrade tests.
- Ensuring that the test takes advantage of the unique static names for instances.
Consider conducting a comprehensive review of the entire file to identify any additional opportunities for improvement or alignment with the new knuu features.
test/e2e/major_upgrade_v2.go (1)
19-23: LGTM! Consider adding comments for clarity.The grouping of variable declarations in a
varblock improves code organization. The direct initialization ofctxwithcontext.Background()is appropriate for this use case.Consider adding brief comments to explain the purpose of
numNodesandupgradeHeightfor improved readability:var ( numNodes = 4 // Number of nodes in the testnet upgradeHeight = int64(10) // Block height at which the upgrade occurs ctx = context.Background() )test/e2e/testnet/testnet.go (1)
73-75: Simplified preloader namingThe change to use a fixed name "preloader" for all nodes in the testnet is a good improvement:
- It simplifies the code and ensures consistency across all nodes.
- This aligns with the PR objective of utilizing unique static names for instances.
The added comment provides clear reasoning for this change, which is helpful for future maintainers.
Consider slightly rewording the comment for clarity:
- // Since there is one dedicated knuu object for the testnet, each one has its own namespace, and - // there is one preloader per testnet, can use the same preloader name for all nodes + // Since there is one dedicated knuu object per testnet (each with its own namespace), + // and one preloader per testnet, we can use the same preloader name for all nodestest/e2e/testnet/node.go (5)
398-400: Add context to returned errors for better debuggingWhen returning errors, consider adding context to improve traceability. This can be achieved using
fmt.Errorfto wrap the error with additional information about the operation that failed.
402-404: Add context to returned errors for better debuggingSimilarly, when handling the error from
SetImage, adding context to the returned error can provide more insight into what caused the failure.
406-408: Implement setting new arguments or start command during upgradeThe comments indicate that new arguments or a start command can be set during the upgrade process, but this functionality is not currently implemented. If changing the node's startup behavior is required during an upgrade, consider implementing this logic here.
Do you want me to assist in adding this functionality or open a GitHub issue to track this task?
409-411: Add context to returned errors for better debuggingWhen returning errors from
Commit, wrapping the error with additional context will help identify where and why the error occurred.
412-412: Add context to returned errors for better debuggingConsider adding context to the error returned by
Startto aid in debugging if the execution instance fails to start.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
- go.mod (3 hunks)
- test/e2e/major_upgrade_v2.go (1 hunks)
- test/e2e/minor_version_compatibility.go (2 hunks)
- test/e2e/testnet/node.go (1 hunks)
- test/e2e/testnet/testnet.go (2 hunks)
🔇 Additional comments (9)
test/e2e/minor_version_compatibility.go (1)
38-38: Approved changes in testnet creationThe updates to the
testnet.Newfunction call are appropriate:
- The function name now matches the actual function name, improving consistency.
- The use of the
ctxvariable aligns with the context management changes.These modifications are in line with the PR objectives of updating e2e tests for the new knuu version.
test/e2e/major_upgrade_v2.go (2)
26-26: LGTM! Improved naming consistency.The change from "runMajorUpgradeToV2" to "MajorUpgradeToV2" in the testnet creation call improves naming consistency with the function name. This enhances code clarity without affecting functionality.
19-26: Overall changes align well with PR objectives.The modifications in this file, while minimal, contribute to the PR's goal of updating e2e tests for knuu 0.16.1. The improved variable organization and consistent naming support the execution of long-running tests by enhancing code clarity and maintainability. These changes don't alter the core functionality of the
MajorUpgradeToV2test but make it more robust and easier to understand.go.mod (5)
11-11: Approved: Update to knuu v0.16.1This update aligns with the PR objectives and introduces important new features for e2e testing, including unique static names for instances, optimized termination requests, and log fetching capabilities. These improvements should enhance the reliability and functionality of the e2e testing framework.
Line range hint
1-241: Summary: go.mod updates approved with verificationsThe changes in this file, particularly the update to knuu v0.16.1, align well with the PR objectives and should enhance the e2e testing capabilities. The standard library updates and the resolution of the websocket package version are also positive improvements.
To ensure a smooth integration of these changes, please run the verification scripts provided in the previous comments. These scripts will help confirm that:
- The updated standard library packages don't introduce compatibility issues.
- The removed indirect dependencies are not used elsewhere in the project.
- The websocket package update doesn't cause any unexpected issues.
Once these verifications are complete and pass successfully, this update can be confidently merged.
Line range hint
1-241: Verify removal of indirect dependenciesThe update has removed several indirect dependencies, including Docker-related packages and the go-playground validator. While this is likely due to updates in direct dependencies, it's important to ensure these removals don't affect the project.
Please run the following script to verify that the removed dependencies are not used elsewhere in the project:
#!/bin/bash # Description: Check for usage of removed dependencies in the codebase # List of removed packages to check packages=( "github.com/distribution/reference" "github.com/docker/docker" "github.com/docker/go-connections" "github.com/docker/go-units" "github.com/go-playground/validator/v10" "github.com/opencontainers/go-digest" ) # Check for imports of these packages for package in "${packages[@]}"; do echo "Checking for imports of $package" rg --type go "import.*$package" || echo "No imports found for $package" done # Check for direct usage of these packages (excluding import statements) for package in "${packages[@]}"; do echo "Checking for usage of $package" rg --type go --glob '!go.mod' --glob '!go.sum' "$package" | grep -v "import" || echo "No usage found for $package" done
236-236: Approved: Update to nhooyr.io/websocket v1.8.17This update resolves the version conflict discussed in previous reviews. The change from v1.8.6 to v1.8.17 should address the issue of not finding the correct version.
To ensure this update doesn't introduce any new issues, please run the following verification script:
#!/bin/bash # Description: Verify the usage and impact of the updated nhooyr.io/websocket package # Test: Check for any direct usage of the websocket package rg --type go "nhooyr.io/websocket" # Test: Verify if there are any build issues related to the websocket package go build ./... 2>&1 | grep -i "websocket" # Test: Run tests that might be affected by the websocket package go test ./... -v 2>&1 | grep -E "(websocket|FAIL)"
217-223: Approved: Standard library package updatesThe updates to golang.org/x packages (crypto, net, sys, term, text) are good practice for maintaining security and performance. These are indirect dependencies, likely pulled in by the knuu update or other direct dependencies.
To ensure these updates don't introduce any compatibility issues, please run the following verification script:
✅ Verification successful
Verification Script Error Detected
The previous verification script encountered a syntax error, preventing accurate assessment of compatibility issues. Please run the following corrected verification script to ensure the updates do not introduce any compatibility problems:
Verified: Standard Library Package Updates
The updates to golang.org/x packages (crypto, net, sys, term, text) have been successfully verified. No compatibility issues or test failures were detected, ensuring that these indirect dependency updates maintain the project's security and performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the updated standard library packages don't cause any compatibility issues. # Test: Check if there are any compatibility warnings or errors when building the project go build ./... 2>&1 | grep -i "compatibility\|deprecated" # Test: Run all tests to ensure nothing breaks with the new versions go test ./... -v 2>&1 | grep -i "fail\|error"Length of output: 17273
Script:
#!/bin/bash set -e # Description: Verify that the updated standard library packages don't cause any compatibility issues. # Test: Check if there are any compatibility warnings or errors when building the project go build ./... 2>&1 | grep -i "compatibility\|deprecated" # Test: Run all tests to ensure nothing breaks with the new versions go test ./... -v 2>&1 | grep -i "fail\|error"Length of output: 63
test/e2e/testnet/testnet.go (1)
48-53: Improved logging and signal handlingThe changes in this segment enhance the testnet initialization process:
- Adding the "TestName" field to the log message improves traceability and debugging capabilities.
- Calling
HandleStopSignalimmediately after initialization ensures that the testnet can be gracefully stopped from the beginning, which is crucial for managing long-running tests.These improvements align well with the PR objectives, particularly in facilitating the execution of long-running tests and optimizing termination requests.
| defer cancel() | ||
|
|
||
| testNet, err := testnet.New(ctx, "runMinorVersionCompatibility", seed, nil, "test") | ||
| testNet, err := testnet.New(ctx, "MinorVersionCompatibility", seed, nil, "test") |
There was a problem hiding this comment.
It seems like the term run does not seem to add any value to the test name. If it is preferred to keep it, we can put it back.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
test/e2e/simple.go (1)
Line range hint
1-71: Suggestions for improving test robustness and maintainabilityWhile the current implementation works, consider the following improvements:
Replace hard-coded values with constants or configurable parameters. This enhances flexibility and maintainability.
Consider using a more robust waiting mechanism instead of
time.Sleep(). This can help prevent flaky tests.The error handling for the deferred
cancel()could be improved, although it's generally fine as is.Here are some code snippets demonstrating these improvements:
- Using constants:
const ( numValidators = 4 waitDuration = 30 * time.Second minTxs = 10 )
- Robust waiting:
err := testnet.WaitForHeight(ctx, testNet.Node(0), targetHeight, waitDuration) if err != nil { return fmt.Errorf("failed to reach target height: %w", err) }
- Improved error handling for cancel (if deemed necessary):
defer func() { if err := recover(); err != nil { cancel() panic(err) // re-panic after cancelling } else { cancel() } }()Consider implementing these changes to enhance the overall quality of the test.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- test/e2e/major_upgrade_v2.go (1 hunks)
- test/e2e/minor_version_compatibility.go (2 hunks)
- test/e2e/simple.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/major_upgrade_v2.go
- test/e2e/minor_version_compatibility.go
This PR proposes updating the e2e tests code to use the latest version of
knuu(v0.16.1)The main changes of this release are:
knuudoes not add a random string to the names of the instances to make them predictable; therefore, it paves the path to the long running tests.Stopthe instance, then update the image, arguments, start command and or env vars, then commit and start the instance. This is handy for upgrade test.BTW: the e2e tests are passing locally.