feat(node/test): add websocket endpoint to sysgo tests

[theochap]

Description

Adapts the websocket endpoint tests to the devstack

[claude]

Claude finished @theochap's task —— View job

---

PR Review: feat(node/test): add websocket endpoint to sysgo tests

Todo List:

• Analyze changed files and understand scope of modifications
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage and testing approach
• Provide comprehensive feedback

---

Review Summary

This PR refactors the websocket-based testing infrastructure by migrating from Kurtosis-based tests to devstack-based tests. The changes demonstrate a well-structured approach to consolidating test utilities and improving maintainability.

Positive Aspects

🚀 Code Organization & Architecture

• Excellent consolidation: Moving from Kurtosis-specific tests to generic common/ utilities makes the testing framework more maintainable
• Clean abstraction: ws.go:50-128 provides a well-designed generic websocket testing framework with proper type parameterization
• Proper separation of concerns: Tests are focused on specific behaviors (engine queue, sync safety, etc.)

🛡️ Robust Error Handling

• Comprehensive error checking: All WebSocket operations include proper error handling with descriptive messages
• Graceful cleanup: ws.go:110-117 properly unsubscribes and closes connections
• Context awareness: Tests respect context cancellation for clean shutdowns

⚡ Performance & Concurrency

• Efficient goroutine management: Uses buffered channels and proper WaitGroup patterns
• Non-blocking reads: ws.go:74-83 implements proper non-blocking WebSocket reads with error channels
• Reasonable timeouts: Uses appropriate timeout constants (10s, 60s, 4 minutes) based on operation type

Areas for Improvement

🐛 Potential Race Conditions

File: sync_ws_test.go:88-104

for _, block := range output {
    for _, node := range nodes {
        // Race condition: node sync status checked before block validation
        otherCLSyncStatus := node.ChainSyncStatus(out.L2Chain.ChainID(), types.LocalUnsafe)
        if otherCLSyncStatus.Number < block.Number {
            continue // Skip check, but this could miss real issues
        }

Issue: The sync status check and block validation are not atomic, potentially leading to false negatives in a rapidly changing environment.

Recommendation: Consider adding a small retry mechanism or tolerance window for sync status checks.

🔒 Error Handling Edge Cases

File: ws.go:99-107

case err := <-readJSON(conn, &msg):
    require.NoError(t, err, "read: %v", err)
    var p push[Out]
    require.NoError(t, json.Unmarshal(msg, &p), "decode: %v", err)

Issue: If WebSocket connection fails, the test fails immediately rather than attempting reconnection or providing diagnostic information.

Recommendation: Consider adding connection health checks and more informative error messages for debugging.

🧪 Test Determinism

File: sync_ws_test.go:20-24

// For this test to be deterministic it should...
// 1. Only have two L2 nodes  
// 2. Only have one DA layer node

Issue: Test comments indicate determinism requirements, but the test doesn't validate these preconditions.

Recommendation: Add runtime assertions to verify the network topology matches the test assumptions.

Security Assessment ✅

• No security vulnerabilities detected
• Proper input validation on JSON RPC responses
• Safe WebSocket handling with timeouts and proper cleanup
• No secrets or sensitive data exposed

Test Coverage Analysis

✅ Strong Coverage

• Multiple sync scenarios: unsafe, safe, finalized head synchronization
• Engine queue monitoring: Validates queue sizes stay within expected bounds
• Cross-node validation: Ensures consistency across all nodes in the network
• Timeout handling: Tests handle various timeout scenarios appropriately

🔍 Coverage Gaps

• Error scenarios: Limited testing of WebSocket connection failures or malformed responses
• Network partition recovery: No tests for node recovery after network issues
• Large message handling: No tests for handling large websocket payloads

Performance Considerations

✅ Efficient Design

• Concurrent testing: Tests run in parallel across nodes using goroutines
• Bounded resource usage: Proper cleanup prevents resource leaks
• Reasonable timeouts: Well-calibrated timeouts for different operations

⚠️ Potential Optimizations

• WebSocket reuse: Consider connection pooling for multiple test methods
• Batch operations: Some individual checks could be batched for better performance

Dependencies & Integration

✅ Clean Dependencies

• Appropriate imports: Uses established patterns from optimism devstack
• Version consistency: go.mod updates look appropriate (line 6, 285)
• No unnecessary dependencies: Removed unused Kurtosis dependencies

Final Verdict

APPROVE with minor suggestions ✅

This is a solid refactoring that improves code organization and maintainability. The websocket testing framework is well-designed and the test scenarios are comprehensive. The suggested improvements are minor and don't block the core functionality.

Recommendations for Follow-up

1. Consider adding retry logic for sync status checks
2. Add network topology validation for deterministic tests
3. Consider adding tests for WebSocket error scenarios
4. Document the expected network configuration for each test

---

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.9%. Comparing base (f10e7ce) to head (a5df68e).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull Request Overview

This PR adapts websocket endpoint tests from a Kurtosis-specific implementation to work with the devstack testing environment. The main goal is to make websocket tests more broadly compatible across different testing setups.

• Refactored websocket testing utilities from Kurtosis-specific to general devstack compatibility
• Simplified test implementations by using devstack DSL methods instead of direct RPC calls
• Removed Kurtosis-specific constraints and dependencies

Reviewed Changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/node/transfers/tx_inclusion_test.go	Changed test to run serially instead of in parallel
tests/node/kurtosis/mod.go	Removed Kurtosis-specific websocket utilities
tests/node/kurtosis/init_test.go	Removed Kurtosis test initialization
tests/node/kurtosis/engine_test.go	Removed Kurtosis-specific engine test
tests/node/common/ws.go	Added general websocket utilities for devstack
tests/node/common/sync_ws_test.go	Refactored sync tests to use devstack DSL
tests/node/common/engine_test.go	Added simplified engine test for devstack
tests/node/common/cpu_monitor_test.go	Updated package name
tests/go.mod	Updated dependency version
crates/node/rpc/src/ws.rs	Removed initial state updates in websocket subscriptions
.github/workflows/node_e2e_kurtosis_tests.yaml	Removed Kurtosis-specific test step

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}