Testing Infrastructure Enhancement and Documentation Overhaul #145
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Here's the code health analysis summary for commits Analysis Summary
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
amery
added
documentation
PR Code Suggestions ✨Latest suggestions up to b79e6ec
Previous suggestionsSuggestions up to commit 47f2cf6
|
|||||||||||||||||||||||||||||||||||||||||
WalkthroughThe pull request refactors the testing assertion API surface and updates documentation references. It renames documentation files (AGENT.md to AGENTS.md), modifies assertion function signatures to use a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (4)**/*.go📄 CodeRabbit inference engine (AGENT.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (AGENT.md)
Files:
**/*.{md,go}📄 CodeRabbit inference engine (AGENT.md)
Files:
**/*.md📄 CodeRabbit inference engine (AGENT.md)
Files:
🧠 Learnings (4)📚 Learning: 2025-08-20T03:45:42.506ZApplied to files:
📚 Learning: 2025-07-18T16:39:19.390ZApplied to files:
📚 Learning: 2025-08-20T03:45:42.506ZApplied to files:
📚 Learning: 2025-08-05T22:27:53.946ZApplied to files:
🧬 Code graph analysis (1)testing_test.go (1)
🪛 LanguageToolREADME.md[grammar] ~536-~536: This sentence seems to be incomplete. Insert a noun before ‘Is’ to make the sentence complete. (MISSING_SUBJECT) [locale-violation] ~539-~539: Lower-case ‘ok’ is American English. For British English use “OK”. (OK) 🔇 Additional comments (12)
Comment |
amery
force-pushed
the
pr-amery-testing
branch
from
47f2cf6 to
b79e6ec
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
testing_test.go (1)
46-69: Avoid circular testing of base assertions in testing_test.go.Per TESTING_core.md, when testing base assertions (e.g., AssertEqual), use the standard testing package, not other assert helpers like AssertTrue/False which depend on AssertEqual.
Apply this pattern locally; example for this test:
- result := AssertEqual(mock, 42, 42, "equal test") - AssertTrue(t, result, "AssertEqual result when equal") - AssertFalse(t, mock.HasErrors(), "no errors on success") - AssertTrue(t, mock.HasLogs(), "has logs on success") + result := AssertEqual(mock, 42, 42, "equal test") + if !result { + t.Error("AssertEqual should return true") + } + if mock.HasErrors() { + t.Error("no errors on success") + } + if !mock.HasLogs() { + t.Error("success should be logged") + }Please apply similarly to other base‑assert tests in this file.
generics_test.go (1)
1-477: Replace remaining t.Errorf/t.Fatalf uses with Assert helpers*Standardise on the repo's public testing utilities (AssertEqual/AssertSame, etc.) and remove redundant early returns after assertions.
Occurrences to fix: generics_test.go — lines 32, 99, 103, 219, 280, 328, 332, 376, 423, 428, 433.
stack_test.go (1)
90-95: Fix: invalid range over ints (compilation error).You cannot range over an int. Switch to conventional for-loops.
Apply:
- for i := range MaxTestDepth { - for j := range MaxTestSpace { + for i := 0; i < MaxTestDepth; i++ { + for j := 0; j < MaxTestSpace; j++ { testDeepStackTrace(t, i, j) } }lists_test.go (1)
383-392: Fix the “nil function” test to actually pass nil.Currently a non‑nil no‑op is passed, so this does not exercise the nil‑fn early‑return path.
Apply:
func runTestListForEachNilFunction(t *testing.T, name string, iterFn func(*list.List, func(int) bool)) { t.Helper() l := list.New() l.PushBack(1) l.PushBack(2) - var result []int - iterFn(l, func(int) bool { return false }) - AssertEqual(t, 0, len(result), name+" nil function") + var result []int + iterFn(l, nil) + AssertEqual(t, 0, len(result), name+" nil function") }
🧹 Nitpick comments (46)
testing.go (2)
787-800: Capture panics from worker goroutines to avoid crashing the process.A panic inside a worker will currently take down the test process. Convert panics to errors on
errCh.Apply:
func runWorkers(numWorkers int, worker func(int) error, errCh chan error) { var wg sync.WaitGroup - for i := range numWorkers { + for i := 0; i < numWorkers; i++ { wg.Add(1) go func(id int) { - defer wg.Done() - if err := worker(id); err != nil { - errCh <- err - } + defer wg.Done() + defer func() { + if r := recover(); r != nil { + if e, ok := r.(error); ok { + errCh <- e + } else { + errCh <- fmt.Errorf("panic: %v", r) + } + } + }() + if err := worker(id); err != nil { + errCh <- err + } }(i) } wg.Wait() close(errCh) }
714-724: British English in comments.Project guidelines require British English in code comments. Please standardise wording.
-// This standardizes concurrent testing patterns. +// This standardises concurrent testing patterns. -// This standardizes benchmark patterns with proper timer management. +// This standardises benchmark patterns with proper timer management.Also applies to: 732-742
TESTING_core.md (1)
376-393: Incorrect example: RunBenchmark doesn’t return an error.The sample assigns
err := RunBenchmark(...)but the function returns no value.-func TestRunBenchmark(t *testing.T) { - called := false - err := RunBenchmark(&testing.B{}, +func TestRunBenchmark(t *testing.T) { + called := false + RunBenchmark(&testing.B{}, func() interface{} { return "test data" }, func(data interface{}) { called = true AssertEqual(t, "test data", data.(string), "benchmark data") }, ) - - AssertNoError(t, err, "benchmark should run") AssertTrue(t, called, "benchmark function should be called") }errgroup_test.go (2)
431-457: Reduce time‑based flakiness in Done test.The fixed sleeps (5 ms / 50 ms) are tight for CI. Prefer synchronisation over timers or widen margins.
- case <-time.After(5 * time.Millisecond): + case <-time.After(50 * time.Millisecond):Optionally gate readiness with a barrier channel or a small WaitGroup instead of sleeping.
373-401: Similar flakiness risks with small timeouts.1 ms budgets can intermittently fail under load. Consider 10–20 ms minima or synchronisation primitives.
Also applies to: 402-429
testing_test.go (1)
512-530: Creatingtesting.Bdirectly is brittle and ties to Go 1.22 loop syntax.Constructing
&testing.B{N: 10}outside the benchmark runner is unsupported andRunBenchmarkcurrently usesfor range b.N(Go 1.22 only). Either:
- abstract a tiny
Binterface with justResetTimer()andNaccess and provide a mock, or- drop this unit test and rely on real benchmarks.
Confirm toolchain (see earlier script) and whether CI enforces 1.22+.
TESTING.md (4)
393-393: Fix typo: "AssertNotContain" should be "AssertNotContains".The function name should be
AssertNotContainsto maintain consistency with the existingAssertContainsfunction and follow proper grammatical structure.-core.AssertNotContain(t, text, substring, "text exclusion") +core.AssertNotContains(t, text, substring, "text exclusion")
1153-1158: Fix command formatting for markdownlint compliance.The command should be on a single line to avoid markdownlint issues, or use proper line continuation syntax.
-<!-- markdownlint-disable MD013 --> ```bash -go run golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment@latest \\ - -fix sample_file.go +go run golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment@latest -fix sample_file.go-
--- `1454-1455`: **Consolidate return type in factory function example.** The example shows `[]TestCase` but earlier documentation states factory functions should return the concrete type for better type safety. ```diff -func makeMyFunctionTestCases() []TestCase { - return []TestCase{ +func makeMyFunctionTestCases() []myTestCase { + return []myTestCase{
1470-1472: Fix inconsistency in return type documentation.The example shows returning
[]TestCasebut this contradicts the earlier guidance about returning concrete types.-func makeValidationTestCases(fieldName string) []TestCase { - return []TestCase{ +func makeValidationTestCases(fieldName string) []validationTestCase { + return []validationTestCase{waitgroup_test.go (2)
100-100: Consider simplifying the factory function signature.The function signature could be more readable by placing the return type on the same line when it fits.
-func newWaitGroupGoCatchTestCase(name string, fn func() error, catch func(error) error, - expectError bool, errorMsg string) TestCase { +func newWaitGroupGoCatchTestCase(name string, fn func() error, catch func(error) error, expectError bool, errorMsg string) TestCase {
181-181: Inconsistent line breaking in function signature.For consistency with other factory functions in the file, consider keeping parameters on the same line when they fit.
-func newWaitGroupOnErrorTestCase(name string, workers []func() error, - onErrorHandler func(error) error, expectError bool, errorMsg string) TestCase { +func newWaitGroupOnErrorTestCase(name string, workers []func() error, onErrorHandler func(error) error, expectError bool, errorMsg string) TestCase {addrs_test.go (1)
53-53: Remove unnecessary revive directive.The
flag-parameterdirective appears unnecessary as the boolean parameter is essential for the function's purpose.-//revive:disable-next-line:flag-parameter func newParseAddrTestCaseStr(name, input, wantAddr string, wantErr bool) TestCase {addrport_test.go (2)
95-95: Consider adding the//revive:disable-next-line:flag-parameterdirective.For consistency with other test files in the codebase that have boolean flag parameters in factory functions.
+//revive:disable-next-line:flag-parameter func newAddrPortTestCase(name string, input any, want netip.AddrPort, wantOK bool) TestCase {
158-159: Function signature formatting inconsistency.Consider keeping the parameters on the same line for consistency with other factory functions.
-func newTypeSpecificAddrPortTestCase(name string, input any, want netip.AddrPort, - wantOK bool) TestCase { +func newTypeSpecificAddrPortTestCase(name string, input any, want netip.AddrPort, wantOK bool) TestCase {slices_test.go (3)
192-192: Inconsistent function signature formatting.For consistency with other factory functions, consider keeping parameters on the same line when they fit.
-func newSliceMinusFnTestCase(name string, a, b, expected []string, - equal func(string, string) bool) TestCase { +func newSliceMinusFnTestCase(name string, a, b, expected []string, equal func(string, string) bool) TestCase {
329-330: Inconsistent function signature formatting in generic factory.For consistency, consider keeping the signature on fewer lines.
-func newSliceMapTestCase[T1, T2 any](name string, input []T1, fn func([]T2, T1) []T2, - expected []T2) sliceMapTestCase[T1, T2] { +func newSliceMapTestCase[T1, T2 any](name string, input []T1, fn func([]T2, T1) []T2, expected []T2) sliceMapTestCase[T1, T2] {
444-445: Inconsistent function signature formatting in generic factory.Consider formatting consistently with other factory functions.
-func newSliceReversedFnTestCase[T any](name string, input []T, fn func([]T, T) (T, bool), - expected []T) sliceReversedFnTestCase[T] { +func newSliceReversedFnTestCase[T any](name string, input []T, fn func([]T, T) (T, bool), expected []T) sliceReversedFnTestCase[T] {context_test.go (4)
266-275: “wrong type” case currently also tests key identity; bind the same key to isolate type‑mismatch.Using two distinct NewContextKeyint instances makes the failure due to key mismatch rather than value type. Reuse the same key in both context.WithValue and Get.
Apply this diff:
func makeContextKeyGetTestCases() []TestCase { var nilCtx context.Context + k := NewContextKey[int]("test") return S( newContextKeyGetTestCase(context.Background(), "nil receiver", nil, 0, false), - newContextKeyGetTestCase(nilCtx, "nil context", NewContextKey[int]("test"), 0, false), - newContextKeyGetTestCase( - context.WithValue(context.Background(), NewContextKey[int]("test"), "string_value"), - "wrong type", NewContextKey[int]("test"), 0, false), + newContextKeyGetTestCase(nilCtx, "nil context", k, 0, false), + newContextKeyGetTestCase( + context.WithValue(context.Background(), k, "string_value"), + "wrong type", k, 0, false), ) }
103-114: Reduce timing flakiness in timeout test.Tight sleeps can be flaky in slow CI. Widen the margins.
- ctx, cancel := WithTimeout(context.Background(), 10*time.Millisecond) + ctx, cancel := WithTimeout(context.Background(), 25*time.Millisecond) ... - case <-time.After(100 * time.Millisecond): + case <-time.After(250 * time.Millisecond):
116-133: Ensure cleanup on early failures.Defer cancel to avoid leaked timers if the test fails before explicit cancel.
func runTestWithTimeoutCancellation(t *testing.T) { ctx, cancel := WithTimeout(context.Background(), time.Hour) + defer cancel()
215-218: Assert cause robustly.Use errors.Is to assert the propagated cause; it’s clearer and resilient to wrapping.
- if cause := context.Cause(ctx); cause != testErr { - AssertEqual(t, testErr, cause, "cause") - } + AssertTrue(t, errors.Is(context.Cause(ctx), testErr), "cause")compounderror_test.go (2)
304-323: Also assert order and identity of unwrapped errors.Strengthen the “unwrappable” path by checking that items are appended in order and by identity.
expectedLen := 3 // original 1 + unwrapped 2 if !AssertEqual(t, expectedLen, len(ce.Errs), "error count") { return } + if !AssertSame(t, mock.errs[0], ce.Errs[1], "first unwrapped error") { + return + } + if !AssertSame(t, mock.errs[1], ce.Errs[2], "second unwrapped error") { + return + }
351-360: Formatted note assertion: simplify and avoid dual-path contains.You can assert the exact formatted string when args are present, and avoid OR checks that can hide regressions.
- if tc.expectNote { - if tc.note != "" { - expectedNote := tc.note - if len(tc.args) > 0 { - expectedNote = "wrapped note: 42" // for the formatted case - } - foundExpected := strings.Contains(errorStr, expectedNote) - foundOriginal := strings.Contains(errorStr, tc.note) - if !AssertTrue(t, foundExpected || foundOriginal, "note in error string") { - return - } - } - } + if tc.expectNote && tc.note != "" { + if len(tc.args) > 0 { + AssertContains(t, errorStr, "wrapped note: 42", "formatted note") + } else { + AssertContains(t, errorStr, tc.note, "note") + } + }Also applies to: 368-402
panicerror_test.go (8)
120-135: Prefer Assert helpers over Fatalf for consistency and pass‑logging.*if tc.expectUnwrap { - if unwrapped == nil { - t.Fatalf("expected unwrapped error, got nil") - } - if unwrapped.Error() != tc.expectedError { - t.Fatalf("expected unwrapped error '%s', got '%s'", tc.expectedError, unwrapped.Error()) - } - } else if unwrapped != nil { - t.Fatalf("expected nil unwrapped, got %v", unwrapped) + if !AssertNotNil(t, unwrapped, "unwrapped error") { + return + } + AssertEqual(t, tc.expectedError, unwrapped.Error(), "unwrapped error message") + } else { + AssertNil(t, unwrapped, "unwrapped error") }
167-182: Use Assert for message and payload type checks.*errorStr := pe.Error() expectedError := fmt.Sprintf("panic: %s", tc.expected) - if errorStr != expectedError { - t.Fatalf("expected error '%s', got '%s'", expectedError, errorStr) - } + AssertEqual(t, expectedError, errorStr, "error message") ... - if _, ok := pe.Recovered().(error); !ok { - t.Fatalf("expected error payload, got %T", pe.Recovered()) - } + if _, ok := pe.Recovered().(error); !AssertTrue(t, ok, "error payload") { + return + }
198-214: Wrap tests: assert with helpers for readability.unwrapped := pe.Unwrap() - if unwrapped == nil { - t.Fatalf("expected unwrapped error, got nil") - } + AssertNotNil(t, unwrapped, "unwrapped error") ... - if !strings.Contains(errorStr, note) { - t.Fatalf("expected error to contain note '%s', got '%s'", note, errorStr) - } - if !strings.Contains(errorStr, originalErr.Error()) { - t.Fatalf("expected error to contain original error '%s', got '%s'", originalErr.Error(), errorStr) - } + AssertContains(t, errorStr, note, "note in error") + AssertContains(t, errorStr, originalErr.Error(), "original error in message")
221-237: Wrapf tests: use AssertContains.- if unwrapped == nil { - t.Fatalf("expected unwrapped error, got nil") - } + AssertNotNil(t, unwrapped, "unwrapped error") ... - if !strings.Contains(errorStr, "wrapped note: 42") { - t.Fatalf("expected error to contain formatted note, got '%s'", errorStr) - } - if !strings.Contains(errorStr, originalErr.Error()) { - t.Fatalf("expected error to contain original error '%s', got '%s'", originalErr.Error(), errorStr) - } + AssertContains(t, errorStr, "wrapped note: 42", "formatted note") + AssertContains(t, errorStr, originalErr.Error(), "original error in message")
397-427: NewUnreachableError: convert to Assert and early returns.*result := NewUnreachableError(0, tc.err, tc.note) - - if result == nil { - t.Fatalf("expected non-nil error, got nil") - } + if !AssertNotNil(t, result, "non-nil error") { return } // Test that it's a PanicError pe, ok := result.(*PanicError) - if !ok { - t.Fatalf("expected PanicError, got %T", result) - } + if !AssertTrue(t, ok, "PanicError type") { return } // Test that ErrUnreachable is somewhere in the chain - if !errors.Is(result, ErrUnreachable) { - t.Fatalf("expected ErrUnreachable in error chain") - } + AssertTrue(t, errors.Is(result, ErrUnreachable), "ErrUnreachable in chain") // Test error message errorStr := result.Error() - if errorStr == "" { - t.Fatalf("expected non-empty error message") - } + AssertTrue(t, errorStr != "", "non-empty error message") // Test stack trace stack := pe.CallStack() - if len(stack) == 0 { - t.Fatalf("expected non-empty stack trace") - } + AssertTrue(t, len(stack) > 0, "non-empty stack trace")
444-476: NewUnreachableErrorf: ditto.- if result == nil { - t.Fatalf("expected non-nil error, got nil") - } + if !AssertNotNil(t, result, "non-nil error") { return } ... - if !errors.Is(result, ErrUnreachable) { - t.Fatalf("expected ErrUnreachable in error chain") - } + AssertTrue(t, errors.Is(result, ErrUnreachable), "ErrUnreachable in chain") ... - if !errors.Is(result, err) { - t.Fatalf("expected original error in error chain") - } + AssertTrue(t, errors.Is(result, err), "original error in chain") ... - if !strings.Contains(errorStr, "formatted note: 42") { - t.Fatalf("expected formatted message in error, got '%s'", errorStr) - } + AssertContains(t, errorStr, "formatted note: 42", "formatted message") ... - if len(stack) == 0 { - t.Fatalf("expected non-empty stack trace") - } + AssertTrue(t, len(stack) > 0, "non-empty stack trace")
376-381: Remove unused field from newUnreachableErrorTestCase.expectType is never read; drop it to keep the type lean.
type newUnreachableErrorTestCase struct { name string err error note string - expectType string } ... func newNewUnreachableErrorTestCase(name string, err error, note string) TestCase { return newUnreachableErrorTestCase{ name: name, err: err, note: note, - expectType: "PanicError", } }Also applies to: 430-436
1-522: Standardise tests — replace direct t.Fatalf calls with Assert helpers*Found 26 direct t.Fatalf occurrences (no t.Errorf) in panicerror_test.go; convert them to the appropriate Assert* helpers (AssertEqual / AssertNotNil / AssertTrue / AssertNil) for consistent, positive test logging.
Locations: panicerror_test.go — lines 78, 127, 130, 133, 175, 180, 203, 209, 212, 226, 232, 235, 287, 307, 355, 402, 408, 413, 419, 425, 447, 453, 458, 463, 469, 475.
generics_test.go (7)
27-34: Use AssertEqual for consistency and pass‑logs.got := Coalesce(tc.inputs...) - if got != tc.expected { - t.Errorf("Coalesce(%v) = %v, want %v", tc.inputs, got, tc.expected) - } + if !AssertEqual(t, tc.expected, got, "Coalesce(%v)", tc.inputs) { + return + }
95-104: Pointer case: assert nilness and value via Assert.*if tc.expected == nil && got == nil { return } if tc.expected == nil || got == nil { - t.Errorf("Coalesce(%v) = %v, want %v", tc.inputs, got, tc.expected) + AssertEqual(t, tc.expected == nil, got == nil, "Coalesce(%v) nilness", tc.inputs) return } - if *got != *tc.expected { - t.Errorf("Coalesce(%v) = %v, want %v", tc.inputs, *got, *tc.expected) - } + AssertEqual(t, *tc.expected, *got, "Coalesce(%v)", tc.inputs)
214-221: IIf int: switch to AssertEqual.got := IIf(tc.cond, tc.yes, tc.no) - if got != tc.expected { - t.Errorf("IIf(%v, %v, %v) = %v, want %v", tc.cond, tc.yes, tc.no, got, tc.expected) - } + if !AssertEqual(t, tc.expected, got, "IIf(%v, %v, %v)", tc.cond, tc.yes, tc.no) { + return + }
275-282: IIf string: use AssertEqual.got := IIf(tc.cond, tc.yes, tc.no) - if got != tc.expected { - t.Errorf("IIf(%v, %v, %v) = %v, want %v", tc.cond, tc.yes, tc.no, got, tc.expected) - } + AssertEqual(t, tc.expected, got, "IIf(%v, %q, %q)", tc.cond, tc.yes, tc.no)
323-333: IIf pointer: assert nilness and value with Assert.*if tc.expected == nil && got == nil { return } if tc.expected == nil || got == nil { - t.Errorf("IIf(%v, %v, %v) = %v, want %v", tc.cond, tc.yes, tc.no, got, tc.expected) + AssertEqual(t, tc.expected == nil, got == nil, "IIf(%v, %v, %v) nilness", tc.cond, tc.yes, tc.no) return } - if *got != *tc.expected { - t.Errorf("IIf(%v, %v, %v) = %v, want %v", tc.cond, tc.yes, tc.no, *got, *tc.expected) - } + AssertEqual(t, *tc.expected, *got, "IIf(%v, %v, %v)", tc.cond, tc.yes, tc.no)
371-377: IIf struct: use AssertEqual.got := IIf(tc.cond, tc.yes, tc.no) - if got != tc.expected { - t.Errorf("IIf(%v, %+v, %+v) = %+v, want %+v", tc.cond, tc.yes, tc.no, got, tc.expected) - } + AssertEqual(t, tc.expected, got, "IIf(%v, %+v, %+v)", tc.cond, tc.yes, tc.no)
412-435: Evaluation test: replace direct errors with Assert.*- if !calledYes || !calledNo { - t.Errorf("Functions not evaluated before IIf call: yes=%v, no=%v", calledYes, calledNo) - } + AssertTrue(t, calledYes && calledNo, "both branches evaluated") ... - if result != 42 { - t.Errorf("IIf(true, 42, 100) = %v, want 42", result) - } + AssertEqual(t, 42, result, "IIf(true, 42, 100)") ... - if result != 100 { - t.Errorf("IIf(false, 42, 100) = %v, want 100", result) - } + AssertEqual(t, 100, result, "IIf(false, 42, 100)")spinlock_test.go (2)
95-126: Avoid sleeps in contended case; synchronise with channels.Time‑based sleeps can flake. Signal the second worker only after the first acquires the lock.
if tc.name == "contended spinlock" { - // Test concurrent access - acquired := make(chan bool, 2) - - err := RunConcurrentTest(t, 2, func(id int) error { - if id == 0 { - // First goroutine acquires lock - sl.Lock() - acquired <- true - time.Sleep(10 * time.Millisecond) - sl.Unlock() - } else { - // Second goroutine waits for lock - time.Sleep(5 * time.Millisecond) - sl.Lock() - acquired <- true - sl.Unlock() - } - return nil - }) + acquired := make(chan bool, 2) + firstLocked := make(chan struct{}) + err := RunConcurrentTest(t, 2, func(id int) error { + if id == 0 { + sl.Lock() + close(firstLocked) + acquired <- true + sl.Unlock() + } else { + <-firstLocked + sl.Lock() + acquired <- true + sl.Unlock() + } + return nil + })
263-281: Benchmark remainder handling.iterations := b.N / numWorkers drops the remainder. Consider distributing the remainder to one worker to account for all iterations. Non‑blocking.
- iterations := b.N / numWorkers + iterations := b.N / numWorkers + remainder := b.N % numWorkers ... - wg.Wait() + wg.Wait() + // handle remainder in the current goroutine + for j := 0; j < remainder; j++ { + sl.Lock(); _ = runtime.NumGoroutine(); sl.Unlock() + }errors_test.go (1)
93-102: Unify constructor return type to TestCase for consistency.Other constructors return TestCase; align newIsErrorFn2TestCase to reduce friction and keep API uniform.
Apply:
-func newIsErrorFn2TestCase(name string, checkFn func(error) (bool, bool), - expectedIs, expectedKnown bool, errs ...error) isErrorFn2TestCase { +func newIsErrorFn2TestCase(name string, checkFn func(error) (bool, bool), + expectedIs, expectedKnown bool, errs ...error) TestCase { return isErrorFn2TestCase{ name: name, checkFn: checkFn, errs: errs, expectedIs: expectedIs, expectedKnown: expectedKnown, } }maps_test.go (1)
268-277: Correctness: do not derive ‘found’ from expected==def.If a key exists with a value equal to def, expected may equal def but found must be true. Compute found from map membership.
Apply:
func newMapValueTestCase(name string, m map[string]int, key string, def int, expected int) TestCase { + found := false + if m != nil { + _, found = m[key] + } return mapValueTestCase{ name: name, m: m, key: key, def: def, expected: expected, - found: expected != def, // derive found from whether expected == def + found: found, // true when key is present, regardless of value } }Also add a case where key exists with value equal to def to prevent regressions.
AGENTS.md (2)
10-14: Docs style: end list items with periods (project rule).Add terminal periods to satisfy docs guidelines and internal linters.
Apply:
-- [README.md](README.md) - Package overview and API reference -- [TESTING.md](TESTING.md) - Testing patterns and guidelines for all - darvaza.org projects -- [TESTING_core.md](TESTING_core.md) - Core-specific testing patterns +- [README.md](README.md) - Package overview and API reference. +- [TESTING.md](TESTING.md) - Testing patterns and guidelines for all + darvaza.org projects. +- [TESTING_core.md](TESTING_core.md) - Core-specific testing patterns.
198-201: Avoid brittle line numbers in docs.Referencing “Makefile (line 10)” and “.tmp/gen.mk:39” will drift; prefer descriptive text.
Apply:
-Before: ... Makefile (line 10) ... used ... `.tmp/gen.mk:39`. +Before: ... The Makefile defines it with an empty default and it is used by the generated rules in `.tmp/gen.mk`.Also applies to: 245-249
README.md (1)
22-30: Docs style: terminate bullets with periods (repo rule).Update this and similar lists to end each item with a period for markdownlint/CSpell/LanguageTool compliance.
Apply:
-* `Signed` - signed integer types. -* `Unsigned` - unsigned integer types. -* `Integer` - all integer types (signed and unsigned). -* `Float` - floating-point types. -* `Complex` - complex number types. -* `Bool` - boolean type. -* `String` - string type. -* `Ordered` - types that support ordering operations. +* `Signed` - signed integer types. +* `Unsigned` - unsigned integer types. +* `Integer` - all integer types (signed and unsigned). +* `Float` - floating-point types. +* `Complex` - complex number types. +* `Bool` - boolean type. +* `String` - string type. +* `Ordered` - types that support ordering operations.Consider running “make tidy” to let markdownlint/LanguageTool flag the rest across the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
AGENTS.md(3 hunks)README.md(8 hunks)TESTING.md(19 hunks)TESTING_core.md(2 hunks)addrport_test.go(8 hunks)addrs_test.go(17 hunks)as_test.go(12 hunks)compounderror_test.go(16 hunks)context_test.go(12 hunks)errgroup_test.go(18 hunks)errors_test.go(21 hunks)generics_test.go(11 hunks)lists_test.go(20 hunks)maps_test.go(25 hunks)panic_test.go(36 hunks)panicerror_test.go(12 hunks)slices_test.go(18 hunks)spinlock_test.go(6 hunks)stack_test.go(18 hunks)testing.go(4 hunks)testing_test.go(41 hunks)waitgroup_test.go(11 hunks)zero_test.go(43 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:
AGENTS.mdREADME.mdTESTING_core.mdTESTING.md
**/*.{md,go}
📄 CodeRabbit inference engine (AGENT.md)
Pass CSpell checks using the project dictionary (British English) for docs and code comments
Files:
AGENTS.mdtesting_test.golists_test.gotesting.goaddrs_test.goaddrport_test.goas_test.goREADME.mdwaitgroup_test.gopanicerror_test.gocontext_test.goslices_test.gocompounderror_test.gomaps_test.gospinlock_test.gozero_test.goerrors_test.gopanic_test.gostack_test.gogenerics_test.goTESTING_core.mderrgroup_test.goTESTING.md
**/*.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_test.golists_test.gotesting.goaddrs_test.goaddrport_test.goas_test.gowaitgroup_test.gopanicerror_test.gocontext_test.goslices_test.gocompounderror_test.gomaps_test.gospinlock_test.gozero_test.goerrors_test.gopanic_test.gostack_test.gogenerics_test.goerrgroup_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.golists_test.goaddrs_test.goaddrport_test.goas_test.gowaitgroup_test.gopanicerror_test.gocontext_test.goslices_test.gocompounderror_test.gomaps_test.gospinlock_test.gozero_test.goerrors_test.gopanic_test.gostack_test.gogenerics_test.goerrgroup_test.go
🧠 Learnings (19)
📚 Learning: 2025-07-18T16:39:19.390Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-07-18T16:39:19.390Z
Learning: Applies to AGENT.md : Update `AGENT.md` to reflect any changes in development workflow or standards.
Applied to files:
AGENTS.mdREADME.md
📚 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: Keep AGENT.md and README.md updated when workflows or APIs change
Applied to files:
AGENTS.md
📚 Learning: 2025-07-18T16:39:19.390Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-07-18T16:39:19.390Z
Learning: Applies to **/*.md : Include examples for complex concepts or common pitfalls in documentation.
Applied to files:
AGENTS.md
📚 Learning: 2025-07-18T16:39:19.390Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-07-18T16:39:19.390Z
Learning: ALWAYS run `make tidy` first before committing: fix all issues (Go formatting, whitespace, CSpell, markdownlint, shellcheck).
Applied to files:
AGENTS.md
📚 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: Run make tidy before committing and ensure all linters pass
Applied to files:
AGENTS.md
📚 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.golists_test.goas_test.gowaitgroup_test.gopanicerror_test.gocontext_test.goslices_test.gocompounderror_test.gomaps_test.gozero_test.goerrors_test.gopanic_test.gostack_test.gogenerics_test.goerrgroup_test.goTESTING.md
📚 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 : Use the public testing utilities from testing.go in tests
Applied to files:
testing_test.gopanic_test.goTESTING.md
📚 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.gotesting.goREADME.mdpanic_test.goTESTING_core.mdTESTING.md
📚 Learning: 2025-08-05T22:27:53.946Z
Learnt from: amery
PR: darvaza-proxy/core#135
File: compounderror_test.go:201-201
Timestamp: 2025-08-05T22:27:53.946Z
Learning: In Go test functions using assertion helpers like AssertSame, AssertEqual, etc., early returns after assertion calls are only necessary when there is subsequent code that should be skipped if the assertion fails. At the end of a function or code block where no further operations follow, the early return pattern is redundant since the function will naturally return anyway.
Applied to files:
testing_test.goREADME.mdTESTING_core.md
📚 Learning: 2024-11-16T07:43:16.106Z
Learnt from: amery
PR: darvaza-proxy/core#86
File: list.go:83-89
Timestamp: 2024-11-16T07:43:16.106Z
Learning: In `list.go`, the `List[T]` methods `Len` and `ForEach` handle a nil receiver gracefully.
Applied to files:
lists_test.go
📚 Learning: 2024-11-15T04:50:14.792Z
Learnt from: amery
PR: darvaza-proxy/core#84
File: list.go:160-171
Timestamp: 2024-11-15T04:50:14.792Z
Learning: In the Go `core` package's `list.go` file, the method `unsafeForEachElement` in `List[T]` does not need to check if `l` is `nil` because all public methods that call it already perform this check.
Applied to files:
lists_test.go
📚 Learning: 2025-09-07T19:20:04.539Z
Learnt from: amery
PR: darvaza-proxy/core#143
File: testing.go:407-437
Timestamp: 2025-09-07T19:20:04.539Z
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.
Applied to files:
panicerror_test.go
📚 Learning: 2025-04-17T20:14:08.093Z
Learnt from: amery
PR: darvaza-proxy/core#105
File: sync.go:11-12
Timestamp: 2025-04-17T20:14:08.093Z
Learning: In the darvaza.org/core module, the SpinLock type is being deprecated in favor of darvaza.org/x/sync/spinlock. The implementation must remain in place (not an alias) to avoid circular dependencies, as the new package likely imports from core.
Applied to files:
spinlock_test.go
📚 Learning: 2025-08-09T22:44:20.744Z
Learnt from: amery
PR: darvaza-proxy/core#135
File: zero.go:300-314
Timestamp: 2025-08-09T22:44:20.744Z
Learning: In Go's `IsSame` function implementation, structs and arrays intentionally return `false` because they are composite value types that are always copied by value, making it impossible to determine if they're "the same instance" versus just equal values. This maintains the distinction between sameness (reference equality) and equality (value equality).
Applied to files:
zero_test.go
📚 Learning: 2025-08-13T00:58:23.367Z
Learnt from: amery
PR: darvaza-proxy/core#135
File: zero.go:178-192
Timestamp: 2025-08-13T00:58:23.367Z
Learning: amery questions whether all invalid reflect.Values should be considered equivalent in IsSame() comparisons, preferring stricter sameness semantics where invalid values are not considered "the same thing" since they represent absence rather than actual comparable values.
Applied to files:
zero_test.go
📚 Learning: 2025-07-18T16:39:19.390Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-07-18T16:39:19.390Z
Learning: Applies to **/*.go : Max function results: 3.
Applied to files:
TESTING.md
📚 Learning: 2025-07-18T16:39:19.390Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-07-18T16:39:19.390Z
Learning: Applies to **/*.go : Max function length: 40 lines.
Applied to files:
TESTING.md
📚 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 **/*.go : Keep struct fields aligned to minimize padding (use fieldalignment -fix)
Applied to files:
TESTING.md
📚 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 .golangci.yml : Use golangci-lint v2 configuration (version: "2") with fieldalignment enabled
Applied to files:
TESTING.md
🧬 Code graph analysis (18)
testing_test.go (1)
testing.go (8)
T(22-33)MockT(45-51)AssertNotContain(356-369)AssertEqual(278-287)TestCase(249-252)RunTestCases(260-267)AssertMustTypeIs(991-998)AssertMustNotContain(874-879)
lists_test.go (2)
testing.go (6)
T(22-33)S(240-245)TestCase(249-252)RunTestCases(260-267)AssertFalse(570-573)AssertEqual(278-287)lists.go (6)
ListForEach(29-40)ListForEachElement(43-55)ListForEachBackward(65-76)ListForEachBackwardElement(79-91)ListContainsFn(16-26)ListCopyFn(106-125)
addrs_test.go (1)
testing.go (4)
TestCase(249-252)S(240-245)RunTestCases(260-267)T(22-33)
addrport_test.go (2)
testing.go (4)
TestCase(249-252)S(240-245)T(22-33)RunTestCases(260-267)addrport.go (1)
AddrPort(20-49)
as_test.go (1)
testing.go (4)
TestCase(249-252)T(22-33)RunTestCases(260-267)S(240-245)
waitgroup_test.go (2)
testing.go (6)
TestCase(249-252)S(240-245)RunTestCases(260-267)T(22-33)AssertNoError(398-407)AssertNil(622-631)sync.go (1)
WaitGroup(42-47)
panicerror_test.go (3)
testing.go (4)
TestCase(249-252)S(240-245)RunTestCases(260-267)T(22-33)panicerror.go (4)
Panic(90-92)Panicf(95-97)PanicWrap(100-102)PanicWrapf(106-108)errors.go (1)
ErrUnreachable(25-25)
context_test.go (1)
testing.go (3)
TestCase(249-252)T(22-33)RunTestCases(260-267)
slices_test.go (2)
testing.go (4)
TestCase(249-252)S(240-245)T(22-33)RunTestCases(260-267)constraints.go (1)
Ordered(39-41)
compounderror_test.go (1)
testing.go (4)
TestCase(249-252)S(240-245)T(22-33)RunTestCases(260-267)
maps_test.go (1)
testing.go (5)
TestCase(249-252)S(240-245)RunTestCases(260-267)T(22-33)AssertSliceEqual(318-327)
spinlock_test.go (2)
sync.go (1)
SpinLock(13-13)testing.go (6)
TestCase(249-252)S(240-245)RunTestCases(260-267)T(22-33)AssertEqual(278-287)AssertPanic(433-440)
zero_test.go (2)
testing.go (7)
S(240-245)T(22-33)RunTestCases(260-267)TestCase(249-252)AssertNil(622-631)AssertSliceEqual(318-327)AssertEqual(278-287)zero.go (2)
Zero(23-26)IsZero(67-85)
errors_test.go (1)
testing.go (3)
TestCase(249-252)T(22-33)RunTestCases(260-267)
panic_test.go (2)
testing.go (6)
TestCase(249-252)RunTestCases(260-267)T(22-33)AssertNoError(398-407)AssertEqual(278-287)S(240-245)panicerror.go (1)
NewPanicError(45-53)
stack_test.go (2)
testing.go (4)
TestCase(249-252)S(240-245)T(22-33)RunTestCases(260-267)stack.go (3)
Here(296-305)Frame(35-46)Stack(242-242)
generics_test.go (1)
testing.go (4)
TestCase(249-252)S(240-245)T(22-33)RunTestCases(260-267)
errgroup_test.go (2)
sync.go (1)
ErrGroup(128-135)testing.go (4)
TestCase(249-252)S(240-245)T(22-33)RunTestCases(260-267)
🔇 Additional comments (34)
testing.go (4)
348-369: New negative‑contain assertion looks good.Clear behaviour, explicit empty‑substring check, and consistent logging. No issues.
381-388: Inline Error/NoError base implementations are sound.Return values and messages are consistent with the new “independent base functions” model.
Also applies to: 399-406
867-879: Must‑variant wrapper is consistent.
AssertMustNotContaincorrectly composesAssertNotContainand aborts viaFailNow. Looks good.
787-800: No change required — repo targets Go 1.23.0go.mod declares 'go 1.23.0' and Go 1.22 added integer-range loops, so
for i := range numWorkers/for range b.Nare supported; keep the loops as-is.TESTING_core.md (3)
23-36: Hierarchy update reads well and matches code.Listing
AssertNotContain, and promotingAssertError/AssertNoErrorto base is consistent with testing.go.
89-92: Rationale aligns with the refactor.Clear separation of base vs derived assertions; no issues.
120-133: Fix Markdown style: end list items with full stops and use British English spellings.
- Several bullet lists in TESTING_core.md omit full stops; convert American spellings (e.g. standardize → standardise, initialize → initialise) to British English.
- File: TESTING_core.md — lines 120–133. Also check lines 135–156, 349–357, 397–401, 412–417.
- Run LanguageTool and markdownlint locally or attach CI linter output to confirm compliance.
errgroup_test.go (3)
114-144: Confirm expected outcome when shutdown returns an error.Case “worker with shutdown error” expects cancellation but treats any non‑
context.Cancelederror as a failure. IfshutdownFuncerrors propagate fromWait(), this test will be flaky/incorrect.Please confirm
ErrGroup.Waitsemantics for shutdown errors. If they can surface, setexpectError=trueor relaxcheckErrorto accept eithercontext.Canceledor the shutdown error.
283-301: Top‑level orchestrator LGTM.Clean grouping via
RunTestCasesplus focused subtests.
219-241: Nil run function panic test is correct.Panic expectation and recovery are handled properly.
testing_test.go (3)
72-95: Same circular‑test pattern repeated.The tests for AssertNotEqual, AssertSliceEqual, AssertContains, AssertNotContain, AssertError/AssertNoError also use Assert* helpers to validate base assertions.
Also applies to: 96-114, 116-139, 140-173, 175-207
699-738: Concurrency test is solid.Good use of
RunConcurrentTestand deterministic expectations per modulo case.
1457-1463: Helper for error substring checks LGTM.Concise and improves test readability.
as_test.go (2)
32-39: Constructors returning TestCase interface are a good abstraction.This aligns with the new unified test harness and keeps call‑sites decoupled.
Also applies to: 115-123, 149-155, 184-191, 250-257, 286-293
312-321: Factory + RunTestCases pattern is clean and consistent.Tables are clear, cover edge cases (nil func, nil slices), and follow the new runner.
Also applies to: 327-343, 349-359, 365-382, 388-403, 409-437
waitgroup_test.go (2)
36-47: Well-structured test case factory with clear scenarios.The factory function effectively covers the essential test scenarios: success, error, panic handling, and nil function cases. Good use of the
S()helper for slice creation.
191-210: Comprehensive test coverage for OnError handler scenarios.Excellent test cases covering successful workers, error dismissal, and error transformation scenarios.
addrs_test.go (3)
29-42: Excellent test coverage for ParseAddr function.The test cases comprehensively cover IPv4, IPv6, edge cases (unspecified addresses), invalid inputs, and zone handling. Good use of both concrete and string-based factory functions.
162-178: Well-designed test cases for AddrFromNetIP.Good coverage of various net.Addr types including edge cases like nil inputs and unsupported types (TCPAddr, UDPAddr).
400-403: Clean subtest organisation for GetInterfacesNames.Good separation of basic test cases and exclusion verification logic using subtests.
addrport_test.go (2)
21-63: Comprehensive test coverage for AddrPort function.Excellent variety of test cases covering direct types, TCP/UDP addresses, interface implementations, and invalid inputs. The factory function properly returns TestCase interface as required.
120-130: Well-structured test organisation with clear subtests.Good separation of concerns with dedicated subtests for basic functionality, type-specific handling, interface chaining, mock connections, and IPv4 handling.
slices_test.go (1)
51-61: Clean factory implementation for SliceReverse test cases.Good use of the S() helper and clear test case progression from empty to multiple elements.
context_test.go (1)
136-138: Nice consolidation to RunTestCases + factories.Consistent with the PR’s testing scaffolding: table‑driven, helpers via S(), and subtests. LGTM.
Also applies to: 225-227, 278-279
compounderror_test.go (1)
404-427: Unified runner and subtests look good.The suite is neatly partitioned; tests use Assert* helpers and log passes. LGTM.
panicerror_test.go (1)
24-31: Adopts factory + RunTestCases pattern consistently.Good alignment with the new TestCase scaffolding and subtests.
Also applies to: 245-260, 481-486, 489-490, 498-499, 512-519
spinlock_test.go (1)
58-60: Good use of TestCase factories and Assert helpers.*The SpinLock tests follow the project’s testing utilities closely and read clearly. LGTM.
Also applies to: 135-136, 182-184, 205-209
errors_test.go (1)
69-71: LGTM: migration to RunTestCases and factory builders.Factories and TestCase usage look good and match testing.go.
Also applies to: 127-129, 283-285, 457-460, 503-506, 549-551
stack_test.go (1)
350-400: LGTM: path‑handling tests for PkgFile are comprehensive.zero_test.go (1)
223-242: LGTM: TestCase factories + RunTestCases adoption.The refactor standardises test construction and execution cleanly.
maps_test.go (1)
108-119: LGTM: SortedKeys/Values and factories use the shared utilities correctly.Also applies to: 157-159, 208-210, 281-283, 321-323
panic_test.go (1)
493-500: LGTM: Must/MustOK test harness split into success/panic suites.Good reuse of runTestMust* helpers and TestCase factories.
lists_test.go (1)
14-19: LGTM: iteration helpers cover forward/backward, element/value, nil and early‑return cases.Also applies to: 37-47, 69-77, 79-87, 89-97, 99-108
README.md (1)
81-83: LGTM: README reflects API shifts (AddrPort, Zero, assertions, stack formatting) coherently.Also applies to: 94-97, 388-391, 516-531, 558-581, 654-656
…ific messaging Convert AssertError and AssertNoError from derived functions that delegate to nil assertions into independent base functions with error-specific messaging. Changes: - AssertError now directly checks err != nil and produces "expected error, got nil". - AssertNoError now directly checks err == nil and produces "unexpected error: %v". - Both functions log domain-appropriate success messages. - Update TESTING_core.md to reflect the new architecture hierarchy. - Maintain 100% test coverage and all existing tests pass. This improves clarity when debugging test failures by providing error-oriented messages rather than generic nil-checking messages. Signed-off-by: Alejandro Mery <amery@apptly.co>
Add negative variants of string containment assertions to complement the existing AssertContains functionality. Implementation: - Add AssertNotContain to verify substring absence. - Add AssertMustNotContain for fatal variant that calls FailNow. - Both functions follow established patterns with appropriate error messages. - Use "NotContain" (not "NotContains") for grammatical consistency. Testing: - Add comprehensive unit test for AssertNotContain. - Add integration test for AssertMustNotContain with MockT. - Achieve 100% test coverage for both new functions. - Overall package coverage improved to 97.4%. Documentation: - Update TESTING_core.md function hierarchy. - Add examples to TESTING.md Advanced Assertions. - Update README.md with new assertions in both basic and fatal lists. - All documentation maintains consistent formatting and passes linting. The implementation provides clear error messages: - Success: "does not contain %q". - Failure: "expected %q not to contain %q". Signed-off-by: Alejandro Mery <amery@apptly.co>
This commit addresses multiple documentation issues identified during API verification, ensuring all function signatures and descriptions accurately reflect the actual implementation. Key fixes: - Fix AddrPort function signature from AddrPort(addr, port) to AddrPort(v) and correct description to reflect extraction rather than creation. - Correct Zero function signature from Zero[T]() to Zero[T](_ *T) to show the required dummy parameter for type inference. - Update all assertion function signatures to use 'name...' instead of 'msg...' to accurately reflect actual parameter names (affects 25+ functions including AssertEqual, AssertTrue, AssertMust* variants). - Fix IsError function signatures by removing incorrect generic type parameters [T] and updating parameter lists to match implementation. The documentation now provides 100% accurate function signatures that match the actual Go implementation, improving developer experience and reducing confusion when using the API. Signed-off-by: Alejandro Mery <amery@apptly.co>
Rename AGENT.md to AGENTS.md as AGENTS.md is now the standard. Update all internal references within the renamed file and cross-references in README.md to maintain documentation integrity. Signed-off-by: Alejandro Mery <amery@apptly.co>
amery
force-pushed
the
pr-amery-testing
branch
from
b79e6ec to
cf593fd
Compare
Branch UpdateRebased to simplify PR scope: Removed:
Retained:
Added:
Result: Focused PR with 6 files changed (175 additions, 66 deletions) instead of 23 files (1,849 additions, 1,252 deletions). |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
User description
Testing Assertions and Documentation Improvements
Summary
This pull request enhances the testing infrastructure for
darvaza.org/coreby adding new assertion functions, improving error assertion architecture, and correcting API documentation inaccuracies.Key Changes
🧪 Testing Framework Enhancements
AssertNotContainandAssertMustNotContainfor negative string containment testingAssertErrorandAssertNoErrorto provide domain-specific messaging rather than delegating to generic nil-checking📚 Documentation Improvements
README.mdto match actual implementationAddrPort(v)signature (was incorrectly documented asAddrPort(addr, port))Zero[T](_ *T)signature (was incorrectly documented asZero[T]())name...parameter (was incorrectly documented asmsg...)IsErrorfamily signatures by removing incorrect generic parametersAGENT.mdtoAGENTS.mdfollowing project conventionsTESTING.mdandTESTING_core.mdTechnical Impact
Statistics
testing.go,testing_test.go,README.md,TESTING.md,TESTING_core.mdAGENT.md→AGENTS.mdArchitecture Improvements
AssertErrorandAssertNoErrorare now independent base functions with error-specific messages:AssertErrorproduces: "expected error, got nil" (failure) or "error: %v" (success)AssertNoErrorproduces: "unexpected error: %v" (failure) or "no error" (success)AssertNotContainfollows established patterns with clear messages:Implementation Details
Commit Breakdown
refactor(testing): Make error assertions independent with domain-specific messaging
AssertError/AssertNoErrorfrom derived to independent base functionsTESTING_core.mdhierarchy documentationfeat(testing): Add
AssertNotContainandAssertMustNotContainassertionsdocs: Fix API documentation inaccuracies
README.mddocs: Rename
AGENT.mdtoAGENTS.mdand update referencesQuality Assurance
Backwards Compatibility
This change maintains complete backwards compatibility. All modifications are either:
AssertNotContain/AssertMustNotContainfunctions)PR Type
Enhancement, Tests, Documentation
Description
Add
AssertNotContainandAssertMustNotContainassertion functionsRefactor error assertions to be independent with domain-specific messaging
Fix 25+ API documentation inaccuracies in README.md
Rename
AGENT.mdtoAGENTS.mdand update all referencesImprove test coverage to 97.4% with comprehensive unit tests
Diagram Walkthrough
File Walkthrough
testing.go
Add negative string assertions and refactor error assertionstesting.go
AssertNotContainfunction to verify substring absence with emptysubstring validation
AssertMustNotContainfatal variant that callsFailNow()onassertion failure
AssertErrorfrom delegating toAssertNotNilto independentimplementation with error-specific messaging
AssertNoErrorfrom delegating toAssertNilto independentimplementation with error-specific messaging
messages
testing_test.go
Add tests for new assertions and refactor test helperstesting_test.go
AssertNotContaincovering success,failure, and empty substring cases
AssertMustNotContainwith MockT for bothsuccess and failure scenarios
assertErrorContainsto reduce test codeduplication
message assertions
testAssertMustNotContainto theTestAssertMustFunctionstestsuite
AGENTS.md
Rename AGENT.md to AGENTS.md with reference updatesAGENTS.md
AGENT.mdtoAGENTS.mdAGENT.mdtoAGENTS.mdin documentationlink guidance
AGENT.mdtoAGENTS.mdREADME.md
Fix 25+ API documentation inaccuracies and parameter namingREADME.md
AddrPortfunction signature fromAddrPort(addr, port)toAddrPort(v)with corrected descriptionZero[T]function signature fromZero[T]()toZero[T](_ *T)withupdated description
IsErrorfamily signatures by removing incorrect genericparameters
msg...toname...throughout documentation
AssertNotContainto basic assertions listAssertMustNotContainto fatal assertions listAGENT.mdtoAGENTS.mdin documentation linksTESTING.md
Add AssertNotContain example to documentationTESTING.md
AssertNotContainexample to advanced assertions code snippetTESTING_core.md
Update assertion hierarchy documentation for refactored functionsTESTING_core.md
AssertNotContainto independent base functions hierarchyAssertErrorandAssertNoErrorfrom derived functions toindependent base functions
base functions with domain-specific validation
delegating to nil checks
Summary by CodeRabbit
New Features
AssertNotContainassertion for validating content exclusion in tests.Refactor
AddrPortand zero-value utility function signatures for improved usability.Documentation