Skip to content

Remove all [Obsolete] members and migrate callers#5384

Merged
thomhurst merged 5 commits intomainfrom
remove-obsolete-members
Apr 4, 2026
Merged

Remove all [Obsolete] members and migrate callers#5384
thomhurst merged 5 commits intomainfrom
remove-obsolete-members

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

Summary

  • Remove ObjectBag properties from TestRegisteredContext and TestBuilderContext (replaced by StateBag)
  • Remove Timing record and ITestOutput.Timings/RecordTiming (replaced by OpenTelemetry activity spans); internal TimingEntry preserves engine functionality
  • Remove obsolete HasCount()/HasCount(int) from CollectionAssertionBase; all callers migrated to Count().IsEqualTo(N) or Count(c => c.IsEqualTo(N))
  • Remove obsolete HasLength()/HasLength(int) from AssertionExtensions; all callers migrated to Length().IsEqualTo(N) or Length(l => l.IsEqualTo(N))
  • Add Length(lambda) overload (StringLengthWithInlineAssertionAssertion) for inline assertions that preserve string context for .And chaining, matching the existing Count(lambda) pattern
  • Update docs, code examples, XUnit migration code fixer, and homepage components

Test plan

  • Solution builds with 0 errors
  • Run assertion tests: TUnit.Assertions.Tests
  • Run source generator tests: TUnit.Assertions.SourceGenerator.Tests, TUnit.Core.SourceGenerator.Tests
  • Run mock tests: TUnit.Mocks.Tests
  • Run integration tests: TUnit.IntegrationTests
  • Verify snapshot tests pass or update .verified.txt files for public API changes

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Remove all [Obsolete] members and migrate callers

Overall this is a clean, well-scoped removal PR that follows through on prior deprecation work. The migration from HasCount/HasLength to Count().IsEqualTo(N) / Length().IsEqualTo(N) is executed consistently across the entire codebase, and removing ObjectBag / Timing / RecordTiming completes an earlier breaking-change cycle properly.

A few observations worth flagging:


1. StringLengthWithInlineAssertionAssertion duplicates CollectionCountWithInlineAssertionAssertion almost exactly

The new class in TUnit.Assertions/Conditions/StringAssertions.cs is structurally identical to CollectionCountWithInlineAssertionAssertion<TCollection, TItem> in CollectionCountSource.cs. Both:

  • Accept a Func<IAssertionSource<int>, Assertion<int>?> lambda
  • Create a ValueAssertion<int> internally
  • Catch failures and return a generic "was N" failure message
  • Use "to satisfy X assertion" as their expectation string

This duplication means any future improvement (better failure messages, async evaluation, etc.) would have to be applied in two places. A shared base class or a helper method (e.g. InlineNumericAssertionHelper.Run(...)) would eliminate the duplication. The CollectionCountWithInlineAssertionAssertion pattern already exists and works — StringLengthWithInlineAssertionAssertion could simply extend or delegate to the same mechanism.


2. The new Length(lambda) overload silently swallows the inner assertion's failure message

When the lambda assertion fails, both the existing Count(lambda) and the new Length(lambda) implementations catch the exception and return a generic AssertionResult.Failed($"length was {_actualLength}"). This loses the inner assertion's error message entirely.

For example:

await Assert.That(str).Length(l => l.IsGreaterThan(10));
// Failure: "expected to satisfy length assertion but length was 3"
// vs. what the user wants: "expected length to be greater than 10 but received 3"

The CollectionCountWithInlineAssertionAssertion has the same issue — this PR is a good opportunity to fix it in both places by propagating the inner exception message rather than discarding it.


3. TimingEntry is now internal record but structurally identical to the deleted public Timing record

The deleted Timing record was:

public record Timing(string StepName, DateTimeOffset Start, DateTimeOffset End)
{
    public TimeSpan Duration => End - Start;
}

The new TimingEntry in TestContext.Output.cs is:

internal record TimingEntry(string StepName, DateTimeOffset Start, DateTimeOffset End)
{
    public TimeSpan Duration => End - Start;
}

This is intentional (it's now engine-internal) and correctly placed, but the TestExtensions.cs usage of testContext.Timings still iterates TimingEntry objects to populate TimingProperty for TRX reporting. The fact that TimingEntry lives in TUnit.Core but is only used by TUnit.Engine is fine, but worth verifying that no consumers in TUnit.Engine reference the deleted public Timing type by name (e.g. via ITestOutput.Timings which is now removed). The #pragma disable CS0618 suppressions being removed is a good sign that this is clean.


4. AssertionResult.Failed("value was null") vs. AssertionResult.Failed("collection was null") inconsistency in StringLengthWithInlineAssertionAssertion

The new class returns "value was null" for a null string, while the equivalent count assertion returns "collection was null". This is a minor wording inconsistency — either "string was null" or mirroring "value was null" across both would be more consistent.


Summary

The bulk of the diff (migrating test callsites) is correct and complete. The structural concern is the near-duplicate implementation of StringLengthWithInlineAssertionAssertion — it would be worth extracting the shared lambda-assertion execution pattern into a helper to avoid future drift. The silent failure message swallowing is a pre-existing issue in Count(lambda) that this PR inadvertently carries forward into Length(lambda) as well.

@codacy-production
Copy link
Copy Markdown

codacy-production bot commented Apr 4, 2026

Not up to standards ⛔

🔴 Issues 1 minor

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category Results
BestPractice 1 minor

View in Codacy

🟢 Metrics 6 complexity

Metric Results
Complexity 6

View in Codacy

TIP This summary will be updated as you push new changes. Give us feedback

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Remove all [Obsolete] members and migrate callers (follow-up)

Comparing against the previous review:

Addressed from prior review ✅

Duplication of inline assertion logic: The extraction of InlineAssertionHelper.ExecuteInlineAssertionAsync() correctly eliminates the near-duplicate between CollectionCountWithInlineAssertionAssertion and StringLengthWithInlineAssertionAssertion. The helper now also propagates ex.Message rather than a generic fallback string, which was the other open concern.


Remaining issues

1. LengthWrapper and CountWrapper are now dead code and should be deleted

HasLength() (no-arg overload) and HasCount() (no-arg overload) that returned these wrappers have been removed. There is no longer any way to construct a LengthWrapper or CountWrapper, yet both files remain with only doc-comment updates applied.

These classes should be deleted outright. Keeping them adds noise to the public API surface (they're public) and the dead code signals an incomplete cleanup. Their presence also means the residual "IsTypeOf is not supported after HasCount()" error messages in CountWrapper are permanently stale — further evidence they should go.

2. StringLengthWithInlineAssertionAssertion.GetExpectation() is too vague

This produces failure messages like: "expected to satisfy length assertion but value was null". The equivalent CollectionCountWithInlineAssertionAssertion likely has the same issue. A better approach would be to lazily capture the expectation from the inner assertion after it has been evaluated — the inner Assertion<int> already has its own GetExpectation() which would produce something like "to be equal to 5". Forwarding that gives: "expected length to be equal to 5 but received 3", which is much clearer.

3. InlineAssertionHelper is typed to int only

The helper signature is:

This works today since both count and length are int, but it rules out reuse for long, double, etc. without a second overload. Making it generic (ExecuteInlineAssertionAsync<T>) would cost nothing now and prevent a copy-paste if another numeric type is needed later.


Minor

  • StringLengthWithInlineAssertionAssertion.CheckAsync returns AssertionResult.Failed("value was null") for a null string, while the count equivalent uses "collection was null". "string was null" would be more specific and consistent with the count wording.

Overall

The bulk of this PR — migrating 80+ callsites and removing the public obsolete API surface — is well-executed and complete. The main actionable items are deleting the now-unreachable CountWrapper/LengthWrapper classes and improving the GetExpectation() message quality in StringLengthWithInlineAssertionAssertion.

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Remove all [Obsolete] members and migrate callers (follow-up)

Comparing against the previous review:

Addressed from prior review ✅

Duplication of inline assertion logic: The extraction of InlineAssertionHelper.ExecuteInlineAssertionAsync() correctly eliminates the near-duplicate between CollectionCountWithInlineAssertionAssertion and StringLengthWithInlineAssertionAssertion. The helper now also propagates ex.Message rather than a generic fallback string, which was the other open concern.


Remaining issues

1. LengthWrapper and CountWrapper are now dead code and should be deleted

HasLength() (no-arg overload) and HasCount() (no-arg overload) that returned these wrappers have been removed. There is no longer any way to construct a LengthWrapper or CountWrapper, yet both files remain with only doc-comment updates applied.

These classes should be deleted outright. Keeping them adds noise to the public API surface (they are public) and the dead code signals an incomplete cleanup. Their presence also means the residual "IsTypeOf is not supported after HasCount()" error messages in CountWrapper are permanently stale — further evidence they should go.

2. StringLengthWithInlineAssertionAssertion.GetExpectation() is too vague

protected override string GetExpectation()
{
    return "to satisfy length assertion";
}

This produces failure messages like: "expected to satisfy length assertion but value was null". A better approach would be to lazily capture the expectation from the inner assertion after evaluation — the inner Assertion<int> already has its own GetExpectation() which produces something like "to be equal to 5". Forwarding that gives: "expected length to be equal to 5 but received 3", which is much clearer. The CollectionCountWithInlineAssertionAssertion likely has the same gap.

3. InlineAssertionHelper is typed to int only

The helper is hard-coded to int. This works today since both count and length fit, but making it generic (ExecuteInlineAssertionAsync<T>) costs nothing now and avoids copy-paste if a long or double variant is needed later.


Minor

  • StringLengthWithInlineAssertionAssertion.CheckAsync returns AssertionResult.Failed("value was null") for a null string, while the count equivalent uses "collection was null". "string was null" would be more specific and consistent with the count wording.

Overall

The bulk of this PR — migrating 80+ callsites and removing the public obsolete API surface — is well-executed and complete. The main actionable items are deleting the now-unreachable CountWrapper/LengthWrapper classes and improving the GetExpectation() message in StringLengthWithInlineAssertionAssertion.

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Remove all [Obsolete] members and migrate callers — follow-up (3rd review)

Comparing against the previous two reviews.

What was addressed ✅

InlineAssertionHelper extraction + inner message propagation — confirmed present in commit 3cf4a1d. The shared helper deduplicates the count/length lambda-assertion execution and correctly propagates ex.Message from the inner assertion rather than replacing it with a generic fallback. This is the right design.

Third commit (96c7a43) test fixes — correctly updates tests to use Length(l => ...) where string context must be preserved for .And chaining, vs Length().IsEqualTo(N) where chaining ends. The comment in the commit message clearly explains the distinction.


Still outstanding from previous reviews

1. LengthWrapper and CountWrapper are unreachable dead code and should be deleted

Both HasLength() (no-arg, returning LengthWrapper) and HasCount() (no-arg, returning CountWrapper) have been removed. There is no longer any way to construct either class — yet both remain as public types with only their doc-comments updated.

Why this matters: They pollute the public API surface, and the NotSupportedException messages inside them (e.g. "IsTypeOf is not supported after HasCount()") are now permanently stale since HasCount() no longer exists. Leaving dead public types in an assembly that just did a deliberate API cleanup is at best confusing to users and future maintainers.

Recommendation: Delete both files.

2. StringLengthWithInlineAssertionAssertion.GetExpectation() returns a vague message

This produces failure messages like: "expected to satisfy length assertion but value was null", which tells the user nothing about what was asserted. The inner Assertion<int> has its own GetExpectation() that would produce something like "to be equal to 5".

A better approach: run the lambda against a no-op source to extract the expectation lazily, or store a reference to the inner assertion and delegate to innerAssertion.GetExpectation() after evaluation. Compare with CollectionCountWithInlineAssertionAssertion — it likely has the same gap and both could be fixed together.

3. InlineAssertionHelper is hardcoded to int

This works today since both count and length are int. Making it generic costs nothing and keeps the helper reusable if a long, double, or TimeSpan inline assertion is ever needed:


Minor (unchanged from previous review)

  • StringLengthWithInlineAssertionAssertion.CheckAsync returns AssertionResult.Failed("value was null") for a null string. The count equivalent uses "collection was null". "string was null" would be more consistent with that wording pattern.

Overall

The removal work is thorough and correct — 80+ callsites migrated consistently. The three items above are the remaining actionable items, with deleting CountWrapper/LengthWrapper being the most important since those are now live dead code in the public API.

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Remove all [Obsolete] members and migrate callers — follow-up (3rd review)

Comparing against the previous two reviews.

What was addressed ✅

InlineAssertionHelper extraction + inner message propagation — confirmed present in commit 3cf4a1d. The shared helper deduplicates the count/length lambda-assertion execution and correctly propagates ex.Message from the inner assertion rather than replacing it with a generic fallback. This is the right design.

Third commit (96c7a43) test fixes — correctly updates tests to use Length(l => ...) where string context must be preserved for .And chaining, vs Length().IsEqualTo(N) where chaining ends. The comment in the commit message clearly explains the distinction.


Still outstanding from previous reviews

1. LengthWrapper and CountWrapper are unreachable dead code and should be deleted

Both HasLength() (no-arg, returning LengthWrapper) and HasCount() (no-arg, returning CountWrapper) have been removed. There is no longer any way to construct either class — yet both remain as public types with only their doc-comments updated.

Why this matters: They pollute the public API surface, and the NotSupportedException messages inside them (e.g. "IsTypeOf is not supported after HasCount()") are now permanently stale since HasCount() no longer exists. Leaving dead public types in an assembly that just did a deliberate API cleanup is at best confusing to users and future maintainers.

Recommendation: Delete both files.

2. StringLengthWithInlineAssertionAssertion.GetExpectation() returns a vague message

protected override string GetExpectation()
{
    return "to satisfy length assertion";
}

This produces failure messages like: "expected to satisfy length assertion but value was null", which tells the user nothing about what was asserted. The inner Assertion<int> has its own GetExpectation() that would produce something like "to be equal to 5".

A better approach: store a reference to the inner assertion after the lambda is evaluated, and delegate GetExpectation() to innerAssertion.GetExpectation(). This would produce messages like "expected length to be equal to 5 but received 3". The CollectionCountWithInlineAssertionAssertion likely has the same gap and both could be fixed together.

3. InlineAssertionHelper is hardcoded to int

The current signature is:

internal static async Task<AssertionResult> ExecuteInlineAssertionAsync(
    int actualValue,
    string label,
    Func<IAssertionSource<int>, Assertion<int>?> assertion)

This works today since both count and length are int. Making it generic costs nothing now and keeps the helper reusable if a long, double, or TimeSpan inline assertion is ever needed.


Minor (unchanged from previous review)

  • StringLengthWithInlineAssertionAssertion.CheckAsync returns AssertionResult.Failed("value was null") for a null string, while the count equivalent uses "collection was null". "string was null" would be more specific and consistent with that wording.

Overall

The removal work is thorough and correct — 80+ callsites migrated consistently. The three items above are the remaining actionable items, with deleting CountWrapper/LengthWrapper being the most important since those are now live dead code in the public API.

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Remove all [Obsolete] members and migrate callers — follow-up (4th review)

Reviewing the two new commits since the last review: the merge commit and the "Regenerate public API snapshots after removing obsolete members" commit (df093be).


What the new commit does

The regenerated public API snapshots correctly reflect:

  • HasCount() and HasLength() factory methods removed from all three target frameworks (net8.0, net9.0, net10.0)
  • HasCount(int) and HasLength(int) removed
  • ObjectBag, Timing, ITestOutput.Timings, RecordTiming removed from TUnit.Core
  • StringLengthWithInlineAssertionAssertion added as a new public type

The snapshot regeneration is correct and necessary — good.


Still unresolved: CountWrapper and LengthWrapper remain as public dead code

The regenerated snapshots confirm what was flagged in the previous three reviews. Both classes still appear in the public API surface:

public class CountWrapper<TCollection, TItem> : ., .<TCollection>
    public CountWrapper(.<TCollection> context) { }
public class LengthWrapper : ., .<string>
    public LengthWrapper(.<string> context) { }

The only way to construct either of these was via HasCount() and HasLength(), which are now deleted. There is no constructor call path left — they are unreachable. Despite this, they remain public types visible to all consumers of TUnit.Assertions.

Why this matters: A future consumer browsing the assembly or IntelliSense will discover CountWrapper and LengthWrapper and have no way to use them. The NotSupportedException messages inside them reference HasCount() / HasLength(), which no longer exist, so the error guidance is permanently incorrect. Leaving them in the public API surface undermines the goal of this PR, which is specifically to clean up the public API.

Recommendation: Delete TUnit.Assertions/Conditions/Wrappers/CountWrapper.cs and TUnit.Assertions/Conditions/Wrappers/LengthWrapper.cs, then re-run snapshot tests to confirm they drop from the .verified.txt files.


Still unresolved: GetExpectation() in StringLengthWithInlineAssertionAssertion is vague

protected override string GetExpectation()
{
    return "to satisfy length assertion";
}

This produces failure messages like: "expected to satisfy length assertion but value was null". The inner Assertion<int> that the lambda returns has its own GetExpectation(). Delegating to it — after evaluating the lambda — would produce messages like "expected length to be equal to 5 but received 3", which is far more actionable for the test author. The CollectionCountWithInlineAssertionAssertion has the same gap. Both could be fixed together.


Still unresolved: InlineAssertionHelper hardcoded to int

internal static async Task<AssertionResult> ExecuteInlineAssertionAsync(
    int actualValue,
    string label,
    Func<IAssertionSource<int>, Assertion<int>?> assertion)

Making this generic (ExecuteInlineAssertionAsync<T>) costs nothing now and avoids duplication if a long, double, or TimeSpan inline assertion is ever needed. The current signature would require either a second copy of this method or a lossy int cast.


Overall

The obsolete API removal and snapshot regeneration are correct and complete. The three remaining items — dead CountWrapper/LengthWrapper classes, vague GetExpectation(), and the int-hardcoded helper — have carried through all four review iterations. The CountWrapper/LengthWrapper deletion is the most important because it directly impacts the exported public API surface that this PR set out to clean up.

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up Review

All four concerns from the prior review have been addressed. Here's a summary of what was fixed and one remaining observation.


✅ Previous Issues — All Resolved

1. Duplication of inline-assertion execution logic — Fixed via the new InlineAssertionHelper.ExecuteInlineAssertionAsync<T>(). Both CollectionCountWithInlineAssertionAssertion and StringLengthWithInlineAssertionAssertion now delegate to a single shared helper. This is exactly the right abstraction.

2. Inner failure message was silently swallowed — Fixed. Both assertions now store _innerAssertion and use _innerAssertion.InternalGetExpectation() in GetExpectation(), so failure messages reflect the actual constraint (e.g. "count to be equal to 5") rather than a generic fallback.

3. TimingEntry placement and #pragma suppressions — Clean. The internal record lives in TestContext.Output.cs, all obsolete CS0618 suppression pragmas are removed, and the TRX reporting path still works via the internal type.

4. "value was null" wording — Fixed to "string was null", consistent with the equivalent "collection was null" in the count assertion.


Minor Observation — InlineAssertionHelper propagates ex.Message as the failure detail

When resultingAssertion.AssertAsync() throws a TUnitAssertionException, ex.Message is the full formatted failure string (e.g. "Expected count to be equal to 5 but received 3"). That message then becomes the "received" portion of the outer assertion, while GetExpectation() returns "count to be equal to 5". The final failure message may read something like:

Expected count to be equal to 5 but count was: Expected count to be equal to 5 but received 3

Using just the raw value instead (e.g. AssertionResult.Failed($"{label} was {actualValue}")) would be cleaner — though this was already the pre-existing behaviour of Count(lambda), so it's not a regression introduced here. Worth a follow-up issue if it causes confusing output in practice.


Minor Observation — Inconsistency in test callsites for Length()

In OrAssertionTests.cs, Or_With_String_Assertions uses the new lambda overload:

While And_With_String_Assertions and Tests.cs use the existing chain form:

Both are valid, but the test suite would read more consistently if all migrated callsites used the same form. Not a blocking concern.


Overall

This is a well-executed cleanup PR. The breaking changes are appropriate (all removed members carried [Obsolete] warnings), the migration is complete and consistent, the abstraction improvement (InlineAssertionHelper) is the right call, and the public API surface snapshots are correctly updated. Good to go.

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up Review

All four concerns from the prior review have been addressed. Here's a summary of what was fixed and one remaining observation.


All Previous Issues Resolved

1. Duplication of inline-assertion execution logic — Fixed via the new InlineAssertionHelper.ExecuteInlineAssertionAsync<T>(). Both CollectionCountWithInlineAssertionAssertion and StringLengthWithInlineAssertionAssertion now delegate to a single shared helper. This is the right abstraction.

2. Inner failure message was silently swallowed — Fixed. Both assertions now store _innerAssertion and use _innerAssertion.InternalGetExpectation() in GetExpectation(), so failure messages reflect the actual constraint (e.g. "count to be equal to 5") rather than a generic fallback.

3. TimingEntry placement and #pragma suppressions — Clean. The internal record lives in TestContext.Output.cs, all obsolete CS0618 suppression pragmas are removed, and the TRX reporting path still works via the internal type.

4. "value was null" wording — Fixed to "string was null", consistent with the equivalent "collection was null" in the count assertion.


Minor: InlineAssertionHelper propagates ex.Message as the failure detail

When resultingAssertion.AssertAsync() throws a TUnitAssertionException, ex.Message is the full formatted failure string (e.g. "Expected count to be equal to 5 but received 3"). That message then becomes the "received" portion of the outer assertion, while GetExpectation() returns "count to be equal to 5". The final failure message could read redundantly.

Using just the raw value (e.g. AssertionResult.Failed($"{label} was {actualValue}")) would be cleaner — though this was already the pre-existing behaviour of Count(lambda), so it's not a regression. Worth a follow-up issue if it causes confusing output in practice.


Minor: Inconsistency in test callsites for Length()

In OrAssertionTests.cs, Or_With_String_Assertions uses the new lambda overload (.Length(l => l.IsEqualTo(11))), while And_With_String_Assertions and Tests.cs use the chained form (.Length().IsEqualTo(11)). Both are valid, but the test suite would read more consistently if all migrated callsites used the same form.


Overall

This is a well-executed cleanup PR. The breaking changes are appropriate (all removed members carried [Obsolete] warnings), the migration is complete and consistent across ~60 callsites, the InlineAssertionHelper abstraction is the right call, and the public API snapshots are correctly updated. Good to go.

- Remove ObjectBag properties from TestRegisteredContext and TestBuilderContext (use StateBag)
- Remove Timing record and ITestOutput.Timings/RecordTiming (replaced by OpenTelemetry spans);
  internal TimingEntry record preserves engine functionality
- Remove obsolete HasCount()/HasCount(int) from CollectionAssertionBase;
  migrate all callers to Count().IsEqualTo(N) or Count(c => c.IsEqualTo(N))
- Remove obsolete HasLength()/HasLength(int) from AssertionExtensions;
  migrate all callers to Length().IsEqualTo(N) or Length(l => l.IsEqualTo(N))
- Add Length(lambda) overload for inline assertions that preserve string
  context for .And chaining, matching the existing Count(lambda) pattern
- Update docs, code examples, and XUnit migration code fixer
…ssages

Deduplicate the lambda-assertion execution logic from
StringLengthWithInlineAssertionAssertion and
CollectionCountWithInlineAssertionAssertion into a shared
InlineAssertionHelper. Inner assertion failure messages are now
propagated instead of being replaced with a generic "count/length was N".
Length() maps context to int, so And/Or chaining after it cannot return
to string assertions. Use Length(l => l.IsEqualTo(N)) which preserves
the string context for continued chaining.
…sages, make helper generic

- Delete CountWrapper and LengthWrapper (dead code after removing HasCount/HasLength no-arg overloads)
- Make InlineAssertionHelper generic and return inner assertion for expectation delegation
- Improve GetExpectation() in StringLengthWithInlineAssertionAssertion and CollectionCountWithInlineAssertionAssertion to delegate to inner assertion (e.g. "length to be equal to 5" instead of "to satisfy length assertion")
- Fix null message from "value was null" to "string was null" for string length assertions
- Remove unused Wrappers namespace imports
- Regenerate public API snapshots
@thomhurst thomhurst force-pushed the remove-obsolete-members branch from 117d15d to 78a5a19 Compare April 4, 2026 20:14
@thomhurst thomhurst enabled auto-merge (squash) April 4, 2026 20:15
@thomhurst thomhurst disabled auto-merge April 4, 2026 20:16
@thomhurst thomhurst enabled auto-merge (squash) April 4, 2026 20:16
Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Remove all [Obsolete] members and migrate callers

Overall Assessment: This is a clean, well-scoped breaking-change PR that removes three deprecated surface areas and migrates all internal callers. The changes are straightforward and safe. A few items worth discussing before merging.


What's Good

Scope is appropriate. The PR focuses exclusively on removals: ObjectBag alias props, the Timing/ITestOutput API, and the HasCount()/HasLength() assertion overloads. It doesn't try to do anything else.

Snapshot tests are updated. All three .verified.txt files across DotNet8_0, DotNet9_0, and DotNet10_0 are updated consistently. The public API surface matches the intent.

The new StringLengthWithInlineAssertionAssertion mirrors the existing CollectionCountWithInlineAssertionAssertion pattern well and enables And chaining after Length(lambda).

Internal TimingEntry rename is correct. TestContext.Timings was converted from using the public Timing record to the new internal record TimingEntry. The engine's GetTimingProperty in TestExtensions.cs and RetryHelper/TestCoordinator still work cleanly against this internal type.

PropertyInitializationContext.ObjectBag is unaffected. That's a separate internal engine type with its own ObjectBag property, not the removed public alias.


Issues and Concerns

1. InlineAssertionHelper swallows exception details — potential loss of failure message quality

File: TUnit.Assertions/Conditions/Helpers/InlineAssertionHelper.cs

catch (Exception ex)
{
    return (AssertionResult.Failed(ex.Message), resultingAssertion);
}

This catches all exceptions including non-assertion exceptions (e.g., NullReferenceException, InvalidOperationException inside a custom assertion). These would be silently converted into a failed AssertionResult whose message is the raw exception message, stripped of the stack trace. The outer assertion chain would then report something like "Object reference not set to an instance of an object" with no context about where it came from.

The original CollectionCountWithInlineAssertionAssertion code had the same issue, so this is not a regression — but the refactor is a good opportunity to tighten it. Consider filtering to BaseAssertionException only:

catch (BaseAssertionException ex)
{
    return (AssertionResult.Failed(ex.Message), resultingAssertion);
}
// Let non-assertion exceptions propagate naturally

Why this matters: Programming errors (NPE, invalid cast, etc.) inside a lambda assertion would be silently turned into assertion failures, making them very hard to diagnose.

2. Weakened error message assertions in tests reduce regression detection

In ParseAssertionTests.cs:

// Before (specific):
}).ThrowsException().And.HasMessageContaining("HasLength(4)");

// After (very broad):
}).ThrowsException().And.HasMessageContaining("length");

"length" could match unrelated parts of an error message. Similarly in MemberCollectionAssertionTests.cs, "but received 2" was weakened to "received 2". Consider matching the actual failure message format more specifically:

}).ThrowsException().And.HasMessageContaining("length equal to 4");

Why this matters: Overly broad message assertions can produce false-passes if the error message format changes significantly — the very regression these tests are meant to catch.

3. GetExpectation() called before CheckAsync() in some code paths

Both StringLengthWithInlineAssertionAssertion and CollectionCountWithInlineAssertionAssertion set _innerAssertion during CheckAsync but read it in GetExpectation(). If GetExpectation() is called before the assertion runs (e.g., in AssertMultiple error building before this assertion has been awaited), the fallback message "to satisfy count/length assertion" is less informative than it could be.

A small improvement: capture the caller expression at construction time via [CallerArgumentExpression] to use in the fallback message. Not blocking, but worth noting as a design smell in the pattern being established.

4. Asymmetry between Length(lambda) and Count(lambda) test coverage

In the test files, Or chains use the lambda form while And chains use the non-lambda form for length assertions. Both forms should work in both chain types. The asymmetry in tests suggests the cross-combination (Or + non-lambda, And + lambda) may not be explicitly tested.


No Issues Found

  • AOT compatibility: No new reflection usage. The InlineAssertionHelper uses generics without unbounded reflection. No issues.
  • Source-gen vs reflection duality: The removed APIs were in TUnit.Assertions and TUnit.Core public surface. Engine internals are unaffected.
  • Performance: No hot-path regressions. The ICollection fast-path for .Count is preserved.
  • #pragma warning disable CS0618 cleanup: Correctly removed throughout engine code now that the backing types are no longer [Obsolete].

Summary

Priority Issue
High InlineAssertionHelper catches all Exception types — should restrict to BaseAssertionException to avoid masking programming errors
Medium Weakened error message assertions in tests reduce specificity and could mask regressions
Low GetExpectation() before CheckAsync() pattern is a design smell worth commenting or improving

@thomhurst thomhurst merged commit df31e91 into main Apr 4, 2026
13 of 15 checks passed
@thomhurst thomhurst deleted the remove-obsolete-members branch April 4, 2026 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant