Skip to content
This repository was archived by the owner on Jan 16, 2026. It is now read-only.

feat(node/e2e-tests): add restart tests#2747

Merged
theochap merged 2 commits intotheo/conn-drop-testfrom
theo/restart-tests
Aug 22, 2025
Merged

feat(node/e2e-tests): add restart tests#2747
theochap merged 2 commits intotheo/conn-drop-testfrom
theo/restart-tests

Conversation

@theochap
Copy link
Member

Description

This PR adds restart tests for the kona node

@theochap theochap self-assigned this Aug 21, 2025
@theochap theochap added M-tests Meta: Testing related A-node Area: cl node (eq. Go op-node) handles single-chain consensus labels Aug 21, 2025
@theochap theochap moved this to In Review in Project Tracking Aug 21, 2025
@claude
Copy link
Contributor

claude bot commented Aug 21, 2025

Claude finished @theochap's task —— View job


Code Review Complete

I've reviewed the restart tests implementation and have the following feedback:

Code Quality & Best Practices

  • Clean, well-structured test following Go testing conventions
  • Good use of goroutines with sync.WaitGroup for parallel testing
  • Proper error handling with t.Require() assertions
  • Clear variable naming and logical flow
  • Consistent with existing test patterns in the codebase

Test Logic & Coverage

The restart test (TestRestartSync) provides comprehensive coverage:

  • Tests multiple validator nodes in parallel
  • Verifies node shutdown (stops service, checks disconnection, validates RPC errors)
  • Tests restart recovery (starts service, reconnects to sequencer)
  • Validates synchronization after restart (checks LocalSafe/LocalUnsafe matching)
  • Confirms peer connectivity post-restart

Potential Issues ⚠️

  1. Hard-coded timeout: Line 53 uses time.Sleep(2 * time.Minute) which could make tests slow. Consider making this configurable or reducing for CI environments.

  2. Race condition potential: The peer disconnection check (lines 41-44) happens immediately after node.Stop(). There might be a brief race condition where the sequencer hasn't yet updated its peer list.

  3. Limited error context: Line 51 only checks that an RPC call returns an error but doesn't validate the specific error type. A connection refused error vs other errors could be distinguished.

Comparison with Existing Code

The restart test follows the same pattern as conn_drop_test.go:

  • Uses identical imports and test structure
  • Leverages same helper functions (GetNodeRPCEndpoint, peer management)
  • Consistent error handling and logging patterns

Minor Improvements

  • Consider extracting the 2-minute timeout as a constant
  • Could add a brief retry/wait loop for peer disconnection verification instead of assuming immediate effect

Dependency Updates

The go.mod changes look appropriate:

  • Optimism dependency update to newer version
  • Go version update to 1.24.3
  • No suspicious or unnecessary additions

Security & Performance

  • No security concerns identified
  • Performance impact is reasonable for integration tests
  • Proper resource cleanup with goroutines and WaitGroup

Overall Assessment

This is a well-implemented test that provides valuable coverage for node restart scenarios. The code is clean, follows established patterns, and tests an important failure/recovery scenario. The minor timing issues mentioned are typical for integration tests and don't significantly impact the test's value.

@codecov
Copy link

codecov bot commented Aug 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.1%. Comparing base (ac92895) to head (2b07643).
⚠️ Report is 18 commits behind head on theo/conn-drop-test.
✅ 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.

## Description

Try to restructure e2e tests go package to prevent race conditions in CI
@theochap theochap merged commit 15152e7 into theo/conn-drop-test Aug 22, 2025
24 of 31 checks passed
@theochap theochap deleted the theo/restart-tests branch August 22, 2025 18:16
@github-project-automation github-project-automation bot moved this from In Review to Done in Project Tracking Aug 22, 2025
theochap added a commit that referenced this pull request Aug 22, 2025
## Description

This PR adds restart tests for the kona node
theochap added a commit that referenced this pull request Aug 25, 2025
This PR adds restart tests for the kona node
theochap added a commit that referenced this pull request Aug 27, 2025
This PR adds restart tests for the kona node
theochap added a commit that referenced this pull request Aug 27, 2025
This PR adds restart tests for the kona node
theochap added a commit that referenced this pull request Aug 27, 2025
This PR adds restart tests for the kona node
theochap added a commit that referenced this pull request Aug 27, 2025
This PR adds restart tests for the kona node
theochap added a commit that referenced this pull request Aug 27, 2025
This PR adds restart tests for the kona node
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A-node Area: cl node (eq. Go op-node) handles single-chain consensus M-tests Meta: Testing related

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant