feat: add IsAssignableFrom<T>() and IsNotAssignableFrom<T>() assertions#5027
feat: add IsAssignableFrom<T>() and IsNotAssignableFrom<T>() assertions#5027
Conversation
Closes #4824. Adds the counterpart to IsAssignableTo<T>() — while IsAssignableTo checks "can this value be assigned to a variable of type T?", IsAssignableFrom checks "can a value of type T be assigned to a variable of this value's type?". Implemented across all assertion source classes, with tests, public API snapshots, and documentation updates.
There was a problem hiding this comment.
Code Review: and Assertions
Overall this is a well-structured PR that correctly mirrors the existing pattern. The implementation is consistent and the API snapshots are all updated. A few observations worth considering:
Implementation Correctness ✅
The core logic is correct:
- : — "can TSource be assigned to this value's runtime type?"
- : negation of the above
The null/exception handling pattern is identical to , which is appropriate.
Architecture: ExpressionBuilder Duplication
The call is copy-pasted 18 times across 15 source files (same duplication exists in ). The reason this lives in the factory method rather than the assertion constructor is presumably so the expression captures the right context timing.
However, it would be cleaner to move this into the assertion class constructor itself — the type name is available there via and doesn't change:
This would reduce drift risk if the string format ever needs to change. That said, has this same pattern, so changing it here alone would be inconsistent — it's probably better as a separate refactor across all type assertions.
Semantic Concern: Counterintuitive Direction
is already confusing in .NET's reflection API — meaning "Dog can be assigned to Animal" is notoriously backwards-feeling. The PR adds solid XML docs and the test helps validate the chaining, but consider whether the assertion name could be clearer, e.g. . This is a design call, not a blocker.
Missing Edge Case Tests
The test suite is good but misses two scenarios that also lacks coverage for:
-
Null value behavior: What happens when the asserted value is ? The implementation returns — there's no test verifying this is the expected behavior.
-
Exception chaining scenario: explicitly handles the case where (from ). The new inherits this behavior but has no test for it. Given the dual-path code (), this seems worth exercising.
Minor Inconsistency in /
In the stubs, the error message example says:
This suggests returns something that supports , which it doesn't — the assertion result type is , not the collection assertion chain. The message for has the same issue in the existing code, so this is pre-existing, but worth noting.
Summary
The PR is solid and follows the established conventions faithfully. The two main suggestions are:
- Consider adding tests for null value and exception-chaining scenarios
- The duplication is worth tracking as future tech debt, ideally fixed alongside and the other type assertions in a single refactor
Happy to approve once the missing test cases are considered. The implementation itself is correct.
There was a problem hiding this comment.
Code Review: IsAssignableFrom<T>() and IsNotAssignableFrom<T>() Assertions
Overall this is a well-structured PR that correctly mirrors the existing IsAssignableTo pattern. The implementation is consistent and the API snapshots are all updated. A few observations worth considering:
Implementation Correctness ✅
The core logic is correct:
IsAssignableFromAssertion:actualType.IsAssignableFrom(_sourceType)— "can TSource be assigned to this value's runtime type?"IsNotAssignableFromAssertion: negation of the above
The null/exception handling pattern is identical to IsAssignableToAssertion, which is appropriate.
Architecture: ExpressionBuilder Duplication
The ExpressionBuilder.Append(...) call is copy-pasted ~18 times across 15 source files (same duplication exists for IsAssignableTo). The reason this lives in the factory method rather than the assertion constructor is presumably so the expression captures the right context timing.
However, it would be cleaner to move this into the assertion class constructor itself — the type name is available there via typeof(TSource).Name and doesn't change:
// Instead of in every source class:
public IsAssignableFromAssertion<TSource, TValue> IsAssignableFrom<TSource>()
{
Context.ExpressionBuilder.Append($".IsAssignableFrom<{typeof(TSource).Name}>()"); // ← repeated 18x
return new IsAssignableFromAssertion<TSource, TValue>(Context);
}
// Could be in the assertion constructor:
public IsAssignableFromAssertion(AssertionContext<TValue> context) : base(context)
{
_sourceType = typeof(TSource);
context.ExpressionBuilder.Append($".IsAssignableFrom<{_sourceType.Name}>()"); // ← once
}This would reduce drift risk if the string format ever needs to change. That said, IsAssignableTo has this same pattern, so changing it here alone would be inconsistent — probably better as a separate refactor across all type assertions.
Semantic Concern: Counterintuitive Direction
IsAssignableFrom is already confusing in .NET's reflection API — typeof(Animal).IsAssignableFrom(typeof(Dog)) meaning "Dog can be assigned to Animal" is notoriously backwards-feeling. The PR adds solid XML docs and the AllSixMethods_InSingleChain_NoAmbiguity test helps validate chaining, but it's worth considering whether this warrants a note in the docs/README that explicitly contrasts IsAssignableTo vs IsAssignableFrom with a side-by-side example. Design call, not a blocker.
Missing Edge Case Tests
The test suite is good but misses two scenarios that IsAssignableTo also lacks coverage for:
-
Null value behavior: When the asserted value is
null, the implementation returnsAssertionResult.Failed("value was null"). There's no test verifying this is the expected UX. -
Exception chaining scenario:
IsAssignableToAssertionexplicitly handles the case whereexception != null(from.Throws().And.IsAssignableTo<SomeException>()). The newIsAssignableFrominherits this behavior via the same dual-path code (objectToCheck = exception ?? value), but has no test exercising the exception path.
Minor Inconsistency in CountWrapper/LengthWrapper Error Messages
The NotSupported stub error messages say:
Use: Assert.That(value).IsAssignableFrom<IList<int>>().HasCount().EqualTo(5)
This implies IsAssignableFrom returns something chainable with .HasCount(), which it doesn't — IsAssignableFromAssertion doesn't expose collection methods. The existing IsNotAssignableTo stubs have the same issue, so this is pre-existing, but both could say something like "call IsAssignableFrom before HasCount()" to avoid implying a valid chain.
Summary
The PR is solid and follows the established conventions faithfully. The two main suggestions are:
- Consider adding tests for null value and exception-chaining scenarios
- The
ExpressionBuilder.Appendduplication is worth tracking as future tech debt, ideally fixed alongsideIsAssignableToand the other type assertions in a single refactor
The implementation itself is correct and the public API snapshots are properly updated across all four TFMs.
Covers the two untested code paths in IsAssignableFrom/IsNotAssignableFrom: - Null value returns "value was null" failure - Exception path (via .Throws().And) checks exception type
There was a problem hiding this comment.
Follow-up Review
The second commit (9f1c048) directly addresses both missing test scenarios from my previous review:
- Null value tests:
IsAssignableFrom_Fails_WhenNullandIsNotAssignableFrom_Fails_WhenNullnow cover the null-returns-failure path. - Exception chaining tests:
IsAssignableFrom_WithException_ChecksExceptionTypeandIsNotAssignableFrom_WithException_ChecksExceptionTypeexercise theexception != nullcode path from.Throws().And.IsAssignableFrom<T>().
All my previous feedback has been addressed. The two open items I noted as not requiring changes in this PR remain unchanged as expected:
ExpressionBuilder.Appendduplication — acknowledged as pre-existing tech debt shared withIsAssignableTo; better handled in a separate refactor.- CountWrapper/LengthWrapper error message wording — pre-existing pattern, minor.
The implementation is correct, consistently follows the existing IsAssignableTo pattern across all 15 source classes, public API snapshots are updated for all four TFMs, and the test coverage is now comprehensive. This is ready to merge.
The template source was updated to net10.0 but the verified snapshot still referenced net8.0, causing CI failures.
There was a problem hiding this comment.
Follow-up Review (3rd Pass)
This review picks up from the previous two rounds. Here's where things stand:
Previous Feedback — Fully Addressed ✅
Both actionable items from the earlier reviews were addressed in commit 9f1c048:
- Null value tests:
IsAssignableFrom_Fails_WhenNullandIsNotAssignableFrom_Fails_WhenNullare present - Exception chaining tests: Both
.Throws().And.IsAssignableFrom<T>()paths are now exercised
The two deferred items (ExpressionBuilder duplication, CountWrapper/LengthWrapper error message wording) were correctly left as pre-existing tech debt.
New Commits Since Last Review
Commits ce40ae5a and 2e1a5953 are bundled in this PR but fix an unrelated CI issue — updating template snapshots from net8.0 to net10.0 because the source templates were updated. These commits are self-explanatory and the fix is correct. The snapshot diff (TUnit.Templates.Tests/Snapshots/InstantiationTest.TUnit._.verified/...) reflects this, so no concern there; just worth noting they're logically separate changes from the main feature.
Minor Observation: Docs Component Asymmetry
In docs/src/components/AssertionsLibrary/index.tsx, only IsNotAssignableFrom appears as a newly added line (+). The IsAssignableFrom entry was apparently committed in the same initial commit but doesn't show as a diff because it was squashed with the baseline. This is a non-issue functionally — both entries are present in the file — but worth flagging in case the PR's changeset is audited later.
Overall Assessment
The implementation is correct, consistently follows the existing IsAssignableTo pattern across all 15 source classes, public API snapshots are updated for all four TFMs (net8.0, net9.0, net10.0, net472), test coverage is comprehensive (null, exception path, chaining, member lambdas, mixed chains), and docs are updated. The core assertion semantics (actualType.IsAssignableFrom(typeof(TSource))) accurately mirror the .NET reflection API.
This is ready to merge.
There was a problem hiding this comment.
Follow-up Review (commits ce40ae5a and 2e1a5953)
Two new commits have landed since the previous review marked the PR ready to merge.
Template Framework Updates
Both new commits update template TargetFramework from net8.0 to net10.0 across:
TUnit.Templates/content/TUnit.*/(source templates)TUnit.Templates.Tests/Snapshots/(snapshot verification files)
Languages covered: C#, VB, F#, and Playwright variants.
Concern: These changes are unrelated to the IsAssignableFrom feature in the PR title/description. Bundling template version bumps into a feature PR makes the git history harder to bisect if a template regression is reported later. It would be cleaner as a separate PR — however, since they're mechanical 1-for-1 net8.0 → net10.0 replacements with matching snapshot updates, this is a minor process concern rather than a correctness issue.
Snapshot consistency: The diff correctly updates both the template source files and the corresponding .verified.txt snapshots in lockstep, which follows the project's snapshot convention.
Summary
No new functional issues introduced. All previously raised concerns remain addressed. The template framework bumps are correct in isolation; the only note is that they'd be better separated by concern. This is still ready to merge from a correctness standpoint.
The template generates the file with a trailing newline but the verified snapshot was missing it, causing snapshot comparison to fail.
There was a problem hiding this comment.
Follow-up Review (4th Pass — commit f6655f05)
One new commit has landed since the previous review confirmed the PR was ready to merge.
Latest Commit: f6655f05 — "fix: add trailing newline to TUnit template snapshot"
File changed: TUnit.Templates.Tests/Snapshots/InstantiationTest.TUnit._.verified/TUnit/TUnit.csproj (+2/-2)
This is a trivial whitespace-only fix — adding a missing trailing newline to a snapshot verification file. It's the kind of fix that prevents snapshot test failures caused by editors or tools that enforce final newlines. No functional impact.
Cumulative Status
All feedback from previous review rounds remains addressed:
- Null value tests: Present ✅ (
IsAssignableFrom_Fails_WhenNull,IsNotAssignableFrom_Fails_WhenNull) - Exception chaining tests: Present ✅ (
IsAssignableFrom_WithException_ChecksExceptionType,IsNotAssignableFrom_WithException_ChecksExceptionType) - ExpressionBuilder duplication: Acknowledged as pre-existing tech debt, deferred appropriately
- CountWrapper/LengthWrapper error message wording: Pre-existing, minor
- Template net8.0 → net10.0 bumps: Mechanically correct, logically separate concern (already noted)
Assessment
No new issues introduced. The IsAssignableFrom<T>() and IsNotAssignableFrom<T>() feature is complete, correct, well-tested, and consistent with the existing IsAssignableTo pattern across all 15 source classes and 4 TFMs. This is ready to merge.
The TUnit template was restructured with new files (BasicTests.cs, Calculator.cs, DataDrivenTests.cs, DependencyInjectionTests.cs, HooksAndLifecycle.cs, Data/AdditionDataGenerator.cs, Data/InMemoryDb.cs) replacing the old files (Tests.cs, Tests2.cs, Tests3.cs, GlobalSetup.cs, Data/DataClass.cs, Data/DataSourceGenerator.cs, Data/DependencyInjectionClassConstructor.cs).
There was a problem hiding this comment.
Follow-up Review (5th Pass — commit 66d42b1f)
One new commit has landed since the previous review confirmed the PR was ready to merge.
Latest Commit: 66d42b1f — "fix: update TUnit template snapshot to match restructured template"
This is a sizeable restructure of the C# TUnit project template snapshot:
Removed (old template files):
Tests.cs,Tests2.cs,Tests3.csGlobalSetup.csData/DataClass.cs,Data/DataSourceGenerator.cs,Data/DependencyInjectionClassConstructor.cs
Added (new template files):
BasicTests.cs,Calculator.csDataDrivenTests.cs,Data/AdditionDataGenerator.csDependencyInjectionTests.cs,Data/InMemoryDb.csHooksAndLifecycle.cs
The restructure makes the template considerably more comprehensive — splitting tests by concern (basic, data-driven, DI, lifecycle) and adding a real InMemoryDb example. The snapshot correctly reflects the new template structure.
Concern (recurring): This is now the fourth unrelated change bundled into a feature PR for IsAssignableFrom. The template restructure is a meaningful improvement but has no connection to the assertion feature. Bisecting regressions in either the template or the assertion becomes harder as a result. Not a blocker, but the pattern is worth calling out.
Cumulative Status
All feedback from previous review rounds remains addressed:
- Null value tests: Present ✅
- Exception chaining tests: Present ✅
- ExpressionBuilder duplication: Acknowledged pre-existing tech debt ✅
- CountWrapper/LengthWrapper error message wording: Pre-existing, minor ✅
- Template net8.0 → net10.0 bumps: Mechanically correct ✅
- Trailing newline fix: Trivial ✅
- Template restructure: Correct snapshot update ✅
The core IsAssignableFrom<T>() and IsNotAssignableFrom<T>() feature remains complete, correct, and well-tested. Ready to merge.
Summary
Closes #4824.
IsAssignableFrom<T>()andIsNotAssignableFrom<T>()as the counterpart to the existingIsAssignableTo<T>()/IsNotAssignableTo<T>()IsAssignableTo<T>()checks: "can this value be assigned to a variable of type T?" (typeof(T).IsAssignableFrom(actualType))IsAssignableFrom<T>()checks: "can a value of type T be assigned to a variable of this value's type?" (actualType.IsAssignableFrom(typeof(T)))Example
Test plan
TypeAssertionAmbiguityTestscovering pass/fail cases, chaining, and member lambdas