feat(node/test): add l2 reorg tests

[theochap]

Description

Add a couple of l2 reorg tests with an appropriate network configuration.

[claude]

Claude finished @theochap's task —— View job

---

PR Review Complete ✅

I've completed a comprehensive review of the L2 reorg tests implementation.

Todo List

• Read repository guidelines and setup requirements
• Analyze the new L2 reorg test files
• Review CI workflow changes
• Examine utility function modifications
• Check test coverage and effectiveness
• Provide comprehensive feedback on code quality and best practices

Analysis Summary

This PR adds L2 reorganization tests to validate that both OP Stack and Kona nodes can properly handle chain reorganizations. The implementation includes:

New Files:

• tests/node/l2_reorg/l2_reorg_test.go - Main test implementation (160 lines)
• tests/node/l2_reorg/init_test.go - Test setup (13 lines)
• tests/node/utils/test_sequencer_preset.go - New test sequencer preset utility (69 lines)

Modified Files:

• CI workflow updated to include L2 reorg tests
• Test utilities refactored for better reusability
• Connection drop test improvements

Detailed Review

✅ Code Quality & Best Practices:

1. Test Structure: Well-organized with clear separation of concerns
2. Error Handling: Proper use of require assertions throughout
3. Naming: Follows Go conventions with descriptive variable names
4. Documentation: Adequate inline comments explaining complex logic
5. Integration: Good reuse of existing test infrastructure

🔍 Potential Issues:

1. Magic Numbers: NUM_BLOCKS_TO_REORG = 1 is hardcoded. Consider making this configurable or testing multiple values.

2. Sleep Dependencies: Lines 114, 118 in l2_reorg_test.go use time.Sleep(2 * time.Second) which could make tests flaky. Consider using polling with timeouts instead.

3. Error Context: Some error messages could be more descriptive, e.g., line 74 could include the hash being looked up.

4. Resource Cleanup: The test stops and starts sequencer/batcher but doesn't have explicit cleanup in defer statements.

⚡ Performance Considerations:

1. Test Duration: The test uses multiple retry loops with significant timeouts (40-200 attempts). This could lead to long test execution times but is appropriate for integration testing.

2. Concurrent Operations: Good use of parallel checking with dsl.CheckAll() for efficiency.

🔒 Security Concerns:

No significant security issues identified. The test uses proper test isolation and doesn't expose sensitive data.

📊 Test Coverage:

Strengths:

• Tests the complete reorg flow from start to finish
• Validates both safe and unsafe head progression
• Ensures transaction inclusion works post-reorg
• Tests reconnection behavior after reorg
• Validates balance updates after reorg
• Uses appropriate mixed node configuration (OP + Kona)

Areas for Enhancement:

• Could test reorgs of different depths (currently only 1 block)
• Missing edge cases like multiple consecutive reorgs
• No negative test cases (e.g., malformed reorg attempts)

🔧 Specific Recommendations:

1. Replace hardcoded sleeps in l2_reorg_test.go:114,118:

// Instead of: time.Sleep(2 * time.Second)
err = retry.Do0(ctx, 10, &retry.FixedStrategy{Dur: 200 * time.Millisecond}, 
    func() error {
        // Check if operation completed successfully
        return nil // or error if not ready
    })

2. Add parameterized test for different reorg depths:

func TestL2ReorgMultipleBlocks(t *testing.T) {
    testCases := []int{1, 2, 5}
    for _, numBlocks := range testCases {
        t.Run(fmt.Sprintf("reorg_%d_blocks", numBlocks), func(t *testing.T) {
            // test with numBlocks instead of hardcoded constant
        })
    }
}

3. Improve error context at l2_reorg_test.go:74:

require.NoError(t, err, "Expected to be able to call L2BlockRefByHash API for hash %s, but got error", parentOfHeadToReorgA.Hash)

4. Add cleanup defer at the beginning of the test:

defer func() {
    if !sequencerCL.IsSequencer() {
        sequencerCL.StartSequencer()  
    }
    if !out.L2Batcher.IsRunning() {
        out.L2Batcher.Start()
    }
}()

CI Integration

The CI workflow addition looks correct and follows existing patterns. The test is properly integrated into the sysgo test suite.

Utility Code Quality

The new test_sequencer_preset.go follows established patterns and provides good abstractions for test sequencer setup. The refactoring in mixed_preset.go removes unused imports and improves code organization.

Overall Assessment

This is a solid implementation that adds important test coverage for L2 reorg scenarios. The code follows good practices and integrates well with the existing test infrastructure. The main areas for improvement are around test reliability (removing sleeps) and expanding coverage to more reorg scenarios.

Rating: ✅ Approve with minor suggestions

The implementation effectively tests the critical reorg functionality and should help catch regression issues. The test design is comprehensive and the integration with existing infrastructure is clean.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.0%. Comparing base (6887336) to head (5352b7c).
⚠️ Report is 2 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.

[refcell]

Should we just remove this?

[refcell]

Approving to unblock. One nit otherwise

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.