Skip to content

Fix Type assignability assertions to evaluate represented type (not RuntimeType) - #6711

Merged
thomhurst merged 8 commits into
mainfrom
copilot/fix-type-checking-issue
Sep 3, 2026
Merged

Fix Type assignability assertions to evaluate represented type (not RuntimeType)#6711
thomhurst merged 8 commits into
mainfrom
copilot/fix-type-checking-issue

Conversation

Copilot AI commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Assert.That(typeof(Dog)).IsAssignableTo<Animal>() was evaluating the runtime type of the Type instance (RuntimeType) rather than the represented type (Dog), causing valid assignability checks to fail. This PR aligns Type assignability behavior with documented usage.

  • Assertion semantics fix

    • Updated assignability assertions to treat asserted Type values as the target of assignability checks.
    • This affects both IsAssignableTo<T>() and IsAssignableFrom<T>() paths when the asserted value is a Type.
  • Type-specific API surface (generator-backed)

    • Added Type-targeted overloads using assertion generation:
      • IsAssignableTo(this Type value, Type expectedType)
      • IsAssignableFrom(this Type value, Type sourceType)
    • Keeps the Type API explicit and consistent with other generated assertions.
  • Regression coverage

    • Added focused tests for generic and runtime-Type assignability scenarios to ensure documented typeof(...) usage remains valid.
public class Animal { }
public class Dog : Animal { }

await Assert.That(typeof(Dog)).IsAssignableTo<Animal>();
await Assert.That(typeof(Animal)).IsAssignableFrom<Dog>();

await Assert.That(typeof(Dog)).IsAssignableTo(typeof(Animal));
await Assert.That(typeof(Animal)).IsAssignableFrom(typeof(Dog));

Copilot AI linked an issue Sep 3, 2026 that may be closed by this pull request
1 task
Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix type checking issue in assertions Fix Type assignability assertions to evaluate represented type (not RuntimeType) Sep 3, 2026
Copilot AI requested a review from thomhurst September 3, 2026 09:22
@thomhurst
thomhurst marked this pull request as ready for review September 3, 2026 09:28
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-03T12:48:36.145491Z c8eebf7 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 38b97aab7a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/TUnit.Assertions/Conditions/TypeAssertionExtensions.cs
Comment thread src/TUnit.Assertions/Conditions/TypeOfAssertion.cs Outdated
@greptile-apps

greptile-apps Bot commented Sep 3, 2026

Copy link
Copy Markdown

Greptile Summary

The PR makes direct Assert.That(Type) assignability assertions evaluate the represented type rather than the reflection object's runtime implementation type.

  • Adds a specialized TypeValueAssertion source and prioritizes the corresponding Assert.That(Type?) overload.
  • Adds represented-type handling for generic assignability assertions and generated overloads accepting runtime Type arguments.
  • Adds regression coverage and updates public API baselines for supported target frameworks.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
src/TUnit.Assertions/Conditions/TypeAssertionExtensions.cs Adds generated runtime-Type assignability assertions with explicit null-argument failures.
src/TUnit.Assertions/Conditions/TypeOfAssertion.cs Adds represented-type semantics to the relevant generic assignability assertion implementations.
src/TUnit.Assertions/Extensions/Assert.cs Introduces and prioritizes the specialized assertion entry point for nullable Type values.
src/TUnit.Assertions/Sources/TypeValueAssertion.cs Defines the direct Type assertion source and its represented-type generic assignability methods.
tests/TUnit.Assertions.Tests/TypeAssertionTests.cs Covers direct generic, runtime-Type, TypeInfo, null-argument, chaining, and generic-source behavior.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A["Assert.That(typeof(Dog))"] --> B["TypeValueAssertion"]
  B --> C{"Assignability overload"}
  C -->|"Generic target"| D["typeof(Target).IsAssignableFrom(typeof(Dog))"]
  C -->|"Runtime Type target"| E["expectedType.IsAssignableFrom(typeof(Dog))"]
  D --> F["Assertion result"]
  E --> F
Loading

Reviews (7): Last reviewed commit: "fix(assertions): prioritize Type overloa..." | Re-trigger Greptile

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2ae67b7490

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/TUnit.Assertions/Conditions/TypeOfAssertion.cs Outdated
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Code Review

I reviewed this PR by tracing the fix's design and empirically verifying findings by building the branch and running targeted repro tests. The core goal — making Assert.That(typeof(Dog)).IsAssignableTo<Animal>() evaluate the represented type instead of the RuntimeType — is a real and sensible fix. However, the implementation has gaps and one broader behavioral risk worth addressing before merge.

1. Fix only covers the direct entry point, not indirect chains (confirmed by test)

src/TUnit.Assertions/Sources/TypeValueAssertion.cs:20

IsAssignableTo<TTarget>() is hidden with new rather than overridden, so the corrected behavior only applies when the compile-time (static) type of the expression is exactly TypeValueAssertion. Any indirect path — e.g. via .And/.Or continuations (AndContinuation<Type> / OrContinuation<Type>, which are ValueAssertion<Type>-typed, not TypeValueAssertion) — still resolves to the original IsAssignableToAssertion<TTarget, Type>, whose Map casts the Type instance to TTarget and always fails.

Verified directly:

Animal? result = await Assert.That(typeof(Dog)).IsClass().And.IsAssignableTo<Animal>();
// assertion passes, but result == null (confirmed via test run: RESULT_IS_NULL=True)

Why this matters: new hiding is a classic C# footgun — it silently reintroduces the bug for any caller who chains through .And/.Or or otherwise accesses the value through a base-typed reference, which is a very common assertion pattern in this codebase. A virtual/override design (or fixing the shared base Map logic itself, as the PR does elsewhere in TypeOfAssertion.cs) would close this gap for all access paths rather than just the one the PR's own tests exercise.

2. Missing null-check on new Type-targeted overloads (confirmed by test)

src/TUnit.Assertions/Conditions/TypeAssertionExtensions.cs:88

The new [GenerateAssertion]-backed IsAssignableTo(this Type value, Type expectedType) / IsAssignableFrom(this Type value, Type sourceType) don't null-check the extra Type parameter before dereferencing it.

Verified directly:

await Assert.That(typeof(Dog)).IsAssignableTo(null!);
// throws raw System.NullReferenceException instead of a graceful assertion failure (EXCEPTION_TYPE=System.NullReferenceException)

Why this matters: every other TUnit assertion fails gracefully with a clear assertion message on invalid input; a raw NRE breaks that contract and gives users a confusing stack trace instead of an assertion failure message pointing at the actual mistake.

3. Shared base-class fix may widen behavior beyond Type-typed sources

src/TUnit.Assertions/Conditions/TypeOfAssertion.cs:137

The objectToCheck is Type typeToCheck ? typeToCheck : objectToCheck.GetType() change was applied to the shared generic IsAssignableToAssertion/IsNotAssignableToAssertion/IsAssignableFromAssertion/IsNotAssignableFromAssertion classes, which are used by every assertion source (PropertyAssertion, MemberAssertion, ListAssertions, DelegateAssertion, etc.), not just Type-typed ones.

This changes assignability semantics for any object/property/member value that happens to hold a Type instance at runtime:

object obj = typeof(Dog);
await Assert.That(obj).IsAssignableTo<Type>();
// previously passed trivially (obj.GetType() is a RuntimeType, always assignable to Type)
// now fails, since the represented type (Dog) is not assignable to Type

Why this matters: this is a behavior change outside the PR's stated scope ("Type assignability behavior" when the asserted value itself is a Type). It's plausible this broader semantic is actually desired/more correct, but it should be a deliberate, called-out decision with its own test coverage — not an incidental side effect of sharing the base class. Consider either scoping the fix to the new TypeValueAssertion source only, or explicitly documenting/testing this as an intentional semantic change for all assertion sources.

Suggested path forward

  • Make TypeValueAssertion's corrected IsAssignableTo<T>()/IsAssignableFrom<T>() reachable through .And/.Or continuations (e.g. by fixing the shared Map used by the base assertion rather than shadowing it on one subclass), and add a regression test that chains through .And.
  • Add null-checks (with a proper assertion failure, not an NRE) to the new Type-parameter overloads.
  • Either scope the TypeOfAssertion.cs base-class fix to Type-typed sources only, or add explicit test coverage + changelog note for the widened behavior on non-Type sources holding Type values.

Nice catch on the underlying bug and good instinct to add generator-backed overloads for the explicit Type-to-Type case — the remaining issues are about closing gaps in how completely the fix is applied, not about the direction of the change.

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Code review (follow-up)

I checked the current head (8e58dc6d9) against the three gaps raised in the prior review. The final commit ("fix(assertions): keep generic type semantics") partially reacted to that feedback, but it doesn't resolve the gaps — and it introduces a new, verifiable inconsistency.

1. Gap 2 (missing null-check) — still unaddressed

{
[GenerateAssertion(ExpectationMessage = "be assignable to {expectedType}", InlineMethodBody = true)]
public static AssertionResult IsAssignableTo(this Type value, Type expectedType)
=> expectedType.IsAssignableFrom(value)
? AssertionResult.Passed
: AssertionResult.Failed($"type {value.Name} is not assignable to {expectedType.Name}");
[GenerateAssertion(ExpectationMessage = "be assignable from {sourceType}", InlineMethodBody = true)]
public static AssertionResult IsAssignableFrom(this Type value, Type sourceType)
=> value.IsAssignableFrom(sourceType)
? AssertionResult.Passed
: AssertionResult.Failed($"type {value.Name} is not assignable from {sourceType.Name}");
}

IsAssignableTo(this Type value, Type expectedType) / IsAssignableFrom(this Type value, Type sourceType) are unchanged from the prior review. expectedType.IsAssignableFrom(value) / value.IsAssignableFrom(sourceType) still dereference expectedType/sourceType without a null check, so Assert.That(typeof(Dog)).IsAssignableTo(null!) still throws a raw NullReferenceException instead of a graceful assertion failure.

2. Gap 1 (indirect .And/.Or chains) — still unaddressed, and now behaves differently

protected override async Task<AssertionResult> CheckAsync(EvaluationMetadata<TTarget> _)
{
var (value, exception) = await _sourceContext.GetAsync();
object? objectToCheck;
// If we have an exception (from Throws/ThrowsExactly), check that
if (exception != null)
{
objectToCheck = exception;
}
// Otherwise check the value
else if (value != null)
{
objectToCheck = value;
}
else
{
return AssertionResult.Failed("value was null");
}
var actualType = objectToCheck.GetType();
if (_targetType.IsAssignableFrom(actualType))
{
return AssertionResult.Passed;
}
return AssertionResult.Failed($"type {actualType.Name} is not assignable to {_targetType.Name}");
}

The last commit reverted IsAssignableToAssertion<TTarget, TValue>.CheckAsync (the class used for .And/.Or continuations, e.g. Assert.That(typeof(Dog)).IsClass().And.IsAssignableTo<Animal>()) back to objectToCheck.GetType(). This is the exact repro from the prior review, and it's back to the pre-widening behavior for that call, so chained calls through a base-typed reference are still not fixed — they just fail in a different way now (the assignability check itself fails, using RuntimeType, rather than passing-but-returning-null as before).

3. New bug: IsAssignableTo vs IsNotAssignableTo/IsAssignableFrom/IsNotAssignableFrom are now inconsistent

}
var actualType = objectToCheck is Type typeToCheck ? typeToCheck : objectToCheck.GetType();

Only IsAssignableToAssertion (L137) was reverted to plain objectToCheck.GetType(). IsNotAssignableToAssertion (L221), IsAssignableFromAssertion (L270), and IsNotAssignableFromAssertion (L318) still use objectToCheck is Type typeToCheck ? typeToCheck : objectToCheck.GetType(). For object obj = typeof(Dog):

  • Assert.That(obj).IsAssignableTo<Type>() → uses reverted logic → actualType = obj.GetType() (a RuntimeType) → Type.IsAssignableFrom(RuntimeType) is true → passes.
  • Assert.That(obj).IsNotAssignableTo<Type>() → uses widened logic → actualType = Dog (the represented type) → Type.IsAssignableFrom(Dog) is false → !falsealso passes.

Both an assertion and its logical negation pass for the same input — that's a contradiction, and a regression introduced by this commit (previously both classes were at least internally consistent with each other, even if the exact behavior was up for debate). This split should be reconciled: either all four classes get the same Type-aware treatment or none do.

None of these are things a linter would catch, and all are derivable directly from reading the diff. Happy to take another pass once these are addressed.

This was referenced Sep 7, 2026
github-actions Bot pushed a commit to BenjaminMichaelis/TrxLib that referenced this pull request Sep 7, 2026
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.65.68 to
1.66.16.

<details>
<summary>Release notes</summary>

_Sourced from [TUnit's
releases](https://github.com/thomhurst/TUnit/releases)._

## 1.66.16

<!-- Release notes generated using configuration in .github/release.yml
at v1.66.16 -->

## What's Changed
### Other Changes
* fix: isolated name is lowercase (#​6727) by @​koryphaee in
thomhurst/TUnit#6728
* fix: preserve concurrent Assert.Multiple failures by @​thomhurst in
thomhurst/TUnit#6730
* fix: preserve original HTTP mock request content by @​thomhurst in
thomhurst/TUnit#6731
### Dependencies
* chore(deps): update tunit to 1.66.10 by @​thomhurst in
thomhurst/TUnit#6726
* chore(deps): update dependency dompurify to v3.4.15 by @​thomhurst in
thomhurst/TUnit#6732


**Full Changelog**:
thomhurst/TUnit@v1.66.10...v1.66.16

## 1.66.10

<!-- Release notes generated using configuration in .github/release.yml
at v1.66.10 -->

## What's Changed
### Other Changes
* fix: restore null suppression for built-in assertion methods by
@​thomhurst in thomhurst/TUnit#6725
### Dependencies
* chore(deps): update tunit to 1.66.8 by @​thomhurst in
thomhurst/TUnit#6724


**Full Changelog**:
thomhurst/TUnit@v1.66.8...v1.66.10

## 1.66.8

<!-- Release notes generated using configuration in .github/release.yml
at v1.66.8 -->

## What's Changed
### Other Changes
* fix(ci): make issue triage work for external reporters by @​thomhurst
in thomhurst/TUnit#6720
* fix(ci): run code review on pull requests from forks by @​thomhurst in
thomhurst/TUnit#6722
* fix: suppress nullability warnings after Should NotBeNull assertions
by @​mvanhorn in thomhurst/TUnit#6700
* fix: Avoid HTML report CLI option clashes by @​mvanhorn in
thomhurst/TUnit#6677
### Dependencies
* chore(deps): update tunit to 1.66.0 by @​thomhurst in
thomhurst/TUnit#6719
* chore(deps): update dependency microsoft.kiota.abstractions to 2.1.1
by @​thomhurst in thomhurst/TUnit#6721
* chore(deps): update dependency awssdk.sqs to 4.0.100.12 by @​thomhurst
in thomhurst/TUnit#6723


**Full Changelog**:
thomhurst/TUnit@v1.66.0...v1.66.8

## 1.66.0

<!-- Release notes generated using configuration in .github/release.yml
at v1.66.0 -->

## What's Changed
### Other Changes
* Compile all C# documentation snippets by @​thomhurst in
thomhurst/TUnit#6695
* Fix `Type` assignability assertions to evaluate represented type (not
`RuntimeType`) by @​thomhurst with @​Copilot in
thomhurst/TUnit#6711
* Clarify ClassDataSource constructor requirements by @​thomhurst in
thomhurst/TUnit#6716
* Add programmatic HTML reporting settings by @​thomhurst in
thomhurst/TUnit#6699
* Fix timeout cancellation diagnostics by @​thomhurst in
thomhurst/TUnit#6715
### Dependencies
* chore(deps): update tunit to 1.65.68 by @​thomhurst in
thomhurst/TUnit#6682
* chore(deps): update dependency verify.tool to v0.9.1 by @​thomhurst in
thomhurst/TUnit#6683
* chore(deps): update dependency mockolate to 3.4.1 by @​thomhurst in
thomhurst/TUnit#6685
* chore(deps): update dependency serialize-javascript to v7.1.1 by
@​thomhurst in thomhurst/TUnit#6687
* chore(deps): update dependency qs to v6.16.0 by @​thomhurst in
thomhurst/TUnit#6691
* chore(deps): update dependency system.reactive to v7 by @​thomhurst in
thomhurst/TUnit#6696
* chore(deps): update dependency imposter to 0.1.10 by @​thomhurst in
thomhurst/TUnit#6701
* chore(deps): update dependency microsoft.kiota.abstractions to 2.1.0
by @​thomhurst in thomhurst/TUnit#6704
* chore(deps): update mstest to 4.4.0 by @​thomhurst in
thomhurst/TUnit#6705
* chore(deps): update dependency dotnet-trace to v10 by @​thomhurst in
thomhurst/TUnit#6706
* chore(deps): update microsoft.testing by @​thomhurst in
thomhurst/TUnit#6703
* chore(deps): update microsoft.testing by @​thomhurst in
thomhurst/TUnit#6713
* chore(deps): bump fast-uri from 3.1.5 to 3.1.7 in /docs by
@​dependabot[bot] in thomhurst/TUnit#6707


**Full Changelog**:
thomhurst/TUnit@v1.65.68...v1.66.0

Commits viewable in [compare
view](thomhurst/TUnit@v1.65.68...v1.66.16).
</details>

[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=TUnit&package-manager=nuget&previous-version=1.65.68&new-version=1.66.16)](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>
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.

[Bug]: Type checking doesn't work as described in docs

2 participants