refactor: convert 15 manual assertions to [GenerateAssertion]#5029
refactor: convert 15 manual assertions to [GenerateAssertion]#5029
Conversation
Replace manually-written Assertion<T> classes with [GenerateAssertion]-decorated static methods, eliminating boilerplate while preserving assertion behavior. Converted assertions: - Comparison: IsGreaterThan, IsGreaterThanOrEqualTo, IsLessThan, IsLessThanOrEqualTo - Null: IsNull (InlineMethodBody for correct null-passes semantics) - Reference: IsSameReferenceAs, IsNotSameReferenceAs (InlineMethodBody) - String: IsEmpty, IsNotEmpty - Enum: HasFlag, DoesNotHaveFlag, HasSameNameAs, DoesNotHaveSameNameAs, HasSameValueAs, DoesNotHaveSameValueAs
There was a problem hiding this comment.
Code Review
This is a solid architectural direction — using [GenerateAssertion] to eliminate boilerplate is exactly what the attribute was designed for, and removing ~500 lines of repetitive class definitions is a clear win. The approach is well-structured and well-tested. That said, there are a few issues worth addressing.
Issue 1: Generated class names pollute the public API (significant)
The generated names like TValue_IsGreaterThan_TValue_Assertion<TValue> are implementation details that have leaked into the public API surface. In AssertionExtensions.cs and CountWrapper.cs, these now appear as return types of public methods:
// Before — readable
public static GreaterThanAssertion<DateTime> IsAfter(...)
public static LessThanAssertion<int> LessThan(...)
public static GreaterThanAssertion<int> Positive()
// After — ugly
public static TValue_IsGreaterThan_TValue_Assertion<DateTime> IsAfter(...)
public static TValue_IsLessThan_TValue_Assertion<int> LessThan(...)
public static TValue_IsGreaterThan_TValue_Assertion<int> Positive()End users hovering over these in IntelliSense will see the cryptic generated names. The old names like GreaterThanAssertion<T> were semantically meaningful and part of the intended public API. This is also reflected in the TUnit.PublicAPI snapshot changes — you're swapping readable type names for generated ones in the public contract.
Suggested approach: Either (a) ask the source generator team to support a ClassName parameter on [GenerateAssertion] to control the generated class name, or (b) add [Obsolete]-style type aliases for the old names if binary compatibility matters. Alternatively, consider whether these methods actually need to expose the concrete assertion type in their return signature, or whether they can return a base/interface type instead.
Issue 2: Null handling regression for comparison assertions (significant)
The old GreaterThanAssertion, LessThanAssertion, etc. had an explicit null guard:
// Old code
if (value == null)
{
return Task.FromResult(AssertionResult.Failed("received null"));
}The new generated code doesn't have this:
// New code — no null check
public static AssertionResult IsGreaterThan<TValue>(this TValue value, TValue minimum)
where TValue : IComparable<TValue>
{
return value.CompareTo(minimum) > 0 // 💥 NullReferenceException if value is null
? AssertionResult.Passed
: AssertionResult.Failed($"received {value}");
}Since TValue : IComparable<TValue> doesn't exclude reference types (e.g., string), a null value will throw NullReferenceException instead of producing a proper assertion failure. This is a behavioral regression for nullable reference types.
The test suite likely doesn't cover this case because existing tests pass, but it would affect users asserting on nullable string comparisons or other nullable comparable reference types.
Issue 3: Misleading error messages for IsEmpty/IsNotEmpty with null strings (moderate)
Old behavior:
IsEmpty(null)→ fails with"value was null"✓ clearIsNotEmpty(null)→ fails with"value was null"✓ clear
New behavior:
public static AssertionResult IsEmpty(this string value)
{
return value == string.Empty
? AssertionResult.Passed
: AssertionResult.Failed($"received \"{value}\""); // 🐛 null renders as ""
}When value is null, the interpolation $"received \"{null}\"" renders as received "" — which looks identical to the empty-string case. A developer seeing this failure message would think the string was empty, not null. The same issue affects IsNotEmpty.
This is arguably worse than the "Actual value is null" message the PR mentions as an acceptable change, since it produces a misleading message rather than just a different one.
Minor observation: file-scoped access modifier usage
Using file on NullAssertionExtension and ReferenceAssertionExtensions for the InlineMethodBody = true cases is a nice touch — it correctly hides implementation details that users shouldn't call directly. The [EditorBrowsable(EditorBrowsableState.Never)] pattern used on the other extensions serves a similar purpose well.
Summary
| Issue | Severity | Blocking? |
|---|---|---|
| Generated type names in public API | Significant | Worth discussing — may be acceptable tradeoff if source gen can't support custom names |
| Null regression for comparison assertions | Significant | Yes — behavioral regression |
| Misleading null message for IsEmpty/IsNotEmpty | Moderate | Yes — produces actively confusing output |
The refactor goal is excellent and the structure is clean. The main asks are: restore explicit null handling for the comparison and string assertions, and ideally find a way to control the generated class names to preserve the public API readability.
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.17.36 to 1.18.0. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.18.0 <!-- Release notes generated using configuration in .github/release.yml at v1.18.0 --> ## What's Changed ### Other Changes * refactor: convert 15 manual assertions to [GenerateAssertion] by @thomhurst in thomhurst/TUnit#5029 * Fix invisible chart labels on benchmark pages by @Copilot in thomhurst/TUnit#5033 * docs: fix position of `--results-directory` in documentation by @vbreuss in thomhurst/TUnit#5038 * fix: IsEquivalentTo falls back to Equals() for types with no public members by @thomhurst in thomhurst/TUnit#5041 * perf: make test metadata creation fully synchronous by @thomhurst in thomhurst/TUnit#5045 * perf: eliminate <>c display class from generated TestSource classes by @thomhurst in thomhurst/TUnit#5047 * perf: generate per-class helper to reduce JIT compilations by ~18,000 by @thomhurst in thomhurst/TUnit#5048 * perf: consolidate per-method TestSource into per-class TestSource (~27k fewer JITs) by @thomhurst in thomhurst/TUnit#5049 * perf: eliminate per-class TestSource .ctor JITs via delegate registration by @thomhurst in thomhurst/TUnit#5051 * feat: rich HTML test reports by @thomhurst in thomhurst/TUnit#5044 ### Dependencies * chore(deps): update tunit to 1.17.54 by @thomhurst in thomhurst/TUnit#5028 * chore(deps): update dependency polyfill to 9.13.0 by @thomhurst in thomhurst/TUnit#5035 * chore(deps): update dependency polyfill to 9.13.0 by @thomhurst in thomhurst/TUnit#5036 **Full Changelog**: thomhurst/TUnit@v1.17.54...v1.18.0 ## 1.17.54 <!-- Release notes generated using configuration in .github/release.yml at v1.17.54 --> ## What's Changed ### Other Changes * docs: restructure, deduplicate, and clean up documentation by @thomhurst in thomhurst/TUnit#5019 * docs: trim, deduplicate, and restructure sidebar by @thomhurst in thomhurst/TUnit#5020 * fix: add newline to github reporter summary to fix rendering by @robertcoltheart in thomhurst/TUnit#5023 * docs: consolidate hooks, trim duplication, and restructure sidebar by @thomhurst in thomhurst/TUnit#5024 * Redesign mixed tests template by @thomhurst in thomhurst/TUnit#5026 * feat: add IsAssignableFrom<T>() and IsNotAssignableFrom<T>() assertions by @thomhurst in thomhurst/TUnit#5027 ### Dependencies * chore(deps): update tunit to 1.17.36 by @thomhurst in thomhurst/TUnit#5018 * chore(deps): update actions/upload-artifact action to v7 by @thomhurst in thomhurst/TUnit#5015 * chore(deps): update dependency microsoft.testing.extensions.codecoverage to 18.5.1 by @thomhurst in thomhurst/TUnit#5025 **Full Changelog**: thomhurst/TUnit@v1.17.36...v1.17.54 Commits viewable in [compare view](thomhurst/TUnit@v1.17.36...v1.18.0). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
Assertion<T>classes with[GenerateAssertion]-decorated static methods, eliminating ~500 lines of boilerplateIAssertionSource<T>extension methodsConverted assertions
IsGreaterThan,IsGreaterThanOrEqualTo,IsLessThan,IsLessThanOrEqualToAssertionResultreturn,IComparable<TValue>constraintIsNullbool+InlineMethodBody(null must pass, not fail)IsSameReferenceAs,IsNotSameReferenceAsbool+InlineMethodBody(ReferenceEquals(null, null) must return true)IsEmpty,IsNotEmptyAssertionResultreturnHasFlag,DoesNotHaveFlag,HasSameNameAs,DoesNotHaveSameNameAs,HasSameValueAs,DoesNotHaveSameValueAsAssertionResultreturn,struct, EnumconstraintMinor behavioral changes (acceptable)
"threw {Name}""threw {FullName}""received null"/"value was null""Actual value is null""received {value}"/"received different references""found {value}"GreaterThanAssertion<TValue>TValue_IsGreaterThan_TValue_Assertion<TValue>public classpublic sealed classTest plan
TUnit.Assertions.Tests— 1813 tests passTUnit.Assertions.SourceGenerator.Tests— 56 tests passTUnit.Core.SourceGenerator.Tests— 114 tests pass (1 pre-existing skip)TUnit.PublicAPI— 3 tests pass (snapshots updated).received.txtfiles remaining