-
-
Notifications
You must be signed in to change notification settings - Fork 108
Fix false positive nullability warnings in Assert.That() for collection types #4248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes false positive nullability warnings (CS8604) that occur when passing nullable collection types to Assert.That(). The issue was that several Assert.That() overloads for collection types didn't accept nullable parameters, causing the C# compiler to emit warnings when users tried to assert on nullable collections like List<string>?, IList<T>?, Dictionary<TKey, TValue>?, etc.
Key Changes
- Updated
Assert.That()overloads to accept nullable collection parameters for all major collection types - Modified assertion class constructors to accept nullable values while maintaining type safety through null-forgiving operators
- Added comprehensive test coverage to validate the fix works correctly for all supported collection types
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| TUnit.Assertions/Extensions/Assert.cs | Updated 8 Assert.That() overloads to accept nullable parameters for IList, IReadOnlyList, arrays, IDictionary, IReadOnlyDictionary, ISet, IReadOnlySet, and HashSet |
| TUnit.Assertions/Sources/ListAssertion.cs | Modified constructor to accept IList<TItem>? and use null-forgiving operator when creating AssertionContext |
| TUnit.Assertions/Sources/DictionaryAssertion.cs | Modified constructor to accept IReadOnlyDictionary<TKey, TValue>? and use null-forgiving operator when creating AssertionContext |
| TUnit.Assertions/Sources/MutableDictionaryAssertion.cs | Modified constructor to accept IDictionary<TKey, TValue>? and use null-forgiving operator when creating AssertionContext |
| TUnit.Assertions/Sources/SetAssertion.cs | Modified constructors for SetAssertion, ReadOnlySetAssertion, and HashSetAssertion to accept nullable parameters and use null-forgiving operator |
| TUnit.Assertions.Tests/CollectionNullabilityWarningTests.cs | Added comprehensive test suite covering nullable collection scenarios for List, IList, Dictionary, IDictionary, IReadOnlyDictionary, arrays, ISet, HashSet, and IReadOnlyList |
| TUnit.PublicAPI/*.verified.txt | Updated public API snapshots for .NET 8.0, 9.0, and 10.0 to reflect the parameter nullability changes |
| namespace TUnit.Assertions.Tests; | ||
|
|
||
| /// <summary> | ||
| /// Tests to ensure that Assert.That() accepts nullable collection types without generating | ||
| /// nullability warnings (CS8604, CS8625, etc.). This validates the fix for GitHub issue reporting | ||
| /// false positive nullability warnings for List, IList, Dictionary, IDictionary types. | ||
| /// </summary> | ||
| public class CollectionNullabilityWarningTests | ||
| { | ||
| // =================================== | ||
| // List<T>? Tests | ||
| // =================================== | ||
|
|
||
| [Test] | ||
| public async Task NullableList_AcceptsNullableValue_NoWarning() | ||
| { | ||
| List<string>? list = ["a", "b", "c"]; | ||
| await Assert.That(list).IsNotNull(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableList_WithNullValue_IsNotNull_Fails() | ||
| { | ||
| List<string>? list = null; | ||
| var action = async () => await Assert.That(list).IsNotNull(); | ||
| await Assert.That(action).Throws<AssertionException>(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableList_WithNullValue_IsNull_Passes() | ||
| { | ||
| List<string>? list = null; | ||
| await Assert.That(list).IsNull(); | ||
| } | ||
|
|
||
| // =================================== | ||
| // IList<T>? Tests | ||
| // =================================== | ||
|
|
||
| [Test] | ||
| public async Task NullableIList_AcceptsNullableValue_NoWarning() | ||
| { | ||
| IList<string>? list = new List<string> { "a", "b", "c" }; | ||
| await Assert.That(list).IsNotNull(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableIList_WithNullValue_IsNotNull_Fails() | ||
| { | ||
| IList<string>? list = null; | ||
| var action = async () => await Assert.That(list).IsNotNull(); | ||
| await Assert.That(action).Throws<AssertionException>(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableIList_WithNullValue_IsNull_Passes() | ||
| { | ||
| IList<string>? list = null; | ||
| await Assert.That(list).IsNull(); | ||
| } | ||
|
|
||
| // =================================== | ||
| // Dictionary<TKey, TValue>? Tests | ||
| // =================================== | ||
|
|
||
| [Test] | ||
| public async Task NullableDictionary_AcceptsNullableValue_NoWarning() | ||
| { | ||
| Dictionary<string, int>? dict = new() { ["key"] = 1 }; | ||
| await Assert.That(dict).IsNotNull(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableDictionary_WithNullValue_IsNotNull_Fails() | ||
| { | ||
| Dictionary<string, int>? dict = null; | ||
| var action = async () => await Assert.That(dict).IsNotNull(); | ||
| await Assert.That(action).Throws<AssertionException>(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableDictionary_WithNullValue_IsNull_Passes() | ||
| { | ||
| Dictionary<string, int>? dict = null; | ||
| await Assert.That(dict).IsNull(); | ||
| } | ||
|
|
||
| // =================================== | ||
| // IDictionary<TKey, TValue>? Tests | ||
| // =================================== | ||
|
|
||
| [Test] | ||
| public async Task NullableIDictionary_AcceptsNullableValue_NoWarning() | ||
| { | ||
| IDictionary<string, int>? dict = new Dictionary<string, int> { ["key"] = 1 }; | ||
| await Assert.That(dict).IsNotNull(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableIDictionary_WithNullValue_IsNotNull_Fails() | ||
| { | ||
| IDictionary<string, int>? dict = null; | ||
| var action = async () => await Assert.That(dict).IsNotNull(); | ||
| await Assert.That(action).Throws<AssertionException>(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableIDictionary_WithNullValue_IsNull_Passes() | ||
| { | ||
| IDictionary<string, int>? dict = null; | ||
| await Assert.That(dict).IsNull(); | ||
| } | ||
|
|
||
| // =================================== | ||
| // IReadOnlyDictionary<TKey, TValue>? Tests | ||
| // =================================== | ||
|
|
||
| [Test] | ||
| public async Task NullableIReadOnlyDictionary_AcceptsNullableValue_NoWarning() | ||
| { | ||
| IReadOnlyDictionary<string, int>? dict = new Dictionary<string, int> { ["key"] = 1 }; | ||
| await Assert.That(dict).IsNotNull(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableIReadOnlyDictionary_WithNullValue_IsNotNull_Fails() | ||
| { | ||
| IReadOnlyDictionary<string, int>? dict = null; | ||
| var action = async () => await Assert.That(dict).IsNotNull(); | ||
| await Assert.That(action).Throws<AssertionException>(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableIReadOnlyDictionary_WithNullValue_IsNull_Passes() | ||
| { | ||
| IReadOnlyDictionary<string, int>? dict = null; | ||
| await Assert.That(dict).IsNull(); | ||
| } | ||
|
|
||
| // =================================== | ||
| // Array? Tests | ||
| // =================================== | ||
|
|
||
| [Test] | ||
| public async Task NullableArray_AcceptsNullableValue_NoWarning() | ||
| { | ||
| string[]? arr = ["a", "b", "c"]; | ||
| await Assert.That(arr).IsNotNull(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableArray_WithNullValue_IsNotNull_Fails() | ||
| { | ||
| string[]? arr = null; | ||
| var action = async () => await Assert.That(arr).IsNotNull(); | ||
| await Assert.That(action).Throws<AssertionException>(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableArray_WithNullValue_IsNull_Passes() | ||
| { | ||
| string[]? arr = null; | ||
| await Assert.That(arr).IsNull(); | ||
| } | ||
|
|
||
| // =================================== | ||
| // ISet<T>? Tests | ||
| // =================================== | ||
|
|
||
| [Test] | ||
| public async Task NullableISet_AcceptsNullableValue_NoWarning() | ||
| { | ||
| ISet<string>? set = new HashSet<string> { "a", "b", "c" }; | ||
| await Assert.That(set).IsNotNull(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableISet_WithNullValue_IsNotNull_Fails() | ||
| { | ||
| ISet<string>? set = null; | ||
| var action = async () => await Assert.That(set).IsNotNull(); | ||
| await Assert.That(action).Throws<AssertionException>(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableISet_WithNullValue_IsNull_Passes() | ||
| { | ||
| ISet<string>? set = null; | ||
| await Assert.That(set).IsNull(); | ||
| } | ||
|
|
||
| // =================================== | ||
| // HashSet<T>? Tests | ||
| // =================================== | ||
|
|
||
| [Test] | ||
| public async Task NullableHashSet_AcceptsNullableValue_NoWarning() | ||
| { | ||
| HashSet<string>? set = ["a", "b", "c"]; | ||
| await Assert.That(set).IsNotNull(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableHashSet_WithNullValue_IsNotNull_Fails() | ||
| { | ||
| HashSet<string>? set = null; | ||
| var action = async () => await Assert.That(set).IsNotNull(); | ||
| await Assert.That(action).Throws<AssertionException>(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableHashSet_WithNullValue_IsNull_Passes() | ||
| { | ||
| HashSet<string>? set = null; | ||
| await Assert.That(set).IsNull(); | ||
| } | ||
|
|
||
| // =================================== | ||
| // IReadOnlyList<T>? Tests | ||
| // =================================== | ||
|
|
||
| [Test] | ||
| public async Task NullableIReadOnlyList_AcceptsNullableValue_NoWarning() | ||
| { | ||
| IReadOnlyList<string>? list = new List<string> { "a", "b", "c" }; | ||
| await Assert.That(list).IsNotNull(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableIReadOnlyList_WithNullValue_IsNotNull_Fails() | ||
| { | ||
| IReadOnlyList<string>? list = null; | ||
| var action = async () => await Assert.That(list).IsNotNull(); | ||
| await Assert.That(action).Throws<AssertionException>(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task NullableIReadOnlyList_WithNullValue_IsNull_Passes() | ||
| { | ||
| IReadOnlyList<string>? list = null; | ||
| await Assert.That(list).IsNull(); | ||
| } | ||
|
|
||
| // =================================== | ||
| // Method parameter tests (original issue scenario) | ||
| // =================================== | ||
|
|
||
| [Test] | ||
| public async Task MethodWithNullableListParameter_NoWarning() | ||
| { | ||
| List<string>? list = ["test"]; | ||
| await VerifyNotNull(list); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task MethodWithNullableIDictionaryParameter_NoWarning() | ||
| { | ||
| IDictionary<string, int>? dict = new Dictionary<string, int> { ["key"] = 1 }; | ||
| await VerifyDictionaryNotNull(dict); | ||
| } | ||
|
|
||
| private static async Task VerifyNotNull(List<string>? actual) | ||
| { | ||
| await Assert.That(actual).IsNotNull(); | ||
| } | ||
|
|
||
| private static async Task VerifyDictionaryNotNull(IDictionary<string, int>? actual) | ||
| { | ||
| await Assert.That(actual).IsNotNull(); | ||
| } | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test coverage is comprehensive for most collection types, but it's missing tests for IReadOnlySet<T>?. Since Assert.That() has an overload for IReadOnlySet<TItem>? (line 111 in Assert.cs), tests should be added to verify nullable IReadOnlySet values work without warnings, similar to the existing tests for ISet, HashSet, IReadOnlyList, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 3 tests for IReadOnlySet<T>? in commit 134ed7a. Tests are wrapped with #if NET5_0_OR_GREATER since IReadOnlySet<T> is only available in .NET 5.0+.
Head branch was pushed to by a user without write access
- Update Assert.That() overloads to accept nullable parameters: - IList<TItem>? - IDictionary<TKey, TValue>? - IReadOnlyDictionary<TKey, TValue>? - ISet<TItem>? - HashSet<TItem>? - IReadOnlySet<TItem>? (NET5+) - IReadOnlyList<TItem>? - TItem[]? - Update corresponding assertion class constructors to accept nullable values - Add CollectionNullabilityWarningTests to verify the fix - Update public API snapshots Fixes false positive nullability warning when calling Assert.That(list).IsNotNull() with nullable List<T>?, IList<T>?, Dictionary<TKey,TValue>?, IDictionary<TKey,TValue>?, etc. Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
Added 3 tests for IReadOnlySet<T>? to complete the test coverage for all nullable collection types supported by Assert.That(). Tests are conditionally compiled for .NET 5.0+ where IReadOnlySet<T> is available. Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
134ed7a to
7d1387f
Compare
SummaryFixes false positive CS8604 nullability warnings when using Critical IssuesNone found ✅ AnalysisCorrectly follows established pattern:
This is safe because the TUnit rules compliance: Test coverage: Code quality: Verdict✅ APPROVE - This is a clean fix that correctly addresses the reported issue by following the established pattern already used for |
Summary
This PR fixes false positive CS8604 nullability warnings when using
Assert.That()with nullable collection types likeList<string>?,IList<T>?,Dictionary<TKey, TValue>?,IDictionary<TKey, TValue>?, etc.The fix was modeled after the existing
IEnumerable<TItem>?overload which already correctly accepts nullable values.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.