Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e085998 to
adf8e72
Compare
456cdb7 to
70bb69a
Compare
15d28cf to
efbb929
Compare
70bb69a to
6fe1f53
Compare
76e2fa6 to
3542838
Compare
c4d9de7 to
33b85c5
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive connection drop tests to verify network behavior when nodes lose connectivity to their peers. The tests simulate connection drops by blacklisting peers and validate synchronization behavior for different node types (sequencers vs non-sequencers).
- Adds three new test functions to validate connection drop scenarios
- Implements utility functions for stopping/starting Kurtosis services
- Fixes function naming inconsistencies in WebSocket helpers
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| tests/node/conn_drops_test.go | New test file implementing connection drop test scenarios |
| tests/node/mod.go | Adds service start/stop utilities and fixes WebSocket function naming |
| tests/node/sync_ws_test.go | Updates function calls to use corrected WebSocket helper naming |
| tests/node/engine_test.go | Removes unused parameter from supportsDevRPC function call |
tests/node/mod.go
Outdated
| } | ||
|
|
||
| // Stop the service | ||
| if err := exec.Command("kurtosis", "service", "stop", |
There was a problem hiding this comment.
Using exec.Command with hardcoded command paths can be a security risk. Consider validating that the 'kurtosis' command exists in PATH and is the expected binary to prevent command injection.
| if err := exec.Command("kurtosis", "service", "stop", | |
| kurtosisPath, err := exec.LookPath("kurtosis") | |
| if err != nil { | |
| return fmt.Errorf("kurtosis binary not found in PATH: %w", err) | |
| } | |
| if err := exec.Command(kurtosisPath, "service", "stop", |
tests/node/mod.go
Outdated
| continue | ||
| } | ||
|
|
||
| if err := exec.Command("kurtosis", "service", "start", |
There was a problem hiding this comment.
Using exec.Command with hardcoded command paths can be a security risk. Consider validating that the 'kurtosis' command exists in PATH and is the expected binary to prevent command injection.
| continue | ||
| } | ||
|
|
||
| t.Log("testing conn drops for node %s", node.Escape().ID().Key()) |
There was a problem hiding this comment.
The log statement uses printf-style formatting with %s but calls t.Log which doesn't support format specifiers. Use t.Logf instead or remove the format specifier.
| t.Log("testing conn drops for node %s", node.Escape().ID().Key()) | |
| t.Logf("testing conn drops for node %s", node.Escape().ID().Key()) |
| continue | ||
| } | ||
|
|
||
| t.Log("testing conn drops for node %s", node.Escape().ID().Key()) |
There was a problem hiding this comment.
The log statement uses printf-style formatting with %s but calls t.Log which doesn't support format specifiers. Use t.Logf instead or remove the format specifier.
| t.Log("testing conn drops for node %s", node.Escape().ID().Key()) | |
| t.Logf("testing conn drops for node %s", node.Escape().ID().Key()) |
|
|
||
| // Check that the engine task count is correct | ||
| clRPC, err := GetNodeRPCEndpoint(t.Ctx(), &node) | ||
| t.Require().NoError(err, "failed to get RPC endpoint for node %s", node.Escape().ID().Key()) |
There was a problem hiding this comment.
The error message uses printf-style formatting with %s but the NoError method may not support format specifiers in this context. Consider using fmt.Sprintf to format the message properly.
| t.Require().NoError(err, "failed to get RPC endpoint for node %s", node.Escape().ID().Key()) | |
| t.Require().NoError(err, fmt.Sprintf("failed to get RPC endpoint for node %s", node.Escape().ID().Key())) |
tests/node/mod.go
Outdated
| } | ||
|
|
||
| // StopNode stops a specific l2cl node. | ||
| func StopNode(ctx context.Context, node *dsl.L2CLNode) error { |
There was a problem hiding this comment.
StopNode and StartNode functions have identical logic for service lookup. Consider extracting the common service lookup logic into a helper function to reduce code duplication.
tests/node/mod.go
Outdated
| // StopKurtosisService stops a specific service in a Kurtosis enclave | ||
| func StopKurtosisService(ctx context.Context, serviceName string) error { |
There was a problem hiding this comment.
StopKurtosisService and StartKurtosisService have nearly identical implementations with only the command differing. Consider creating a generic function that accepts the action as a parameter to reduce code duplication.
| // StopKurtosisService stops a specific service in a Kurtosis enclave | |
| func StopKurtosisService(ctx context.Context, serviceName string) error { | |
| // ManageKurtosisService performs the specified action on a service in a Kurtosis enclave | |
| func ManageKurtosisService(ctx context.Context, serviceName string, action string) error { |
7884e28 to
454551f
Compare
454551f to
1c2fc01
Compare
3124483 to
146c137
Compare
## Description This PR adds back the connection drop test and the restart test inside our e2e testing suite See op-rs/kona#2554 and op-rs/kona#2553
Description
Test what happens when the connection to the other nodes of the network drops (or gets cut off from blacklisting)