Feat: Add Same-ness Testing Functions and Improve Test Patterns #135
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds IsSame(a, b) for value vs reference identity, refactors nil handling, introduces AssertSame/AssertNotSame and many AssertMust* fatal wrappers plus generic S[T], updates docs, and refactors tests to use the new assertions with comprehensive unit tests and edge-case coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test code
participant Assert as AssertSame / AssertNotSame
participant IsSame as IsSame helper
Test->>Assert: call AssertSame(t, expected, actual, ...)
Assert->>IsSame: IsSame(expected, actual)
IsSame-->>Assert: true / false
Assert-->>Test: log success or failure (returns bool)
alt Must variant failure
Assert->>Test: FailNow / abort execution
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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 🔍(Review updated until commit 0f7833f)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 8735af2
Previous suggestionsSuggestions up to commit a408c65
Suggestions up to commit 89c67cd
|
|||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
compounderror_test.go (1)
390-393: Consider simplifying the assertion logicThe complex boolean condition could be simplified by using separate
AssertContainscalls for better clarity and more precise error messages.- condition := strings.Contains(errorStr, expectedNote) || strings.Contains(errorStr, tc.note) - if !AssertTrue(t, condition, "note in error string") { - return - } + if !AssertContains(t, errorStr, expectedNote, "formatted note in error string") && + !AssertContains(t, errorStr, tc.note, "original note in error string") { + return + }testing.go (2)
466-466: Consider improving the error message for clarityThe error message "expected same reference, got different references" might be confusing when comparing value types. Consider making it more generic to cover both value and reference comparisons.
- doError(t, name, args, "expected same reference, got different references") + doError(t, name, args, "expected same value or reference, got different")
497-497: Consider improving the error message for claritySimilar to AssertSame, the error message could be more generic to avoid confusion with value types.
- doError(t, name, args, "expected different references, got same reference") + doError(t, name, args, "expected different values or references, got same")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(2 hunks)TESTING.md(1 hunks)as_test.go(3 hunks)compounderror_test.go(15 hunks)errgroup_test.go(4 hunks)lists_test.go(1 hunks)testing.go(1 hunks)testing_test.go(1 hunks)zero.go(1 hunks)zero_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.md
📄 CodeRabbit Inference Engine (AGENT.md)
**/*.md: Enforce Markdown formatting rules, including 80-character line limits and strict formatting, using markdownlint with configuration in internal/build/markdownlint.json.
Check Markdown files for grammar and style issues using LanguageTool with configuration in internal/build/languagetool.cfg.
When editing Markdown files, ensure compliance with LanguageTool, CSpell, and Markdownlint.
End all bullet points in Markdown files with periods for consistency.
Capitalize proper nouns correctly (JavaScript, TypeScript, Markdown) in documentation.
Use consistent punctuation in examples and lists in documentation.
Provide context and 'why' explanations in documentation, not just 'what' descriptions.
Add examples for complex concepts or common pitfalls in documentation.
Update documentation when adding new tools or changing workflows.
Keep the pre-commit checklist current with project practices.
Review documentation changes for missing articles, punctuation, and compound modifiers.
Files:
TESTING.mdREADME.md
**/*.{md,go}
📄 CodeRabbit Inference Engine (AGENT.md)
Check spelling in both Markdown and Go source files using CSpell with configuration in internal/build/cspell.json.
Files:
TESTING.mdlists_test.goerrgroup_test.gocompounderror_test.goas_test.gotesting_test.gotesting.gozero.gozero_test.goREADME.md
**/*.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*.go: Use Go 1.23+ generics for type-safe utilities.
Enforce strict linting rules via revive as configured in internal/build/revive.toml: max function length 40 lines, max function results 3, max arguments 5, cognitive complexity 7, cyclomatic complexity 10.
Enforce struct field alignment optimization for memory efficiency using the fieldalignment tool.
Use golangci-lint with configuration in .golangci.yml for comprehensive Go code linting.
Files:
lists_test.goerrgroup_test.gocompounderror_test.goas_test.gotesting_test.gotesting.gozero.gozero_test.go
**/*_test.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*_test.go: Prefer table-driven tests for Go code.
Use helper functions like ST, AssertEqualT, AssertSliceEqualT, AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Provide comprehensive coverage for generic functions.
Files:
lists_test.goerrgroup_test.gocompounderror_test.goas_test.gotesting_test.gozero_test.go
README.md
📄 CodeRabbit Inference Engine (AGENT.md)
Update README.md to reflect significant changes in functionality or API.
Files:
README.md
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Use Go 1.23+ generics for type-safe utilities.
📚 Learning: applies to **/*_test.go : use helper functions like s[t](), assertequal[t](), assertsliceequal[t](),...
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Applied to files:
TESTING.mdlists_test.goerrgroup_test.gocompounderror_test.goas_test.gotesting_test.gotesting.gozero.gozero_test.goREADME.md
📚 Learning: applies to **/*_test.go : provide comprehensive coverage for generic functions....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
Applied to files:
TESTING.mdlists_test.goerrgroup_test.gocompounderror_test.goas_test.gotesting_test.gotesting.gozero.gozero_test.goREADME.md
📚 Learning: applies to readme.md : update readme.md to reflect significant changes in functionality or api....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to README.md : Update README.md to reflect significant changes in functionality or API.
Applied to files:
TESTING.mdREADME.md
📚 Learning: applies to **/*_test.go : prefer table-driven tests for go code....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Applied to files:
lists_test.goerrgroup_test.gocompounderror_test.goas_test.gotesting_test.gotesting.gozero_test.goREADME.md
📚 Learning: in the go `core` package's `list.go` file, the method `unsafeforeachelement` in `list[t]` does not n...
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.gocompounderror_test.go
📚 Learning: in `list.go`, the `list[t]` methods `len` and `foreach` handle a nil receiver gracefully....
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.gocompounderror_test.go
📚 Learning: applies to **/*.go : use go 1.23+ generics for type-safe utilities....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Use Go 1.23+ generics for type-safe utilities.
Applied to files:
compounderror_test.gozero.go
📚 Learning: when suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files with...
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Applied to files:
compounderror_test.go
📚 Learning: applies to **/*.go : enforce strict linting rules via revive as configured in internal/build/revive....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Enforce strict linting rules via revive as configured in internal/build/revive.toml: max function length 40 lines, max function results 3, max arguments 5, cognitive complexity 7, cyclomatic complexity 10.
Applied to files:
compounderror_test.go
📚 Learning: applies to **/*.go : max function length: 40 lines....
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:
compounderror_test.go
📚 Learning: applies to **/*.go : max function results: 3....
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:
compounderror_test.go
📚 Learning: `errnilreceiver`, defined in `errors.go`, is intended for use by other packages even if it's not use...
Learnt from: amery
PR: darvaza-proxy/core#82
File: errors.go:22-23
Timestamp: 2024-10-11T13:59:15.993Z
Learning: `ErrNilReceiver`, defined in `errors.go`, is intended for use by other packages even if it's not used within the current codebase.
Applied to files:
compounderror_test.go
🔇 Additional comments (19)
zero.go (5)
173-249: Excellent implementation and documentation.The
IsSamefunction is well-designed with comprehensive documentation that clearly explains the distinction between value and reference type comparisons. The implementation correctly handles edge cases including nil values, typed nils, and interface types. The structured approach using helper functions keeps the cognitive complexity manageable whilst maintaining readability.
251-260: LGTM!The
handleNilCaseshelper function correctly handles untyped nil comparisons with clear logic and appropriate return values.
262-274: LGTM!The
handleTypedNilCaseshelper function correctly handles typed nil comparisons and appropriately reuses existing helper functions for consistency.
276-290: LGTM!The
isSamePointerhelper function correctly implements the core comparison logic, appropriately distinguishing between reference types (compared by pointer) and value types (compared by value). The handling of interface types with recursive comparison is well-designed.
292-298: LGTM!The
isSameInterfacehelper function correctly handles interface comparison by extracting concrete values and using recursion. The nil check appropriately prevents issues with nil interfaces.lists_test.go (1)
222-222: Excellent modernisation of test assertions.The replacement of direct pointer comparison with
AssertNotSameimproves test consistency and leverages the new sameness comparison utilities. This provides better error messages and aligns with the modern testing patterns introduced in this PR.TESTING.md (1)
92-93: Well-documented new assertion functions.The documentation for
AssertSameandAssertNotSameis clear and appropriately describes their functionality for checking reference/value sameness. The descriptions are consistent with the existing documentation style and accurately reflect the underlying implementation.as_test.go (3)
165-167: Improved test pattern with standardised assertions.The replacement of manual length comparison with
AssertEqualand early return improves test consistency and leverages the new assertion utilities for better error reporting.
201-203: Consistent modernisation of test assertions.The update maintains consistency with other test case types by using
AssertEqualfor length comparison, providing standardised error reporting and improved test maintainability.
302-304: Consistent test modernisation completed.The final update to use
AssertEqualcompletes the consistent modernisation of test assertions across all affected test case types in this file.errgroup_test.go (4)
320-320: Appropriate use of reference equality assertion.The use of
AssertSamecorrectly verifies that the exact same error instance is preserved during subsequent cancellations, which is the intended behavior for error group cancellation semantics.
333-333: Correct assertion for sentinel error verification.The use of
AssertSameappropriately verifies that the specificcontext.Canceledsentinel error is returned when cancelling with nil, ensuring correct error group semantics.
356-356: Proper verification of error instance propagation.The use of
AssertSamecorrectly verifies that the error handler receives the exact same error instance, ensuring proper error propagation through the callback mechanism.
491-491: Appropriate verification of error pass-through behavior.The use of
AssertSamecorrectly verifies that the error catcher returns the exact same error instance without modification when the group is not cancelled, ensuring proper pass-through semantics.testing_test.go (2)
635-680: Well-structured test coverage for AssertSameThe test thoroughly covers both value and reference type scenarios, correctly verifying return values, error states, and log messages.
682-727: Comprehensive test coverage for AssertNotSameThe test properly validates the inverse behaviour of AssertSame, with good coverage of edge cases including slices with identical contents but different backing arrays.
README.md (2)
127-133: Clear and accurate documentation for IsSameThe documentation effectively explains the distinction between reference and value type comparisons, with a clear statement about nil handling.
476-479: Well-integrated testing utilities documentationThe new assertion functions are properly documented alongside existing assertions, maintaining consistency in style and format.
zero_test.go (1)
882-1090: Excellent test coverage for IsSame functionThe test suite is comprehensive and well-organised:
- Proper memory-efficient struct field alignment
- Thorough coverage of all type categories
- Clear test descriptions
- Good edge case handling, particularly for nil values and interfaces
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
zero.go (1)
277-290: Consider renamingisSamePointerto better reflect its purposeThe function name
isSamePointeris misleading as it handles both reference types (using pointer comparison) and value types (using value comparison). Consider renaming it to something likeisSameByKindorcompareByKindto better convey that it handles all types based on their kind.-// isSamePointer compares pointer equality based on the reflect value kind -func isSamePointer(va, vb reflect.Value) bool { +// isSameByKind compares values based on their reflect.Kind +func isSameByKind(va, vb reflect.Value) bool {Also update the call site at line 248:
- return isSamePointer(va, vb) + return isSameByKind(va, vb)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(2 hunks)TESTING.md(1 hunks)as_test.go(3 hunks)compounderror_test.go(12 hunks)errgroup_test.go(4 hunks)lists_test.go(1 hunks)testing.go(1 hunks)testing_test.go(1 hunks)zero.go(1 hunks)zero_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- TESTING.md
- as_test.go
- errgroup_test.go
- lists_test.go
- testing.go
- testing_test.go
- README.md
- compounderror_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{md,go}
📄 CodeRabbit Inference Engine (AGENT.md)
Check spelling in both Markdown and Go source files using CSpell with configuration in internal/build/cspell.json.
Files:
zero.gozero_test.go
**/*.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*.go: Use Go 1.23+ generics for type-safe utilities.
Enforce strict linting rules via revive as configured in internal/build/revive.toml: max function length 40 lines, max function results 3, max arguments 5, cognitive complexity 7, cyclomatic complexity 10.
Enforce struct field alignment optimization for memory efficiency using the fieldalignment tool.
Use golangci-lint with configuration in .golangci.yml for comprehensive Go code linting.
Files:
zero.gozero_test.go
**/*_test.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*_test.go: Prefer table-driven tests for Go code.
Use helper functions like ST, AssertEqualT, AssertSliceEqualT, AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Provide comprehensive coverage for generic functions.
Files:
zero_test.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
Learnt from: amery
PR: darvaza-proxy/core#135
File: compounderror_test.go:201-201
Timestamp: 2025-08-05T22:27:53.893Z
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.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Use Go 1.23+ generics for type-safe utilities.
📚 Learning: applies to **/*_test.go : provide comprehensive coverage for generic functions....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
Applied to files:
zero.gozero_test.go
📚 Learning: applies to **/*.go : use go 1.23+ generics for type-safe utilities....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Use Go 1.23+ generics for type-safe utilities.
Applied to files:
zero.go
📚 Learning: applies to **/*_test.go : use helper functions like s[t](), assertequal[t](), assertsliceequal[t](),...
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Applied to files:
zero_test.go
📚 Learning: applies to **/*_test.go : prefer table-driven tests for go code....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Applied to files:
zero_test.go
📚 Learning: in go test functions using assertion helpers like assertsame, assertequal, etc., early returns after...
Learnt from: amery
PR: darvaza-proxy/core#135
File: compounderror_test.go:201-201
Timestamp: 2025-08-05T22:27:53.893Z
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:
zero_test.go
🧬 Code Graph Analysis (1)
zero_test.go (2)
testing.go (5)
TestCase(158-161)T(20-28)AssertEqual(187-196)S(149-154)RunTestCases(169-176)zero.go (1)
IsSame(232-249)
🔇 Additional comments (2)
zero.go (1)
173-249: Well-structured implementation with good separation of concernsThe
IsSamefunction is well-designed with clear logic flow and appropriate use of helper functions to maintain low cognitive complexity. The comprehensive documentation with examples is particularly helpful.zero_test.go (1)
882-1090: Excellent test coverage with well-organised test casesThe test implementation for
IsSameis comprehensive and well-structured:
- Good field alignment optimisation in the
isSameTestCasestruct- Thorough coverage of all scenarios: value types, reference types, nil handling, different types, and interfaces
- Clear test organisation with descriptive names
- Proper use of existing test helpers and patterns
The tests validate all the behaviours documented in the
IsSamefunction.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
zero.go (1)
296-310: Review the interface handling inisSamePointer.The switch statement covers most cases well, but there's a potential issue: if none of the cases match (which shouldn't happen with valid reflect.Values), the function returns
falseby default. Consider adding a default case that handles unexpected kinds explicitly.func isSamePointer(va, vb reflect.Value) bool { var ok bool switch va.Kind() { case reflect.Ptr, reflect.Slice, reflect.Map, reflect.Chan, reflect.Func, reflect.UnsafePointer: ok = va.Pointer() == vb.Pointer() case reflect.Interface: ok = isSameInterface(va, vb) case reflect.String, reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr, reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128, reflect.Bool: ok = va.Interface() == vb.Interface() + default: + // Arrays, structs, and other composite types should use deep comparison + // For now, return false as these cases need specific handling + ok = false } return ok }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(2 hunks)TESTING.md(1 hunks)as_test.go(3 hunks)compounderror_test.go(12 hunks)errgroup_test.go(4 hunks)lists_test.go(1 hunks)testing.go(1 hunks)testing_test.go(1 hunks)zero.go(2 hunks)zero_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- testing.go
- errgroup_test.go
- lists_test.go
- TESTING.md
- as_test.go
- compounderror_test.go
- README.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{md,go}
📄 CodeRabbit Inference Engine (AGENT.md)
Check spelling in both Markdown and Go source files using CSpell with configuration in internal/build/cspell.json.
Files:
testing_test.gozero_test.gozero.go
**/*.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*.go: Use Go 1.23+ generics for type-safe utilities.
Enforce strict linting rules via revive as configured in internal/build/revive.toml: max function length 40 lines, max function results 3, max arguments 5, cognitive complexity 7, cyclomatic complexity 10.
Enforce struct field alignment optimization for memory efficiency using the fieldalignment tool.
Use golangci-lint with configuration in .golangci.yml for comprehensive Go code linting.
Files:
testing_test.gozero_test.gozero.go
**/*_test.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*_test.go: Prefer table-driven tests for Go code.
Use helper functions like ST, AssertEqualT, AssertSliceEqualT, AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Provide comprehensive coverage for generic functions.
Files:
testing_test.gozero_test.go
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
Learnt from: amery
PR: darvaza-proxy/core#135
File: compounderror_test.go:201-201
Timestamp: 2025-08-05T22:27:53.893Z
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.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Use Go 1.23+ generics for type-safe utilities.
📚 Learning: applies to **/*_test.go : use helper functions like s[t](), assertequal[t](), assertsliceequal[t](),...
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Applied to files:
testing_test.gozero_test.gozero.go
📚 Learning: in go test functions using assertion helpers like assertsame, assertequal, etc., early returns after...
Learnt from: amery
PR: darvaza-proxy/core#135
File: compounderror_test.go:201-201
Timestamp: 2025-08-05T22:27:53.893Z
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.gozero_test.gozero.go
📚 Learning: applies to **/*_test.go : provide comprehensive coverage for generic functions....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
Applied to files:
testing_test.gozero_test.gozero.go
📚 Learning: applies to **/*_test.go : prefer table-driven tests for go code....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Applied to files:
testing_test.gozero_test.go
📚 Learning: in the go `core` package's `list.go` file, the method `unsafeforeachelement` in `list[t]` does not n...
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:
zero.go
📚 Learning: applies to **/*.go : use go 1.23+ generics for type-safe utilities....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Use Go 1.23+ generics for type-safe utilities.
Applied to files:
zero.go
📚 Learning: in `list.go`, the `list[t]` methods `len` and `foreach` handle a nil receiver gracefully....
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:
zero.go
📚 Learning: use only the go standard library and minimal golang.org/x packages; zero external dependencies....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Use only the Go standard library and minimal golang.org/x packages; zero external dependencies.
Applied to files:
zero.go
📚 Learning: in go, avoid suggesting dereferencing a `nil` pointer receiver within a method to initialize it, as ...
Learnt from: amery
PR: darvaza-proxy/core#86
File: list.go:32-36
Timestamp: 2024-11-16T05:20:11.432Z
Learning: In Go, avoid suggesting dereferencing a `nil` pointer receiver within a method to initialize it, as this will cause a runtime panic.
Applied to files:
zero.go
📚 Learning: `errnilreceiver`, defined in `errors.go`, is intended for use by other packages even if it's not use...
Learnt from: amery
PR: darvaza-proxy/core#82
File: errors.go:22-23
Timestamp: 2024-10-11T13:59:15.993Z
Learning: `ErrNilReceiver`, defined in `errors.go`, is intended for use by other packages even if it's not used within the current codebase.
Applied to files:
zero.go
🧬 Code Graph Analysis (2)
testing_test.go (1)
testing.go (5)
T(20-28)MockT(32-38)AssertSame(462-471)S(149-154)AssertNotSame(493-502)
zero_test.go (2)
testing.go (6)
TestCase(158-161)T(20-28)AssertEqual(187-196)S(149-154)RunTestCases(169-176)AssertTrue(345-348)zero.go (1)
IsSame(253-263)
🔇 Additional comments (12)
zero.go (4)
133-140: LGTM! Clean refactoring with helper function.The simplification of
IsNilby extracting the reflect value conversion toasReflectValueimproves code reusability and reduces duplication.
142-150: LGTM! Well-designed helper function.The
asReflectValuehelper efficiently handles the case where the input is already areflect.Value, avoiding double-wrapping whilst maintaining the same interface for both scenarios.
194-263: Excellent implementation with comprehensive documentation.The
IsSamefunction is well-documented with clear examples and handles the distinction between value equality and reference equality correctly. The implementation follows a clear logical flow: handle untyped nils first, then delegate to reflection-based comparison.
312-324: LGTM! Robust interface comparison logic.The
isSameInterfacefunction correctly handles all three cases: both nil, one nil, and both non-nil with recursive comparison. The recursive call toIsSameelegantly handles nested interface scenarios.testing_test.go (2)
635-680: LGTM! Comprehensive test coverage for AssertSame.The test covers all essential scenarios:
- Value type comparison (integers)
- Reference type comparison (slices)
- Both success and failure cases
- Proper verification of log and error messages
- Correct use of mock reset between test cases
The test structure follows the established patterns in the file and provides excellent coverage.
683-727: LGTM! Thorough test coverage for AssertNotSame.The test properly validates the inverse logic of
AssertSame, covering:
- Different values should pass the assertion
- Same values should fail the assertion
- Reference types with different backing arrays
- Proper error message validation
The test demonstrates good understanding of the sameness semantics.
zero_test.go (6)
17-17: LGTM! Proper test case interface compliance.The compile-time verification ensures the new
isSameTestCasecorrectly implements theTestCaseinterface.
882-914: Excellent struct design with field alignment optimisation.The
isSameTestCasestruct demonstrates good memory efficiency practices:
- Large fields (interfaces, strings) placed first
- Small fields (booleans) placed last
- Clear documentation of field ordering rationale
This follows the coding guidelines for struct field alignment optimisation.
916-936: LGTM! Comprehensive value type test coverage.The test cases properly cover all basic value types with both same and different value scenarios, ensuring the value equality semantics work correctly for integers, strings, booleans, and floats.
938-991: LGTM! Thorough reference type test coverage.Excellent coverage of reference type semantics:
- Slices with same backing array vs different backing arrays
- Maps with same reference vs different references
- Channels, functions, and pointers with proper reference equality testing
- Good use of shared variables to demonstrate reference vs value distinction
The test cases clearly demonstrate the reference equality behaviour for all supported types.
1043-1104: LGTM! Comprehensive interface test cases with critical edge cases.The interface test cases provide excellent coverage including:
- All three paths through
isSameInterface()(both nil, one nil, both non-nil)- Nested interface scenarios testing recursive comparison
- Interface-wrapped reference types
- Critical nil interface cases for complete code coverage
The test cases ensure the complex interface comparison logic works correctly in all scenarios.
1106-1123: LGTM! Well-organised test structure using subtests.The main
TestIsSamefunction provides clear organisation with descriptive subtest names, making it easy to identify which category of tests failed if issues arise.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
zero_test.go (1)
1125-1168: Address test duplication and align with past feedback.The functions
testIsSameReflectInterfaceValuesandTestIsSameInterfaceReflectValuesare nearly identical and test the same scenario. This duplication should be resolved by keeping only one test function.Additionally, based on the past review comment, if this tests a known limitation in
isSamePointer(), consider either:
- Fixing the implementation to handle this case properly, or
- Adjusting the test expectations and adding a TODO comment explaining the limitation
-// Test that demonstrates reflect.Interface values falling through to false in isSamePointer -func TestIsSameInterfaceReflectValues(t *testing.T) { - // Create interface variables containing the same value - var interface1 any = 42 - var interface2 any = 42 - - // Create reflect.Values with Kind() == reflect.Interface - v1 := reflect.ValueOf(&interface1).Elem() - v2 := reflect.ValueOf(&interface2).Elem() - - // Verify these are actually interface kinds - AssertEqual(t, reflect.Interface, v1.Kind(), "v1 should be reflect.Interface") - AssertEqual(t, reflect.Interface, v2.Kind(), "v2 should be reflect.Interface") - - // This should return true since both interfaces contain the same value (42) - // But without the reflect.Interface case in isSamePointer(), it will return false - result := IsSame(v1, v2) - - // This assertion will fail, demonstrating the missing case - AssertTrue(t, result, "Same values in interface reflect.Values should be same") -}
🧹 Nitpick comments (1)
zero.go (1)
265-277: Minor: Improve comment placement.The function logic is correct, but the comment on line 272 appears after the return statement. Consider moving it before the return for better readability.
case a == nil || b == nil: - return false, true - // one nil, one not + // one nil, one not + return false, true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(2 hunks)TESTING.md(1 hunks)as_test.go(3 hunks)compounderror_test.go(12 hunks)errgroup_test.go(4 hunks)lists_test.go(1 hunks)testing.go(1 hunks)testing_test.go(1 hunks)zero.go(2 hunks)zero_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- compounderror_test.go
- lists_test.go
- as_test.go
- TESTING.md
- errgroup_test.go
- testing_test.go
- README.md
- testing.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{md,go}
📄 CodeRabbit Inference Engine (AGENT.md)
Check spelling in both Markdown and Go source files using CSpell with configuration in internal/build/cspell.json.
Files:
zero_test.gozero.go
**/*.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*.go: Use Go 1.23+ generics for type-safe utilities.
Enforce strict linting rules via revive as configured in internal/build/revive.toml: max function length 40 lines, max function results 3, max arguments 5, cognitive complexity 7, cyclomatic complexity 10.
Enforce struct field alignment optimization for memory efficiency using the fieldalignment tool.
Use golangci-lint with configuration in .golangci.yml for comprehensive Go code linting.
Files:
zero_test.gozero.go
**/*_test.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*_test.go: Prefer table-driven tests for Go code.
Use helper functions like ST, AssertEqualT, AssertSliceEqualT, AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Provide comprehensive coverage for generic functions.
Files:
zero_test.go
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
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.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Use Go 1.23+ generics for type-safe utilities.
📚 Learning: applies to **/*_test.go : use helper functions like s[t](), assertequal[t](), assertsliceequal[t](),...
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Applied to files:
zero_test.gozero.go
📚 Learning: applies to **/*_test.go : provide comprehensive coverage for generic functions....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
Applied to files:
zero_test.gozero.go
📚 Learning: applies to **/*_test.go : prefer table-driven tests for go code....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Applied to files:
zero_test.go
📚 Learning: in go test functions using assertion helpers like assertsame, assertequal, etc., early returns after...
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:
zero_test.gozero.go
📚 Learning: in go, avoid suggesting dereferencing a `nil` pointer receiver within a method to initialize it, as ...
Learnt from: amery
PR: darvaza-proxy/core#86
File: list.go:32-36
Timestamp: 2024-11-16T05:20:11.432Z
Learning: In Go, avoid suggesting dereferencing a `nil` pointer receiver within a method to initialize it, as this will cause a runtime panic.
Applied to files:
zero_test.gozero.go
📚 Learning: in the go `core` package's `list.go` file, the method `unsafeforeachelement` in `list[t]` does not n...
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:
zero.go
📚 Learning: applies to **/*.go : use go 1.23+ generics for type-safe utilities....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Use Go 1.23+ generics for type-safe utilities.
Applied to files:
zero.go
📚 Learning: in `list.go`, the `list[t]` methods `len` and `foreach` handle a nil receiver gracefully....
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:
zero.go
📚 Learning: use only the go standard library and minimal golang.org/x packages; zero external dependencies....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Use only the Go standard library and minimal golang.org/x packages; zero external dependencies.
Applied to files:
zero.go
📚 Learning: `errnilreceiver`, defined in `errors.go`, is intended for use by other packages even if it's not use...
Learnt from: amery
PR: darvaza-proxy/core#82
File: errors.go:22-23
Timestamp: 2024-10-11T13:59:15.993Z
Learning: `ErrNilReceiver`, defined in `errors.go`, is intended for use by other packages even if it's not used within the current codebase.
Applied to files:
zero.go
🔇 Additional comments (9)
zero.go (6)
133-140: LGTM: Clean refactoring that reduces duplication.The refactoring of
IsNilto use the newasReflectValuehelper eliminates duplicate type checking logic whilst maintaining the same behaviour. This improves maintainability and readability.
142-150: LGTM: Excellent helper function design.The
asReflectValuehelper effectively reduces code duplication whilst maintaining type safety. The implementation correctly handles bothreflect.Valueand other types, following the DRY principle.
178-192: LGTM: Well-structured comparison logic.The function follows a clear logical progression: type comparison, nil handling, then pointer comparison. This separation of concerns enhances readability and maintainability whilst keeping cognitive complexity low.
194-263: LGTM: Excellent documentation and clean implementation.The
IsSamefunction provides comprehensive documentation with clear examples distinguishing value types from reference types. The implementation correctly delegates to helper functions, maintaining low cognitive complexity whilst providing the intended sameness semantics.
279-294: LGTM: Consistent nil handling pattern.The function correctly implements typed nil comparison using the same logical pattern as
isSameNil, but appropriately adapted forreflect.Valuetypes. The delegation toisReflectValueNilmaintains consistency with existing patterns.
296-312: LGTM: Comprehensive type handling with elegant recursion.The function correctly handles all major Go types, appropriately distinguishing between reference types (compared by pointer) and value types (compared by value). The recursive call for interfaces is elegant and maintains the sameness semantics throughout nested structures.
zero_test.go (3)
882-914: LGTM: Excellent struct design with field alignment optimisation.The
isSameTestCasestruct demonstrates proper field alignment optimisation with clear documentation of the rationale. The implementation correctly follows the establishedTestCaseinterface pattern and uses appropriate assertion helpers.
916-1104: LGTM: Comprehensive and well-organised test coverage.The test cases provide excellent coverage across all major scenarios: value types, reference types, nil handling, and interface edge cases. The logical grouping and descriptive naming make the tests highly maintainable whilst ensuring thorough validation of the
IsSamefunction's behaviour.
1106-1123: LGTM: Well-organised test structure following established patterns.The main test function correctly uses subtests to organise different test categories and leverages the existing
RunTestCasesutility. This approach ensures clear test output and maintainable test organisation.
|
Persistent review updated to latest commit f0ac879 |
amery
force-pushed
the
pr-amery-same
branch
2 times, most recently
from
cc19333 to
d4ffb64
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(2 hunks)TESTING.md(1 hunks)as_test.go(3 hunks)compounderror_test.go(12 hunks)errgroup_test.go(4 hunks)lists_test.go(1 hunks)testing.go(1 hunks)testing_test.go(1 hunks)zero.go(2 hunks)zero_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- TESTING.md
🚧 Files skipped from review as they are similar to previous changes (8)
- lists_test.go
- testing.go
- as_test.go
- errgroup_test.go
- README.md
- compounderror_test.go
- testing_test.go
- zero_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{md,go}
📄 CodeRabbit Inference Engine (AGENT.md)
Check spelling in both Markdown and Go source files using CSpell with configuration in internal/build/cspell.json.
Files:
zero.go
**/*.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*.go: Use Go 1.23+ generics for type-safe utilities.
Enforce strict linting rules via revive as configured in internal/build/revive.toml: max function length 40 lines, max function results 3, max arguments 5, cognitive complexity 7, cyclomatic complexity 10.
Enforce struct field alignment optimization for memory efficiency using the fieldalignment tool.
Use golangci-lint with configuration in .golangci.yml for comprehensive Go code linting.
Files:
zero.go
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
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.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Use Go 1.23+ generics for type-safe utilities.
📚 Learning: applies to **/*_test.go : use helper functions like s[t](), assertequal[t](), assertsliceequal[t](),...
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Applied to files:
zero.go
📚 Learning: applies to **/*_test.go : provide comprehensive coverage for generic functions....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
Applied to files:
zero.go
📚 Learning: in the go `core` package's `list.go` file, the method `unsafeforeachelement` in `list[t]` does not n...
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:
zero.go
📚 Learning: applies to **/*.go : use go 1.23+ generics for type-safe utilities....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Use Go 1.23+ generics for type-safe utilities.
Applied to files:
zero.go
📚 Learning: in `list.go`, the `list[t]` methods `len` and `foreach` handle a nil receiver gracefully....
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:
zero.go
📚 Learning: use only the go standard library and minimal golang.org/x packages; zero external dependencies....
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Use only the Go standard library and minimal golang.org/x packages; zero external dependencies.
Applied to files:
zero.go
📚 Learning: in go, avoid suggesting dereferencing a `nil` pointer receiver within a method to initialize it, as ...
Learnt from: amery
PR: darvaza-proxy/core#86
File: list.go:32-36
Timestamp: 2024-11-16T05:20:11.432Z
Learning: In Go, avoid suggesting dereferencing a `nil` pointer receiver within a method to initialize it, as this will cause a runtime panic.
Applied to files:
zero.go
📚 Learning: `errnilreceiver`, defined in `errors.go`, is intended for use by other packages even if it's not use...
Learnt from: amery
PR: darvaza-proxy/core#82
File: errors.go:22-23
Timestamp: 2024-10-11T13:59:15.993Z
Learning: `ErrNilReceiver`, defined in `errors.go`, is intended for use by other packages even if it's not used within the current codebase.
Applied to files:
zero.go
📚 Learning: in go test functions using assertion helpers like assertsame, assertequal, etc., early returns after...
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:
zero.go
🔇 Additional comments (6)
zero.go (6)
133-140: LGTM! Simplified IsNil implementation.The refactored IsNil function is cleaner and more maintainable by extracting the reflect.Value conversion to the new asReflectValue helper. This reduces code duplication and improves readability.
142-150: LGTM! Well-designed helper function.The asReflectValue function provides a clean abstraction for converting values to reflect.Value whilst avoiding double-wrapping. The implementation is efficient and the documentation is clear.
178-192: LGTM! Clean separation of concerns.The isReflectValueSame function properly delegates to helper functions for different comparison scenarios, maintaining good readability and testability.
194-263: Excellent implementation with comprehensive documentation.The IsSame function is well-designed with:
- Clear separation between untyped nil handling and typed comparisons
- Comprehensive documentation with practical examples
- Proper delegation to helper functions to maintain cognitive complexity under the limit of 7
The function correctly distinguishes between value equality for basic types and reference equality for complex types, which addresses the core requirement stated in the PR objectives.
265-277: LGTM! Correct untyped nil handling.The isSameNil function correctly handles all untyped nil scenarios:
- Both nil: returns true
- One nil: returns false
- Neither nil: delegates to further processing
The logic is clear and the return pattern with ok boolean is well-implemented.
279-294: LGTM! Proper typed nil comparison.The isSameTypedNil function correctly handles typed nil scenarios using the same logical pattern as isSameNil. The use of isReflectValueNil for nil detection is appropriate.
Add comprehensive test cases to verify IsSame handles edge cases that could potentially cause stack overflow due to recursive interface processing. Test scenarios include: - Circular interface references that could cause infinite recursion. - Deeply nested interface chains (100+ levels) testing recursion depth. - Self-referential structures wrapped in interfaces. - Nested interfaces containing circular references. All tests pass, confirming the current IsSame implementation correctly handles these scenarios without stack overflow. Addresses qodo-merge-pro feedback from PR #135 discussion. Signed-off-by: Alejandro Mery <amery@apptly.co>
amery
changed the title
Add comprehensive test cases to verify IsSame handles edge cases that could potentially cause stack overflow due to recursive interface processing. Test scenarios include: - Circular interface references that could cause infinite recursion. - Deeply nested interface chains (100+ levels) testing recursion depth. - Self-referential structures wrapped in interfaces. - Nested interfaces containing circular references. All tests pass, confirming the current IsSame implementation correctly handles these scenarios without stack overflow. Addresses qodo-merge-pro feedback from PR #135 discussion. Signed-off-by: Alejandro Mery <amery@apptly.co>
PR Update: Rebased + New EnhancementsThis PR has been rebased and includes two additional commits: New Commits1. AssertMustSame/AssertMustNotSame Functions
2. Cross-Compatible Test Function Documentation
Status
|
|
Persistent review updated to latest commit 0f7833f |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
testing_test.go (2)
635-680: Test function exceeds linting limits
TestAssertSameis ~45 LOC, breaching the 40-line maximum enforced byrevive.
Split into table-driven sub-tests or helper functions to avoid future CI failures.
683-727: Duplicate pattern – consider consolidating
TestAssertNotSamereplicates most of the logic fromTestAssertSame, leading to maintenance overhead and again exceeds the 40-line limit.
A single parameterised table with a “negate” flag would cover both cases cleanly.zero_test.go (1)
1180-1223: Stress depth is generous but may still mask issuesThe “deep nested chain” cases build only 100 interface levels. While adequate today, a future change that introduces accidental unbounded recursion could still pass.
Consider running the same logic inside a loop (e.g. depth 10 000 in a quick benchmark) guarded byt.Parallel()to make the test future-proof, or add a targeted fuzz test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(4 hunks)TESTING.md(1 hunks)as_test.go(3 hunks)compounderror_test.go(12 hunks)errgroup_test.go(4 hunks)lists_test.go(1 hunks)testing.go(3 hunks)testing_test.go(3 hunks)zero.go(2 hunks)zero_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- TESTING.md
- lists_test.go
- as_test.go
- compounderror_test.go
- testing.go
- errgroup_test.go
- README.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{md,go}
📄 CodeRabbit Inference Engine (AGENT.md)
Check spelling in both Markdown and Go source files using CSpell with configuration in internal/build/cspell.json.
Files:
testing_test.gozero.gozero_test.go
**/*.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*.go: Use Go 1.23+ generics for type-safe utilities.
Enforce strict linting rules via revive as configured in internal/build/revive.toml: max function length 40 lines, max function results 3, max arguments 5, cognitive complexity 7, cyclomatic complexity 10.
Enforce struct field alignment optimization for memory efficiency using the fieldalignment tool.
Use golangci-lint with configuration in .golangci.yml for comprehensive Go code linting.
Files:
testing_test.gozero.gozero_test.go
**/*_test.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*_test.go: Prefer table-driven tests for Go code.
Use helper functions like ST, AssertEqualT, AssertSliceEqualT, AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Provide comprehensive coverage for generic functions.
Files:
testing_test.gozero_test.go
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
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.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Use Go 1.23+ generics for type-safe utilities.
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Applied to files:
testing_test.gozero.gozero_test.go
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
Applied to files:
testing_test.gozero.gozero_test.go
📚 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.gozero.gozero_test.go
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Applied to files:
testing_test.gozero.gozero_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:
zero.go
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Use Go 1.23+ generics for type-safe utilities.
Applied to files:
zero.go
📚 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:
zero.go
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Use only the Go standard library and minimal golang.org/x packages; zero external dependencies.
Applied to files:
zero.go
📚 Learning: 2024-11-16T05:20:11.432Z
Learnt from: amery
PR: darvaza-proxy/core#86
File: list.go:32-36
Timestamp: 2024-11-16T05:20:11.432Z
Learning: In Go, avoid suggesting dereferencing a `nil` pointer receiver within a method to initialize it, as this will cause a runtime panic.
Applied to files:
zero.gozero_test.go
📚 Learning: 2024-10-11T13:59:15.993Z
Learnt from: amery
PR: darvaza-proxy/core#82
File: errors.go:22-23
Timestamp: 2024-10-11T13:59:15.993Z
Learning: `ErrNilReceiver`, defined in `errors.go`, is intended for use by other packages even if it's not used within the current codebase.
Applied to files:
zero.go
📚 Learning: 2024-10-01T19:43:25.764Z
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Applied to files:
zero.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:
zero.go
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*.go : Enforce strict linting rules via revive as configured in internal/build/revive.toml: max function length 40 lines, max function results 3, max arguments 5, cognitive complexity 7, cyclomatic complexity 10.
Applied to files:
zero.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 length: 40 lines.
Applied to files:
zero.go
🧬 Code Graph Analysis (2)
testing_test.go (1)
testing.go (7)
T(22-33)MockT(45-51)AssertSame(553-562)S(240-245)AssertNotSame(584-593)AssertMustSame(903-908)AssertMustNotSame(918-923)
zero_test.go (2)
testing.go (6)
TestCase(249-252)T(22-33)AssertEqual(278-287)S(240-245)RunTestCases(260-267)AssertTrue(436-439)zero.go (1)
IsSame(253-263)
⏰ 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: make (1.23)
karasz
left a comment
karasz
left a comment
There was a problem hiding this comment.
This looks awesome as usual. However just for the sake of completeness don't you think that we should have at least:
func isSameUnsafePointerTestCases() []isSameTestCase {
x := 42
ptr := &x
unsafePtr1 := unsafe.Pointer(ptr)
unsafePtr2 := unsafePtr1
unsafePtr3 := unsafe.Pointer(&x) // Same address, different unsafe.Pointer
y := 42
unsafePtr4 := unsafe.Pointer(&y) // Different address
return S(
newIsSameTestCase("same unsafe pointer", unsafePtr1, unsafePtr2, true,
"same unsafe pointer should be same"),
newIsSameTestCase("unsafe pointers to same address", unsafePtr1, unsafePtr3, true,
"unsafe pointers to same address should be same"),
newIsSameTestCase("unsafe pointers to different addresses", unsafePtr1, unsafePtr4, false,
"unsafe pointers to different addresses should not be same"),
// Nil unsafe pointers
newIsSameTestCase("nil unsafe pointers",
unsafe.Pointer(nil), unsafe.Pointer(nil), true,
"nil unsafe pointers should be same"),
)
}
func isSameComplexTestCases() []isSameTestCase {
return S(
// Complex64
newIsSameTestCase("same complex64", complex64(1+2i), complex64(1+2i), true,
"same complex64 values should be same"),
newIsSameTestCase("different complex64", complex64(1+2i), complex64(1+3i), false,
"different complex64 values should not be same"),
// Complex128
newIsSameTestCase("same complex128", complex128(1+2i), complex128(1+2i), true,
"same complex128 values should be same"),
newIsSameTestCase("different complex128", complex128(1+2i), complex128(2+2i), false,
"different complex128 values should not be same"),
// Edge cases with NaN and Inf
newIsSameTestCase("complex with NaN",
complex(math.NaN(), 1), complex(math.NaN(), 1), false,
"complex with NaN should not be same (NaN != NaN)"),
newIsSameTestCase("complex with Inf",
complex(math.Inf(1), 1), complex(math.Inf(1), 1), true,
"complex with same Inf should be same"),
)
}
Absolutely |
Add comprehensive test cases to verify IsSame handles edge cases that could potentially cause stack overflow due to recursive interface processing. Test scenarios include: - Circular interface references that could cause infinite recursion. - Deeply nested interface chains (100+ levels) testing recursion depth. - Self-referential structures wrapped in interfaces. - Nested interfaces containing circular references. All tests pass, confirming the current IsSame implementation correctly handles these scenarios without stack overflow. Addresses qodo-merge-pro feedback from PR #135 discussion. Signed-off-by: Alejandro Mery <amery@apptly.co>
PR Update: Rebased and Additional Tests AddedThis PR has been rebased onto the latest main branch and updated with additional test coverage as requested. Changes in this update:
Test coverage:
Thank you @karasz for the review and suggestion to add these test cases for completeness. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
zero_test.go (1)
1134-1151: Double-check reflect.Interface handling in IsSame.These tests assert true for reflect.Value of kind Interface wrapping equal values. If isReflectValueSame/isSamePointer doesn’t special-case reflect.Interface to unwrap and recurse, this may fail.
Run the following script to verify the implementation covers reflect.Interface:
#!/bin/bash # Show places handling reflect.Interface and the relevant compare helpers echo "== Find reflect.Interface cases ==" rg -n --pretty 'reflect\.Interface' -A 6 -B 6 echo echo "== isSamePointer / isReflectValueSame implementations ==" rg -n --pretty $'^func\\s+(isSamePointer|isReflectValueSame)\\b' -A 60 -B 2 echo echo "== Calls that unwrap interface values before comparison ==" rg -n --pretty 'Elem\\(\\)|Interface\\(\\)'
🧹 Nitpick comments (4)
zero_test.go (1)
919-939: Coverage is strong; consider adding explicit struct/array “composite value types” cases.Per the documented semantics (structs/arrays are not considered “same”), add explicit cases to lock this in and avoid regressions.
Apply this diff to add composite value type cases and run them:
@@ func isSameValueTypeTestCases() []isSameTestCase { return S( @@ ) } +func isSameCompositeValueTypeTestCases() []isSameTestCase { + type comp struct{ A int; B string } + return S( + // Composite value types (arrays, structs) are not considered "same" + newIsSameTestCase("equal structs not same", comp{1, "x"}, comp{1, "x"}, false, + "structs are composite value types; sameness not defined"), + newIsSameTestCase("equal arrays not same", [2]int{1, 2}, [2]int{1, 2}, false, + "arrays are composite value types; sameness not defined"), + ) +} @@ func TestIsSame(t *testing.T) { t.Run("value types", func(t *testing.T) { RunTestCases(t, isSameValueTypeTestCases()) }) + t.Run("composite value types", func(t *testing.T) { + RunTestCases(t, isSameCompositeValueTypeTestCases()) + }) t.Run("reference types", func(t *testing.T) { RunTestCases(t, isSameReferenceTypeTestCases()) })Also applies to: 941-994, 996-1030, 1032-1044, 1046-1107, 1109-1131
testing.go (3)
553-562: Include types in assertion messages for faster diagnosis.Adding %T helps pinpoint mismatches (especially across interface-holding values).
Apply this diff to enrich messages:
func AssertSame(t T, expected, actual any, name string, args ...any) bool { t.Helper() ok := IsSame(expected, actual) if !ok { - doError(t, name, args, "expected same value or reference, got different") + doError(t, name, args, "expected same value or reference; got different (expected=%T, actual=%T)", expected, actual) } else { - doLog(t, name, args, "same value or reference") + doLog(t, name, args, "same value or reference (%T vs %T)", expected, actual) } return ok }
585-593: Mirror message improvement in AssertNotSame.Consistent, richer detail aids debugging when “same” unexpectedly holds.
Apply this diff:
func AssertNotSame(t T, expected, actual any, name string, args ...any) bool { t.Helper() ok := !IsSame(expected, actual) if !ok { - doError(t, name, args, "expected different values or references, got same") + doError(t, name, args, "expected different values or references; got same (expected=%T, actual=%T)", expected, actual) } else { - doLog(t, name, args, "different values or references") + doLog(t, name, args, "different values or references (%T vs %T)", expected, actual) } return ok }
895-909: Minor wording: prefer “sameness” over “same-ness”.This avoids potential spelling lint warnings and reads more naturally.
Apply this diff:
-// For value types (numbers, strings, booleans), same-ness means equal values. -// For reference types (slices, maps, pointers, channels, functions), same-ness +// For value types (numbers, strings, booleans), sameness means equal values. +// For reference types (slices, maps, pointers, channels, functions), sameness @@ -// For value types (numbers, strings, booleans), same-ness means equal values. -// For reference types (slices, maps, pointers, channels, functions), same-ness +// For value types (numbers, strings, booleans), sameness means equal values. +// For reference types (slices, maps, pointers, channels, functions), samenessAlso applies to: 910-923
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(4 hunks)TESTING.md(1 hunks)as_test.go(3 hunks)compounderror_test.go(12 hunks)errgroup_test.go(4 hunks)lists_test.go(1 hunks)testing.go(3 hunks)testing_test.go(3 hunks)zero.go(2 hunks)zero_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- compounderror_test.go
- errgroup_test.go
- README.md
- as_test.go
- zero.go
- TESTING.md
- testing_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{md,go}
📄 CodeRabbit Inference Engine (AGENT.md)
Check spelling in both Markdown and Go source files using CSpell with configuration in internal/build/cspell.json.
Files:
lists_test.gozero_test.gotesting.go
**/*.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*.go: Use Go 1.23+ generics for type-safe utilities.
Enforce strict linting rules via revive as configured in internal/build/revive.toml: max function length 40 lines, max function results 3, max arguments 5, cognitive complexity 7, cyclomatic complexity 10.
Enforce struct field alignment optimization for memory efficiency using the fieldalignment tool.
Use golangci-lint with configuration in .golangci.yml for comprehensive Go code linting.
Files:
lists_test.gozero_test.gotesting.go
**/*_test.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*_test.go: Prefer table-driven tests for Go code.
Use helper functions like ST, AssertEqualT, AssertSliceEqualT, AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Provide comprehensive coverage for generic functions.
Files:
lists_test.gozero_test.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Applied to files:
lists_test.gozero_test.gotesting.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-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
Applied to files:
zero_test.go
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Applied to files:
zero_test.go
📚 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.go
🧬 Code Graph Analysis (3)
lists_test.go (1)
testing.go (1)
AssertNotSame(584-593)
zero_test.go (2)
testing.go (6)
TestCase(249-252)T(22-33)AssertEqual(278-287)S(240-245)RunTestCases(260-267)AssertTrue(436-439)zero.go (1)
IsSame(253-263)
testing.go (1)
zero.go (1)
IsSame(253-263)
🔇 Additional comments (8)
zero_test.go (6)
5-9: LGTM: new imports are justified by added tests.math and unsafe are used in complex and unsafe.Pointer test cases respectively.
19-20: Good compile-time interface assertions.Verifying isSameTestCase and isSameStackOverflowTestCase implement TestCase prevents silent drift in the harness.
885-917: Well-structured test case type with field ordering for alignment.The memory-layout note and helper constructor keep the table tests lean.
1153-1231: Strong recursion/overflow edge-case coverage.Good to see deep nesting and self-referential structures covered; this guards against stack overflows and infinite recursion.
1284-1308: Unsafe pointer identity tests look correct.Covers identical address, different address, and nil cases.
1310-1332: NaN/Inf cases are correctly specified.Value-equality semantics imply NaN cannot be “same”; Inf matches when identical. Tests reflect this.
lists_test.go (1)
222-222: Nice modernisation: use AssertNotSame to assert list identity.This expresses intent more clearly than a raw pointer comparison and aligns with the new assertion suite.
testing.go (1)
196-210: Helpful Run compatibility pattern docs.The guidance for handling both testing.T and MockT via interface type assertions is clear and practical.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/build/cspell.json(1 hunks)zero.go(2 hunks)zero_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{md,go}
📄 CodeRabbit Inference Engine (AGENT.md)
Check spelling in both Markdown and Go source files using CSpell with configuration in internal/build/cspell.json.
Files:
zero.gozero_test.go
**/*.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*.go: Use Go 1.23+ generics for type-safe utilities.
Enforce strict linting rules via revive as configured in internal/build/revive.toml: max function length 40 lines, max function results 3, max arguments 5, cognitive complexity 7, cyclomatic complexity 10.
Enforce struct field alignment optimization for memory efficiency using the fieldalignment tool.
Use golangci-lint with configuration in .golangci.yml for comprehensive Go code linting.
Files:
zero.gozero_test.go
**/*_test.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*_test.go: Prefer table-driven tests for Go code.
Use helper functions like ST, AssertEqualT, AssertSliceEqualT, AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Provide comprehensive coverage for generic functions.
Files:
zero_test.go
🧠 Learnings (7)
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Add technical terms to internal/build/cspell.json for spelling and grammar tools.
Applied to files:
internal/build/cspell.json
📚 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.gozero_test.go
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
Applied to files:
zero.gozero_test.go
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Applied to files:
zero.gozero_test.go
📚 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:
zero.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:
zero.go
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Applied to files:
zero_test.go
🧬 Code Graph Analysis (1)
zero_test.go (2)
testing.go (6)
TestCase(249-252)T(22-33)AssertEqual(278-287)S(240-245)RunTestCases(260-267)AssertTrue(436-439)zero.go (1)
IsSame(253-263)
🔇 Additional comments (9)
internal/build/cspell.json (1)
65-65: LGTM: Added “skipcq” to the dictionaryThis aligns with our practice of whitelisting technical terms for tooling. Placement keeps the list alphabetically ordered.
zero.go (3)
133-140: Good simplification and consistent nil handlingSwitching IsNil to use asReflectValue + isReflectValueNil keeps behaviour consistent with the reflection helpers and handles typed nils correctly. Looks good.
146-150: Helper is clean and avoids double-wrapping reflect.ValueasReflectValue correctly returns an existing reflect.Value unchanged. This prevents accidental nesting and keeps helpers composable.
296-308: Docs accurately describe “sameness” vs “equality” and exclude structs/arraysThe commentary here is crisp and aligns with the agreed behaviour: structs/arrays are composite value types and are intentionally excluded from sameness checks.
zero_test.go (5)
1133-1151: Nice edge-case coverage for reflect.Value of interface kindVerifying that IsSame handles reflect.Values whose Kind() is Interface is valuable and matches the recursive interface path in isSamePointer. Good addition.
1109-1131: Consider adding a test for invalid reflect.Value inputs to IsSameIf IsSame receives invalid reflect.Values, current code would panic; the proposed guard in isReflectValueSame avoids that. Add a small test to codify the behaviour.
Apply this diff to add the test:
+func TestIsSameInvalidReflectValues(t *testing.T) { + var v1, v2 reflect.Value // both invalid + AssertTrue(t, IsSame(v1, v2), "both invalid reflect.Values should be considered same") + + // One invalid, one valid + AssertFalse(t, IsSame(v1, reflect.ValueOf(42)), "invalid vs valid reflect.Value should not be same") +}
1284-1312: Unsafe pointer coverage is appropriate and well-isolatedGood use of skipcq annotations and clear scenarios (same pointer, same address, different address). This matches the identity semantics expected by IsSame.
1314-1336: Complex number edge cases are covered correctlyNaN handling and Inf equivalence reflect Go’s equality semantics. Tests read clearly and align with the value-type branch in isSamePointer.
1046-1107: Interfaces and nested interfaces are well covered; aligns with the agreed recursion boundariesThe scenarios include typed nils in interfaces, nested interface values, and reference/value contents. This provides good confidence that the recursive Interface branch doesn’t blow the stack and behaves as intended.
PR Branch UpdateChanges:
The |
@karasz added, they all passed :) |
Modernise test functions to use table-driven test case pattern for better consistency and maintainability. Refactored tests: - TestIsZeroReflectValue: Now uses isZeroTestCase pattern. - TestIsNilWithReflectValue: Now uses isNilTestCase pattern. - TestIsNilReflectValue: Consolidated with TestIsNilWithReflectValue. - TestIsNilTypedInterface: Now uses isNilTestCase pattern. Benefits: - Consistent test structure across the test suite. - Better test names in output via subtests. - Easier to add new test cases. - Reduced code duplication. All tests continue to pass with no change in coverage. Signed-off-by: Alejandro Mery <amery@apptly.co>
Adds same-ness testing utilities that distinguish between value equality and reference equality for different Go types, alongside comprehensive refactoring of existing test suites to use modern assertion patterns. For value types (numbers, strings, booleans), same-ness means equal values. For reference types (slices, maps, pointers, channels, functions), same-ness means pointer equality to the same underlying data structure. New functions: - IsSame() base function with comprehensive type handling. - AssertSame() and AssertNotSame() testing assertions. - asReflectValue() helper to avoid duplicate reflect.ValueOf() calls. - isReflectValueSame() helper following established naming patterns. Key features: - Proper nil handling: typed nils of same type are considered same. - Interface support with recursive comparison of contained values. - Invalid reflect.Value protection to prevent panics. - Special handling for empty slices due to Go runtime optimisation. - Unsafe pointer and complex number support with appropriate tests. Special cases handled: - Zero-capacity slices: Go optimises make([]T, 0) to share memory. IsSame treats these as distinct to preserve expected semantics. - Invalid reflect.Values: Returns false rather than panicking when comparing invalid reflect.Values. - Interface wrapping: Recursively unwraps and compares contained values. Test modernisation: - Refactored as_test.go to use AssertEqual for length checks. - Modernised compounderror_test.go with AssertSame, AssertEqual, AssertNotNil. - Updated errgroup_test.go to use AssertSame for error instance checks. - Improved lists_test.go with AssertNotSame for independence verification. - Applied fatal assertion pattern consistently for critical preconditions. Documentation updates: - Added same-ness function documentation to README.md. - Extended TESTING.md with assertion usage examples. - Updated function documentation to match final implementation. Includes comprehensive test suite covering all scenarios: value types, reference types, nil handling, different types, interface wrapping, unsafe pointers, complex numbers, stack overflow protection, and invalid reflect.Value handling. Signed-off-by: Alejandro Mery <amery@apptly.co>
Add AssertMustSame() and AssertMustNotSame() functions that automatically call t.FailNow() on assertion failure, completing the AssertMust family for same-ness testing utilities. New functions: - AssertMustSame(t, expected, actual, msg...) - terminate on same-ness failure. - AssertMustNotSame(t, expected, actual, msg...) - terminate on difference failure. Both functions follow established patterns: - Call corresponding AssertSame/AssertNotSame functions internally. - Automatically call t.FailNow() to terminate test execution on failure. - Include comprehensive documentation with usage examples. - Support same parameter signatures as non-fatal variants. Add comprehensive test coverage with MockT.Run() testing both success (execution continues) and failure (execution terminates via FailNow) scenarios for reference type comparisons. Update README.md to document the new functions in the Fatal Assertion Functions section alongside existing AssertMust variants. Coverage maintained at 97.2% with all tests passing. Signed-off-by: Alejandro Mery <amery@apptly.co>
Add documentation for the doRun compatibility pattern that enables test functions to work with both *testing.T and MockT interfaces. Enhanced MockT.Run documentation: - Add detailed explanation of interface compatibility pattern. - Include complete code example with type assertions. - Document fallback mechanism for different testing interfaces. New README.md section: - Add "Cross-Compatible Test Functions" section with usage guidance. - Provide complete doRun helper implementation example. - Explain benefits: nested subtests, graceful degradation, type safety. This pattern enables test utilities to seamlessly support both real Go tests and mock testing scenarios, as demonstrated in related slog package bidirectional testing utilities. Signed-off-by: Alejandro Mery <amery@apptly.co>
PR Update: Enhanced IsSame ImplementationRecent ImprovementsAfter addressing feedback and further testing, several enhancements have been Bug Fixes
Test Improvements
Documentation Updates
ImpactThese changes ensure the IsSame functionality is production-ready with:
All tests pass with 97.2% coverage maintained. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
testing_test.go (4)
635-680: Good coverage for AssertSame; consider adding a function-reference success caseYou’re exercising both value and slice-reference paths. To broaden reference-type coverage without ambiguity, add a success case for identical function references.
Apply this diff to append a function-reference success case before the first Reset:
@@ AssertEqual(t, "same value test: same value or reference", lastLog, "log message on success") + // Additional success: identical function references + f1 := func() {} + f2 := f1 + result = AssertSame(mock, f1, f2, "same func reference") + AssertTrue(t, result, "AssertSame result when same func references") + mock.Reset()
682-727: Strengthen AssertNotSame with a function-reference negative caseYou already cover value and slice-reference negatives. Add a negative case for distinct function values to exercise another reference type.
Apply this diff to insert a function negative case before the first Reset:
@@ AssertEqual(t, "different value test: different values or references", lastLog, "log message on success") + // Additional success: distinct function references + f1 := func() {} + f2 := func() {} + result = AssertNotSame(mock, f1, f2, "different func references") + AssertTrue(t, result, "AssertNotSame result when different func references") + mock.Reset()
1280-1305: Add a value-type success case to MustSame to cover both semanticsThis test currently focuses on slices. Include a simple value-type success to assert MustSame also behaves correctly on value equality.
Apply this diff to add a value-type success before the Reset:
@@ AssertTrue(t, ok, "Success case should not abort") + // Additional success: identical values + ok = mock.Run("value success", func(mt T) { + AssertMustSame(mt, 10, 10, "same integer values") + mt.Log("execution continues") + }) + AssertTrue(t, ok, "Value success case should not abort") + mock.Reset()
1307-1332: Add a value-type success case to MustNotSame to cover both semanticsSimilar to MustSame, include a simple value-type path to verify MustNotSame on differing values.
Apply this diff to add a value-type success before the Reset:
@@ AssertTrue(t, ok, "Success case should not abort") + // Additional success: different values + ok = mock.Run("value success", func(mt T) { + AssertMustNotSame(mt, 10, 20, "different integer values") + mt.Log("execution continues") + }) + AssertTrue(t, ok, "Value success case should not abort") + mock.Reset()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
README.md(4 hunks)TESTING.md(1 hunks)as_test.go(3 hunks)compounderror_test.go(12 hunks)errgroup_test.go(4 hunks)internal/build/cspell.json(1 hunks)lists_test.go(1 hunks)testing.go(3 hunks)testing_test.go(3 hunks)zero.go(2 hunks)zero_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/build/cspell.json
- compounderror_test.go
- errgroup_test.go
- TESTING.md
- testing.go
- zero.go
- lists_test.go
- README.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{md,go}
📄 CodeRabbit Inference Engine (AGENT.md)
Check spelling in both Markdown and Go source files using CSpell with configuration in internal/build/cspell.json.
Files:
as_test.gozero_test.gotesting_test.go
**/*.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*.go: Use Go 1.23+ generics for type-safe utilities.
Enforce strict linting rules via revive as configured in internal/build/revive.toml: max function length 40 lines, max function results 3, max arguments 5, cognitive complexity 7, cyclomatic complexity 10.
Enforce struct field alignment optimization for memory efficiency using the fieldalignment tool.
Use golangci-lint with configuration in .golangci.yml for comprehensive Go code linting.
Files:
as_test.gozero_test.gotesting_test.go
**/*_test.go
📄 CodeRabbit Inference Engine (AGENT.md)
**/*_test.go: Prefer table-driven tests for Go code.
Use helper functions like ST, AssertEqualT, AssertSliceEqualT, AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Provide comprehensive coverage for generic functions.
Files:
as_test.gozero_test.gotesting_test.go
🧠 Learnings (7)
📓 Common learnings
Learnt from: amery
PR: darvaza-proxy/core#135
File: zero.go:178-192
Timestamp: 2025-08-13T00:58:23.340Z
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.
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Use helper functions like S[T](), AssertEqual[T](), AssertSliceEqual[T](), AssertError(), AssertBool(), AssertPanic(), AssertNoPanic(), RunConcurrentTest(), RunBenchmark(), and RunTestCases() in testutils_test.go to reduce boilerplate and improve test consistency.
Applied to files:
as_test.gozero_test.gotesting_test.go
📚 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:
as_test.gozero_test.gotesting_test.go
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Prefer table-driven tests for Go code.
Applied to files:
as_test.gozero_test.gotesting_test.go
📚 Learning: 2025-08-04T16:07:42.136Z
Learnt from: CR
PR: darvaza-proxy/core#0
File: AGENT.md:0-0
Timestamp: 2025-08-04T16:07:42.136Z
Learning: Applies to **/*_test.go : Provide comprehensive coverage for generic functions.
Applied to files:
zero_test.gotesting_test.go
📚 Learning: 2025-08-13T00:58:23.340Z
Learnt from: amery
PR: darvaza-proxy/core#135
File: zero.go:178-192
Timestamp: 2025-08-13T00:58:23.340Z
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-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
🧬 Code Graph Analysis (3)
as_test.go (1)
testing.go (1)
AssertEqual(278-287)
zero_test.go (2)
testing.go (5)
TestCase(249-252)S(240-245)T(22-33)RunTestCases(260-267)AssertEqual(278-287)zero.go (1)
IsSame(260-270)
testing_test.go (1)
testing.go (10)
T(22-33)MockT(45-51)AssertSame(553-562)AssertTrue(436-439)AssertFalse(451-454)AssertEqual(278-287)S(240-245)AssertNotSame(584-593)AssertMustSame(903-908)AssertMustNotSame(918-923)
🔇 Additional comments (13)
testing_test.go (1)
943-945: Must variants included in the test suite: LGTMAdding AssertMustSame and AssertMustNotSame to TestAssertMustFunctions keeps the suite consistent with the new helpers.
as_test.go (3)
165-167: LGTM! Modernises test with assertion helper.The replacement of manual length checking with
AssertEqualaligns with the PR's modernisation of test patterns. The early return ensures subsequent element comparisons are skipped if the length check fails, preventing potential slice bounds errors.
201-203: LGTM! Consistent with test modernisation pattern.Same improvement as the previous case - using
AssertEqualfor length validation with appropriate early return handling.
302-304: LGTM! Completes the assertion helper migration.Final instance of the length check modernisation, maintaining consistency across all slice test cases in the file.
zero_test.go (9)
5-9: LGTM! Essential imports for comprehensive testing.The addition of
mathandunsafeimports enables testing of complex numbers (including NaN/Inf edge cases) and unsafe pointer behaviour, which are critical for the comprehensive coverage of theIsSamefunction.
19-20: LGTM! TestCase interface compliance.Properly declares the new test case types as implementing the
TestCaseinterface, ensuring type safety and compatibility with the table-driven testing framework.
294-319: LGTM! Well-structured reflect.Value test cases.The test cases comprehensively cover reflect.Value scenarios including invalid values, zero/non-zero content, and nil/non-nil pointers. The function structure follows the established pattern and the test organisation is clear.
558-582: LGTM! Comprehensive reflect.Value nil testing.Excellent coverage of reflect.Value nil scenarios, including invalid values, various nil types, and basic type handling. The test case organisation is logical and follows established patterns.
673-704: LGTM! Edge case coverage for typed interfaces.Good coverage of complex nil scenarios including typed nil in interfaces, arrays/maps with nil elements, and channel operations. This helps ensure robust nil handling across different container types.
884-1148: LGTM! Comprehensive IsSame test implementation.Excellent implementation of the
IsSametest suite with several strengths:
- Comprehensive coverage: Value types, reference types, nil handling, different types, interfaces, unsafe pointers, and complex numbers
- Well-organised structure: Separate test case functions for different categories
- Edge case handling: Empty slices, zero-length slices from same array, complex interface scenarios
- Memory efficiency: Proper struct field alignment in
isSameTestCase- Clear test organisation: Uses
RunTestCasesfor structured executionThe test cases properly exercise all the critical paths of the
IsSamefunction implementation.
1150-1230: LGTM! Comprehensive invalid reflect.Value testing.Excellent coverage of edge cases:
- Interface reflect.Values: Properly tests
reflect.ValueswithKind() == reflect.Interface- Invalid reflect.Values: Covers invalid vs valid, nil vs invalid scenarios
- Cross-function testing: Tests
IsZeroandIsNilbehaviour with invalid reflect.ValuesThe expectation that invalid reflect.Values cannot be compared (returning
false) aligns with the retrieved learning about stricter sameness semantics.
1232-1419: LGTM! Excellent stack overflow protection testing.Comprehensive testing of potential stack overflow scenarios:
- Circular references: Tests both different and same circular interface references
- Deep nesting: 100-level deep nested chains to stress-test recursion limits
- Self-referential structures: Node structures with circular references
- Memory efficiency: Proper struct field alignment in
isSameStackOverflowTestCaseThese tests are crucial for ensuring the
IsSamefunction handles complex recursive structures without causing stack overflow.
1363-1419: LGTM! Complete unsafe pointer and complex number coverage.Excellent final test cases covering:
- Unsafe pointers: Same pointer, same address, different addresses, nil pointers
- Complex numbers: complex64, complex128, NaN edge cases, infinity handling
- Proper annotations: DeepSource skip comments for legitimate unsafe pointer usage
The NaN test case correctly expects
falsesinceNaN != NaNin Go, and the infinity test properly expectstruefor identical infinity values.
:) phewww! my mojo seems not that bad as it should. |
karasz
left a comment
karasz
left a comment
There was a problem hiding this comment.
Awesome work. Muchas gracias.
|
🎉 Released in v0.18.2! The same-ness testing functions and documentation improvements are now available. Available via |
User description
Add Same-ness Testing Functions and Improve Test Patterns
Summary
This PR introduces new testing utilities for same-ness comparison in Go
and modernises existing test patterns to use them. Same-ness testing
distinguishes between value equality (for basic types) and reference
equality (for reference types), filling a gap in the current testing
toolkit.
Key Features
New Functions Added
IsSame(a, b any) bool- Core function for same-ness comparison.compares by pointer equality.
AssertSame(t, expected, actual, name, args...)- Testingassertion for same references/values.
AssertNotSame(t, expected, actual, name, args...)- Testingassertion for different references/values.
AssertMustSame(t, expected, actual, name, args...)- Fatalassertion for same references/values that calls
t.FailNow()onfailure.
AssertMustNotSame(t, expected, actual, name, args...)- Fatalassertion for different references/values that calls
t.FailNow()onfailure.
Cross-Compatible Test Utilities
creating test functions that work with both
*testing.TandMockT:Test Pattern Improvements
Modernised 24 locations across 4 test files to follow documentation
standards:
AssertSame()/AssertNotSame().t.Fatal()patterns toif !Assert() { return }perdocumentation.
Technical Details
Implementation Highlights
complexity ≤7 requirements.
and interfaces.
functions.
Special Cases Handled
make([]T, 0)to share memory.IsSame treats these as distinct to preserve expected semantics.
comparing invalid reflect.Values.
annotations.
Files Changed
zero.goIsSame()implementation with helperstesting.gozero_test.gotesting_test.gocompounderror_test.goerrgroup_test.golists_test.goas_test.goREADME.mdTESTING.mdUsage Examples
Verification
Why This Matters
The existing
AssertEqual()uses deep comparison, which doesn't work fortesting same-ness of reference types like maps and slices. These new
functions provide:
equality.
This enhancement strengthens the testing foundation for both internal
library tests and external library users.
PR Type
Enhancement, Tests
Description
IsSame(),AssertSame(), andAssertNotSame()functions.AssertMustSame()andAssertMustNotSame()fatal assertion variants.*testing.TandMockT.t.Fatal()patterns toif !Assert() { return }Diagram Walkthrough
File Walkthrough
6 files
Add IsSame function with helper functionsAdd AssertSame and AssertNotSame functionsReplace manual length checks with AssertEqualModernize test patterns and error validationReplace manual error checks with AssertSameReplace pointer comparison with AssertNotSame2 files
Add comprehensive IsSame test suiteAdd tests for new assertion functions2 files
Document new IsSame and assertion functionsAdd new assertion functions to listSummary by CodeRabbit
New Features
Documentation
Tests
Chores