T4: test quality audit — add custom-IComparable<T> + null-bound tests (closes #122)#134
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the unit test suite for IComparableExtensions to better reflect the library’s advertised behavior by (1) exercising generic dispatch with a custom IComparable<T> implementation and (2) pinning the current behavior when bound arguments are null.
Changes:
- Added a minimal custom
Moneytype implementingIComparable<Money>and new tests forIsBetween/IsInRangeboundary semantics on that type. - Added tests covering
nulllower-bound behavior forstringto lock in currentCompareTo(null)-driven semantics. - Added explanatory comments to document the intent of the new audit-related tests.
#122) Test-quality audit findings on IComparableExtensionsTests.cs. The test class was already in good shape — all assertions present, all named per the MethodName_when_x_y pattern, full boundary matrix covered for int / DateTime / string / char, both methods' null-value contracts verified. Two real gaps fixed: 1. Generic dispatch was exercised only against BCL types (int, DateTime, string, char). The README and XML docs both advertise working with custom IComparable<T> types, but no test ever instantiated one. Added a minimal Money record implementing IComparable<Money> and tests for both IsBetween and IsInRange that cover mid-range, exact-lower, and exact-upper boundary cases — exercising strict vs inclusive semantics on the custom type. 2. Null-bound behaviour was undocumented and untested. The methods only null-check value; null bounds defer to T.CompareTo. For string, CompareTo(null) returns +1, so IsBetween(null!, 'z') is true. Added Facts pinning the current behaviour for both methods so a future change can't silently break it. Pure additions — no existing tests changed. T4 audit otherwise found no nits worth changing: - No tests-that-don't-assert - No weak assertions - No Assert.Equal(null, ...) pattern misuses - No shared mutable state across runs - No implementation-detail tests (everything goes through public API) - No magic-number candidates worth named-constant extraction - No tests of behaviour the source doesn't explicitly handle 58 tests pass on net10.0 (54 before this commit + 4 new).
Chris-Wolfgang
force-pushed
the
stack/t4-test-quality-audit
branch
from
4635e65 to
0b23a18
Compare
Chris-Wolfgang
added a commit
that referenced
this pull request
Jun 1, 2026
The bulk of the T4 audit landed in #134 (custom IComparable<T> tests + null-bound contract tests). This follow-up closes the smaller edge-case gaps the audit flagged but skipped at the time: 1. Negative integer range — every existing int Theory used positive bounds (1, 10). Added matching Theories with bounds (-10, 0) for both IsBetween and IsInRange covering below, exact-lower, mid, exact-upper, above. By construction these exercise the same code path as the positive cases, but a future refactor introducing an Abs-based shortcut or an unsigned compare would now fail. 2. int.MinValue / int.MaxValue boundaries — added four Facts covering value=int.MaxValue and value=int.MinValue against (MinValue, MaxValue) bounds for both methods. Confirms that strict bounds reject the exact extremes (IsBetween → false) and inclusive bounds accept them (IsInRange → true). 3. Money record's CompareTo(null) convention documented with a comment explaining the choice ('null < any value', per the IComparable<T> guidance). 72 tests pass on net10.0 (58 after #134 + 14 new rows / Facts here).
This was referenced Jun 1, 2026
…ll tests Two Copilot findings on T4 (closes #122): 1. The block-comment header claimed null-bound semantics were 'documented' — they aren't in the public XML docs, only in this internal test header. Reworded to clearly say these tests pin *currently undocumented* implementation behaviour. The expanded comment now also includes the receiver in the examples ("m".IsInRange(...) rather than just IsInRange(...)) per the second part of the feedback. 2. Only lowerBound-null cases were tested. Added two matching upperBound-null Facts for IsBetween and IsInRange. For string, CompareTo(null) returns +1, so the upper-bound check (value.CompareTo(upperBound) < 0 / <= 0) fails and the methods return false — a different code path from the lowerBound-null case, now covered both ways.
Chris-Wolfgang
added a commit
that referenced
this pull request
Jun 2, 2026
The bulk of the T4 audit landed in #134 (custom IComparable<T> tests + null-bound contract tests). This follow-up closes the smaller edge-case gaps the audit flagged but skipped at the time: 1. Negative integer range — every existing int Theory used positive bounds (1, 10). Added matching Theories with bounds (-10, 0) for both IsBetween and IsInRange covering below, exact-lower, mid, exact-upper, above. By construction these exercise the same code path as the positive cases, but a future refactor introducing an Abs-based shortcut or an unsigned compare would now fail. 2. int.MinValue / int.MaxValue boundaries — added four Facts covering value=int.MaxValue and value=int.MinValue against (MinValue, MaxValue) bounds for both methods. Confirms that strict bounds reject the exact extremes (IsBetween → false) and inclusive bounds accept them (IsInRange → true). 3. Money record's CompareTo(null) convention documented with a comment explaining the choice ('null < any value', per the IComparable<T> guidance). 72 tests pass on net10.0 (58 after #134 + 14 new rows / Facts here).
vNext brought commit 696d3dc ('C5: dedent test class to match file-scoped namespace convention') which moved the entire class body from 8-space indent to 4-space indent. The T4 branch was still on the older 8-space indentation, so every method's [Theory]/[Fact]/braces conflicted line-for-line on indent depth. Resolution: kept all T4 additions (custom-IComparable<T> tests, null- bound contract tests, the round-2 upperBound-null follow-ups from the PR #134 Copilot review) and uniformly dedented the entire class body by 4 spaces to match vNext's C5 convention. No test logic touched. Build clean, 0 warnings 0 errors under TWEA- Release; tests pass net10.0.
Chris-Wolfgang
added a commit
that referenced
this pull request
Jun 3, 2026
The bulk of the T4 audit landed in #134 (custom IComparable<T> tests + null-bound contract tests). This follow-up closes the smaller edge-case gaps the audit flagged but skipped at the time: 1. Negative integer range — every existing int Theory used positive bounds (1, 10). Added matching Theories with bounds (-10, 0) for both IsBetween and IsInRange covering below, exact-lower, mid, exact-upper, above. By construction these exercise the same code path as the positive cases, but a future refactor introducing an Abs-based shortcut or an unsigned compare would now fail. 2. int.MinValue / int.MaxValue boundaries — added four Facts covering value=int.MaxValue and value=int.MinValue against (MinValue, MaxValue) bounds for both methods. Confirms that strict bounds reject the exact extremes (IsBetween → false) and inclusive bounds accept them (IsInRange → true). 3. Money record's CompareTo(null) convention documented with a comment explaining the choice ('null < any value', per the IComparable<T> guidance). 72 tests pass on net10.0 (58 after #134 + 14 new rows / Facts here).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
T4 test-quality audit on
IComparableExtensionsTests.cs. The testclass was already solid — all assertions present, naming pattern
followed, boundary matrix complete for int / DateTime / string /
char. Two gaps worth fixing:
Generic dispatch only exercised against BCL types. The README
and XML docs both advertise working with custom
IComparable<T>types, but no test ever instantiated one. Added a minimal
Moneyrecord implementing
IComparable<Money>plus tests for bothmethods covering mid-range, exact-lower, and exact-upper boundary
cases. Verifies strict vs inclusive semantics on the custom type.
Null-bound behaviour was undocumented and untested. The
methods only null-check
value; nulllowerBound/upperBounddefer to
T.CompareTo. For string,CompareTo(null)returns +1,so
"m".IsBetween(null!, "z")istrue. AddedFacts pinningthe current behaviour for both methods so a future change can't
silently regress it.
Audit findings — clean apart from those two gaps
Assert.True(x.Equals(y))etc.) — usesAssert.True/False/Equal/ThrowsappropriatelyAssert.Equal(null, actual)misusepublic API
outside-low, outside-high, equal-bounds, inverted-bounds, null
types (string), and immutable structs (DateTime) — and now custom
reference types (Money)
Verified
58 cases — 54 pre-existing + 4 new (2 Money + 2 null-bound).
Stacked on vNext (PR #129).