refactor(testing): enhance AssertPanic with type-aware matching #143
Conversation
WalkthroughRefactors AssertPanic to support multiple, type-aware panic matching modes via new internal helpers, updates related comments, and converts its tests to a table-driven suite. README wording adjusted to reflect type-aware panic testing. Public APIs remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant A as AssertPanic
participant R as defer recover()
participant H as doAssertPanic
participant E as Helpers (Equal/Error/Contains)
C->>A: AssertPanic(fn, expected, name, args...)
activate A
A->>R: defer recover()
A->>A: invoke fn()
R-->>A: recovered value (panic or nil)
A->>H: doAssertPanic(recovered, expected, name, args...)
alt expected == nil
H-->>A: success if any panic occurred
else expected is Recovered
H->>E: direct compare (no unwrap)
E-->>H: match result
else expected is error
H->>E: errors.Is(recoveredError, expected)
E-->>H: match result
else expected is string
H->>E: substring match on panic message
E-->>H: match result
else
H->>E: DeepEqual(unwrappedRecovered, expected)
E-->>H: match result
end
H-->>A: bool (matched)
A-->>C: bool (assertion result)
deactivate A
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Here's the code health analysis summary for commits Analysis Summary
|
Refactored AssertPanic to provide type-specific panic validation, making tests more resilient to implementation changes whilst maintaining clear intent. Key improvements: - Added type-specific matching strategies: * nil: accepts any panic (most common use case) * error: uses errors.Is for chain matching * string: substring matching instead of exact equality * Recovered: direct comparison without unwrapping * other types: exact equality with Recovered unwrapping - Extracted validation logic to dedicated handlers - Simplified code by removing report flag - Enhanced logging with panic-specific context - Achieved 100% test coverage for testing.go Breaking changes: - String expectations now use substring matching - Error expectations now use errors.Is semantics The enhanced behaviour aligns with developer expectations, focusing on validating panic reasons rather than exact values. This makes tests less brittle whilst preserving their intent. Testing: - Added comprehensive table-driven tests using TestCase pattern - All branches and type paths fully covered Signed-off-by: Alejandro Mery <amery@apptly.co>
amery
force-pushed
the
pr-amery-errors
branch
from
70948b1 to
9dad1d1
Compare
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
![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 (4)
README.md (1)
538-538: Wrap the changed bullet to respect the 80‑char markdownlint limit.Split the description onto a continuation line to match surrounding style.
-* `AssertPanic(t, fn, expectedPanic, msg...)` / `AssertNoPanic(t, fn, msg...)` - panic testing with type-aware matching. +* `AssertPanic(t, fn, expectedPanic, msg...)` / `AssertNoPanic(t, fn, msg...)` - + panic testing with type-aware matching.testing.go (2)
407-437: Handle typed‑nil expectations (e.g., var e error = nil).If callers pass a typed‑nil interface as expectedPanic, it currently routes to the error branch and fails via errors.Is(err, nil). Normalise typed‑nil to untyped nil before the switch.
func doAssertPanic(t T, recovered, expected any, name string, args ...any) bool { t.Helper() // Check if panic occurred if recovered == nil { doError(t, name, args, "expected panic but got nil") return false } - switch target := expected.(type) { + // Normalise typed-nil interfaces/pointers to untyped nil. + if expected != nil { + v := reflect.ValueOf(expected) + switch v.Kind() { + case reflect.Interface, reflect.Pointer, reflect.Slice, reflect.Map, reflect.Func, reflect.Chan: + if v.IsNil() { + expected = nil + } + } + } + + switch target := expected.(type) { case nil: // Any panic is acceptable doLog(t, name, args, "panic: %v", recovered) return true
466-484: Empty substring edge‑case: decide and enforce semantics.strings.Contains(msg, "") is always true; consider rejecting empty substrings to avoid accidental always‑pass expectations.
func doAssertPanicContains(t T, recovered any, substr string, name string, args ...any) bool { var msg string t.Helper() + if substr == "" { + doError(t, name, args, "expected non-empty substring for panic match") + return false + } if s, ok := recovered.(string); ok { msg = s } else { msg = AsRecovered(recovered).Error() }testing_test.go (1)
335-385: Add a case for typed‑nil expected error (normalised to “any panic”).Guards the edge‑case suggested in code. Proposed addition below.
func assertPanicTestCases() []assertPanicTestCase { testErr := errors.New("test error") otherErr := errors.New("other error") panicErr := NewPanicError(0, "recovered panic") wrappedPanic := NewPanicError(0, 99) + var nilErr error return []assertPanicTestCase{ // Any panic tests newAssertPanicTestCase("any panic accepted", func() { panic("test panic") }, nil, "panic test", true, false, "test panic"), + newAssertPanicTestCase("typed-nil error treated as any panic", + func() { panic("typed nil ok") }, nilErr, "typed nil expected", + true, false, "typed nil ok"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)testing.go(1 hunks)testing_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.md
📄 CodeRabbit inference engine (AGENT.md)
**/*.md: Comply with markdownlint rules (80-character line limit, strict formatting, selective HTML allowlist)
Run LanguageTool with British English; fix missing articles, punctuation, and hyphenation issues
End all bullet list items with periods
Capitalize proper nouns correctly (e.g., JavaScript, TypeScript, Markdown)
Hyphenate compound modifiers when used before nouns (e.g., capture-specific)
Files:
README.md
**/*.{md,go}
📄 CodeRabbit inference engine (AGENT.md)
Pass CSpell checks using the project dictionary (British English) for docs and code comments
Files:
README.mdtesting.gotesting_test.go
**/*.go
📄 CodeRabbit inference engine (AGENT.md)
**/*.go: Adhere to revive limits: max function length 40 lines, max 3 results, max 5 arguments, cognitive complexity ≤7, cyclomatic complexity ≤10
Keep struct fields aligned to minimize padding (use fieldalignment -fix)
Files:
testing.gotesting_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENT.md)
**/*_test.go: Prefer table-driven tests
Use the public testing utilities from testing.go in tests
Test assertions should log successful checks for better debugging
Files:
testing_test.go
🧠 Learnings (2)
📚 Learning: 2025-08-20T03:45:42.506Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-20T03:45:42.506Z
Learning: Applies to **/*_test.go : Test assertions should log successful checks for better debugging
Applied to files:
README.mdtesting_test.go
📚 Learning: 2025-08-20T03:45:42.506Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-20T03:45:42.506Z
Learning: Applies to **/*_test.go : Prefer table-driven tests
Applied to files:
testing_test.go
🧬 Code graph analysis (2)
testing.go (1)
panic.go (2)
Recovered(8-11)AsRecovered(15-28)
testing_test.go (2)
testing.go (7)
TestCase(249-252)T(22-33)MockT(45-51)AssertPanic(398-405)AssertEqual(278-287)AssertContains(337-346)RunTestCases(260-267)panicerror.go (1)
NewPanicError(45-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
testing.go (5)
375-385: Docs for type‑aware matching read well. LGTM.Clear, British English (“whilst”), and accurately reflects behaviour.
390-398: Examples cover the main matching modes. LGTM.Good spread: any panic, substring, and error matching.
401-401: Defer+recover dispatch is correct and preserves the boolean result.No spurious re-panics; OK.
439-451: Exact‑equality path via DeepEqual. LGTM.Covers non‑error, non‑string payloads cleanly.
452-465: errors.Is on recovered chain. LGTM.The AsRecovered() bridge keeps this resilient to wrapping.
testing_test.go (1)
289-390: Table‑driven coverage for all matching modes. LGTM.Solid cases for any/no panic, substring, error via errors.Is, exact, and Recovered unwrapping, with log checks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
testing.go (2)
439-450: Include types on mismatch to aid debuggingWhen DeepEqual fails, printing types helps pinpoint subtle mismatches (e.g., int vs int64).
Apply:
- doError(t, name, args, "expected panic %v, got %v", expected, recovered) + doError(t, name, args, "expected panic (%T)%v, got (%T)%v", expected, expected, recovered, recovered)
466-484: Substring matcher works; consider logging the recovered type for extra contextOptional: enrich the success/failure messages with %T(msgSource) to show whether the message came from a string, error, or wrapper, which eases triage when tests fail.
Example:
- doLog(t, name, args, "panic contains %q: %q", substr, msg) + doLog(t, name, args, "panic contains %q: %q", substr, msg) + // optionally: include source type using a separate log if desired + // doLog(t, name, args, "panic message type: %T", recovered)testing_test.go (2)
303-320: Driver exercises both result and logging pathsNice reuse of MockT and AssertPanic. Minor optional: when expectErrors is true, consider also asserting on LastError() content to lock messages, but current coverage is fine.
335-385: Strong coverage; add two edge cases to harden behaviour
- Add a negative “Recovered mismatch” to prove pointer identity is required when expected is Recovered.
- Add an error-chain match where the panic wraps the target error (e.g., errors.Join) to exercise errors.Is path through a wrapper in AssertPanic.
Proposed additions:
func assertPanicTestCases() []assertPanicTestCase { testErr := errors.New("test error") otherErr := errors.New("other error") panicErr := NewPanicError(0, "recovered panic") wrappedPanic := NewPanicError(0, 99) + chainedErr := errors.Join(testErr, errors.New("context")) + otherPanic := NewPanicError(0, "recovered panic") // distinct instance return []assertPanicTestCase{ @@ // Recovered type tests newAssertPanicTestCase("Recovered type match", func() { panic(panicErr) }, panicErr, "recovered test", true, false, "panic:"), newAssertPanicTestCase("Recovered unwrapping", func() { panic(wrappedPanic) }, 99, "unwrapped test", true, false, "panic: 99"), + // Recovered mismatch (pointer identity should fail) + newAssertPanicTestCase("Recovered type mismatch", + func() { panic(panicErr) }, otherPanic, "recovered mismatch", + false, true, ""), + + // Error chain via wrapper should match using errors.Is + newAssertPanicTestCase("error chain match via wrapper", + func() { panic(chainedErr) }, testErr, "wrapped error test", + true, false, "panic error"), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)testing.go(1 hunks)testing_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENT.md)
**/*.go: Adhere to revive limits: max function length 40 lines, max 3 results, max 5 arguments, cognitive complexity ≤7, cyclomatic complexity ≤10
Keep struct fields aligned to minimize padding (use fieldalignment -fix)
Files:
testing.gotesting_test.go
**/*.{md,go}
📄 CodeRabbit inference engine (AGENT.md)
Pass CSpell checks using the project dictionary (British English) for docs and code comments
Files:
testing.gotesting_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENT.md)
**/*_test.go: Prefer table-driven tests
Use the public testing utilities from testing.go in tests
Test assertions should log successful checks for better debugging
Files:
testing_test.go
🧠 Learnings (2)
📚 Learning: 2025-08-20T03:45:42.506Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-20T03:45:42.506Z
Learning: Applies to **/*_test.go : Prefer table-driven tests
Applied to files:
testing_test.go
📚 Learning: 2025-08-20T03:45:42.506Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-20T03:45:42.506Z
Learning: Applies to **/*_test.go : Test assertions should log successful checks for better debugging
Applied to files:
testing_test.go
🧬 Code graph analysis (2)
testing.go (1)
panic.go (2)
Recovered(8-11)AsRecovered(15-28)
testing_test.go (2)
testing.go (7)
TestCase(249-252)T(22-33)MockT(45-51)AssertPanic(398-405)AssertEqual(278-287)AssertContains(337-346)RunTestCases(260-267)panicerror.go (1)
NewPanicError(45-53)
🔇 Additional comments (7)
testing.go (3)
398-405: Defer-recover placement is correctCapturing recover() in the single defer and delegating to doAssertPanic keeps the control flow simple and avoids double-recovers. LGTM.
452-464: errors.Is matching path LGTMWrapping via AsRecovered and then applying errors.Is aligns with the new semantics. The logging is also informative.
375-398: Audit AssertPanic substring matching in tests
The string-based matching rule now uses substring checks instead of exact equality. Verify tests relying on an exact panic message – notably spinlock_test.go:170 – still behave as expected.testing_test.go (4)
289-299: Good table-driven harness for AssertPanicThe struct covers inputs, expectations, and log sniffing cleanly. Fits our preference for table-driven tests.
300-302: Compile-time interface check LGTMEnsures the test case type implements TestCase at build time.
321-333: Constructor keeps call sites tidyGood use of revive suppression for arg count.
387-390: Orchestration via RunTestCases is idiomaticKeeps the suite concise and consistent with our TestCase pattern.
| func doAssertPanic(t T, recovered, expected any, name string, args ...any) bool { | ||
| t.Helper() | ||
|
|
||
| // Check if panic occurred | ||
| if recovered == nil { | ||
| doError(t, name, args, "expected panic but got nil") | ||
| return false | ||
| } | ||
|
|
||
| switch target := expected.(type) { | ||
| case nil: | ||
| // Any panic is acceptable | ||
| doLog(t, name, args, "panic: %v", recovered) | ||
| return true | ||
| case Recovered: | ||
| // Direct comparison without unwrapping | ||
| return doAssertPanicEqual(t, recovered, expected, name, args...) | ||
| case error: | ||
| // Error chain matching | ||
| return doAssertPanicError(t, recovered, target, name, args...) | ||
| case string: | ||
| // Substring matching | ||
| return doAssertPanicContains(t, recovered, target, name, args...) | ||
| default: | ||
| // Exact equality after potential unwrapping | ||
| if r, ok := recovered.(Recovered); ok { | ||
| recovered = r.Recovered() | ||
| } | ||
| return doAssertPanicEqual(t, recovered, expected, name, args...) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Dispatch logic is sound; confirm Unwrap support for error chains
The precedence (Recovered before error) matches the documented intent. For error matching to work reliably through wrappers, PanicError must implement Unwrap() so that errors.Is can traverse to the payload. Please verify that.
🏁 Script executed:
#!/bin/bash
# Verify PanicError (or Recovered impl) provides Unwrap to support errors.Is
rg -nP --type=go '(?s)type\s+PanicError\b.*|func\s*\(\s*\*?PanicError\s*\)\s*Unwrap\s*\(' -C2Length of output: 230
Add Unwrap method to PanicError for errors.Is compatibility
panicerror.go: PanicError lacks an Unwrap() error method, so calls to errors.Is/As can’t traverse through the wrapped payload. Implement e.g.:
func (p PanicError) Unwrap() error {
if err, ok := p.payload.(error); ok {
return err
}
return nil
}🤖 Prompt for AI Agents
In testing.go around lines 407 to 437: the test helper handles PanicError
payloads but PanicError itself lacks an Unwrap() error method, preventing
errors.Is/As from traversing to the underlying error; add an Unwrap() error
method to the PanicError type (in panicerror.go) that returns the payload when
it implements error and nil otherwise, ensuring standard error chain
compatibility and allowing the existing panic assertion helpers to work with
errors.Is/As.
There was a problem hiding this comment.
that's what Recovered() is supposed to do
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the Recovered type/interface definition and its implementation
rg -nP --type=go 'type\s+Recovered\b' -A10
# Look for Recovered method implementations
rg -nP --type=go 'func.*Recovered\s*\(' -A5
# Check how Recovered is used with errors
rg -nP --type=go 'Recovered.*error|error.*Recovered' -C3
# Find PanicError type definition and methods
rg -nP --type=go 'type\s+PanicError\b' -A10
Length of output: 5347
🏁 Script executed:
#!/bin/bash
# Check if there's already an Unwrap method implementation
rg -nP --type=go 'func.*Unwrap.*error' -A3
# Look for the complete PanicError methods
rg -nP --type=go 'panicerror\.go' -A50 | head -60
Length of output: 1222
@amery You're absolutely right! I see that PanicError already has an Unwrap() method implemented:
func (p *PanicError) Unwrap() error {
if err, ok := p.payload.(error); ok {
return err
}
return nil
}The Recovered() method provides access to the original payload, and the existing Unwrap() method already handles the errors.Is compatibility by returning the payload when it's an error. The design is already complete and supports proper error chain traversal.
My suggestion was unnecessary - the panic assertion logic with AsRecovered(recovered) and errors.Is(err, target) will work correctly with the existing implementation.
(\ /)
( ._.)
o_(")(")
✏️ Learnings added
Learnt from: amery
PR: darvaza-proxy/core#143
File: testing.go:407-437
Timestamp: 2025-09-07T19:20:04.501Z
Learning: PanicError already implements both Recovered() method (which returns the original payload) and Unwrap() method (which returns the payload as error if it implements error interface), providing complete support for error chain traversal and errors.Is compatibility.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Released as v0.18.3 🎉 go get darvaza.org/core@v0.18.3 |
User description
Refactored AssertPanic to provide type-specific panic validation, making tests more resilient to implementation changes whilst maintaining clear intent.
Key improvements:
Breaking changes:
The enhanced behaviour aligns with developer expectations, focusing on validating panic reasons rather than exact values. This makes tests less brittle whilst preserving their intent.
Testing:
PR Type
Enhancement
Description
Enhanced
AssertPanicwith type-aware panic validation strategiesAdded substring matching for strings and error chain matching for errors
Extracted validation logic into dedicated handler functions
Comprehensive test coverage with table-driven test cases
Diagram Walkthrough
flowchart LR A["AssertPanic"] --> B["doAssertPanic"] B --> C{Expected Type?} C -->|nil| D["Accept any panic"] C -->|string| E["doAssertPanicContains"] C -->|error| F["doAssertPanicError"] C -->|Recovered| G["doAssertPanicEqual"] C -->|other| H["Unwrap & Compare"] E --> I["Substring matching"] F --> J["errors.Is matching"] G --> K["Direct comparison"] H --> KFile Walkthrough
testing.go
Enhanced AssertPanic with type-aware validationtesting.go
AssertPanicto support type-specific validation strategiestesting_test.go
Comprehensive table-driven tests for AssertPanictesting_test.go
assertPanicTestCasestruct implementingTestCaseinterfaceREADME.md
Updated AssertPanic documentationREADME.md
AssertPanicdocumentation to mention type-aware matchingSummary by CodeRabbit
New Features
Documentation
Tests