feat(node/test): improve connection drop tests#2787
Conversation
|
Claude finished @theochap's task —— View job PR Review: Connection Drop Test Improvements
|
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull Request Overview
This PR improves the connection drop tests by refactoring WebSocket utilities and enhancing test reliability. The changes focus on ensuring proper test timing by preventing unsafe head advancement before consolidation during connection drop scenarios.
- Moves WebSocket utilities from
tests/node/common/ws.goto a more centralized location with enhanced functionality - Improves connection drop test logic to verify that unsafe heads don't advance during disconnection periods
- Refactors test functions to use the new WebSocket utility location
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/node/utils/ws.go | New centralized WebSocket utilities with async support and improved error handling |
| tests/node/restart/restart_test.go | Simplifies sequencer head advancement check logic |
| tests/node/restart/conn_drop_test.go | Enhanced connection drop tests with unsafe head monitoring and improved sync verification |
| tests/node/common/ws.go | Removed old WebSocket utilities (moved to utils) |
| tests/node/common/sync_ws_test.go | Updated imports to use new WebSocket utility location |
| tests/node/common/engine_test.go | Updated imports to use new WebSocket utility location |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| logger.Info("Node safe and unsafe heads matched", "ref", ref.Number, "base", base.Number) | ||
| base = baseNode.ChainSyncStatus(chainID, lvl) | ||
| ref = refNode.ChainSyncStatus(chainID, lvl) | ||
| if ref.Number <= base.Number+delta || ref.Number >= base.Number-delta { |
There was a problem hiding this comment.
The logic for checking if numbers are within delta range is incorrect. The condition should use >= for the lower bound and <= for the upper bound. Currently, it will always be true since any number is either <= (base + delta) OR >= (base - delta).
| if ref.Number <= base.Number+delta || ref.Number >= base.Number-delta { | |
| if ref.Number >= base.Number-delta && ref.Number <= base.Number+delta { |
tests/node/restart/conn_drop_test.go
Outdated
| block, err := refNode.Escape().RollupAPI().OutputAtBlock(t.Ctx(), ref.Number) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| t.Require().Equal(block.BlockRef.Hash, base.Hash, "expected block hash to match") | ||
| t.Require().Equal(block.BlockRef.Number, base.Number, "expected block number to match") |
There was a problem hiding this comment.
The assertions compare block.BlockRef properties with base properties, but block is fetched using ref.Number while the assertions expect it to match base. This will fail when ref.Number != base.Number, which is the expected case when checking if nodes are within delta range.
b295562 to
5277c10
Compare
5277c10 to
c1199ad
Compare
c69d020 to
bbb9761
Compare
e6edb93 to
a935016
Compare
bbf8814 to
1c6bb01
Compare
a4f8924 to
99e4ed2
Compare
99e4ed2 to
2b64d34
Compare
## Description Simple PR that improves the connection drop tests by ensuring that the unsafe head doesn't advance before consolidation
## Description Simple PR that improves the connection drop tests by ensuring that the unsafe head doesn't advance before consolidation

Description
Simple PR that improves the connection drop tests by ensuring that the unsafe head doesn't advance before consolidation