refactor(e2e): use alternative errgroup to catch panics #3623
Conversation
📝 WalkthroughWalkthroughThis pull request replaces the import source for the Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Goroutine
participant EG as Errgroup Instance
participant Worker as Worker Goroutine
Main->>EG: WithContext(ctx)
Main->>EG: EG.Go(fn)
Worker->>EG: Execute fn wrapped in catchPanics
Worker-->>EG: Pass error or converted panic error
Main->>EG: Wait()
EG-->>Main: Return first non-nil error or nil
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3623 +/- ##
===========================================
+ Coverage 64.55% 64.59% +0.04%
===========================================
Files 469 470 +1
Lines 32723 32772 +49
===========================================
+ Hits 21123 21170 +47
- Misses 10639 10641 +2
Partials 961 961
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/errgroup/errgroup_test.go (1)
11-78: Consider reducing duplicated test logic for panic scenarios.You have multiple tests (e.g.,
TestPanicWithString,TestPanicWithError,TestPanicWithOtherValue) that follow a nearly identical pattern of verifying that a panic cancels the context. Refactoring shared logic into helper functions or using table-driven tests with sub-tests (viat.Runblocks) can improve maintainability and readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/zetae2e/local/bitcoin.go(1 hunks)cmd/zetae2e/local/evm.go(1 hunks)cmd/zetae2e/local/local.go(1 hunks)pkg/errgroup/errgroup.go(1 hunks)pkg/errgroup/errgroup_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- cmd/zetae2e/local/local.go
- cmd/zetae2e/local/bitcoin.go
- cmd/zetae2e/local/evm.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/errgroup/errgroup_test.gopkg/errgroup/errgroup.go
🪛 GitHub Check: codecov/patch
pkg/errgroup/errgroup.go
[warning] 30-31: pkg/errgroup/errgroup.go#L30-L31
Added lines #L30 - L31 were not covered by tests
🔇 Additional comments (4)
pkg/errgroup/errgroup_test.go (1)
11-166: Add a test case forpanic(nil)to improve coverage.According to your implementation,
panic(nil)would yield anilreturn infromPanicValue. Currently, no test covers that exact scenario, leaving lines 30-31 infromPanicValue()untested. Consider adding a small test to ensure thatpanic(nil)doesn’t break the group logic and properly returnsnilor no error, thus increasing coverage.pkg/errgroup/errgroup.go (3)
30-31: Improve test coverage for handlingnilpanic values.Lines 30-31 handle the case where the panic value is
nil. This logic is currently uncovered by tests, as noted by static analysis. Adding a dedicated test that triggerspanic(nil)would ensure this path is tested and guarantee correct behavior.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 30-31: pkg/errgroup/errgroup.go#L30-L31
Added lines #L30 - L31 were not covered by tests
47-62: Well-structured panic handling logic.Wrapping user-defined functions in
catchPanics()effectively captures panics and converts them to errors without introducing undue complexity. This approach is clear and properly isolates panic handling.
64-125: Robust concurrency model with straightforward error propagation.The design of
Group—cancelling the context on the first encountered error or panic and collecting the result withWait()—is clean and follows established Go concurrency patterns. The named return mechanism incatchPanics()ensures that goroutine panics are captured with minimal overhead.
Use an errgroup implementation based on https://github.com/StevenACoffman/errgroup so that panics in the test routines are caught. This will reduce the amount of log spam when you have a test failure in one routine before another routine has even started running.
I just vendored the library to avoid external dependencies. The tests are rewritten to just use
requirerather thansuite.Closes #3613
Related to #3578
Summary by CodeRabbit