feat: Respect UsingPropertiesComparer#883
Conversation
As part of this bumping the tests to NUnit version `4.4.0-alpha.0.22`.
mikkelbu
force-pushed
the
feat/respect-UsingPropertiesComparer
branch
from
aa9269d to
65735a8
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR updates the NUnit analyzers to support the new UsingPropertiesComparer behavior introduced for NUnit v4 and updates the tests and constants accordingly. Key changes include:
- Updating constraint checks in SomeItemsIncompatibleTypesAnalyzer to support both the legacy and generic SomeItemsConstraint.
- Enhancing EqualToIncompatibleTypesAnalyzer with a new helper method to properly detect UsingPropertiesComparer invocations with arguments.
- Updating tests and project settings to reflect the NUnit v4.4.0-alpha.0.22 package upgrade.
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 |
|---|---|
| src/nunit.analyzers/SomeItemsIncompatibleTypes/SomeItemsIncompatibleTypesAnalyzer.cs | Updated condition to check for both legacy and generic some-items constraint types. |
| src/nunit.analyzers/EqualToIncompatibleTypes/EqualToIncompatibleTypesAnalyzer.cs | Added support for detecting UsingPropertiesComparer invocations with arguments. |
| src/nunit.analyzers/Constants/NUnitV4FrameworkConstants.cs | Introduced new constants for UsingPropertiesComparer and generic constraint representation. |
| src/nunit.analyzers.tests/EqualToIncompatibleTypes/EqualToIncompatibleTypesAnalyzerTests.cs | Updated tests to expect diagnostics and validate new analyzer behavior. |
| src/nunit.analyzers.tests/nunit.analyzers.tests.csproj | Bumped NUnit package to the new alpha version. |
| build.cake | Removed explicit package source configuration. |
| NuGet.Config | Added new configuration for NuGet package sources. |
| if ((!IsDoesContain(constraintPart) && !IsContainsItem(constraintPart)) | ||
| || constraintPart.Root?.Type?.GetFullMetadataName() != NUnitFrameworkConstants.FullNameOfSomeItemsConstraint) | ||
| || (constraintPart.Root?.Type?.GetFullMetadataName() != NUnitFrameworkConstants.FullNameOfSomeItemsConstraint | ||
| && constraintPart.Root?.Type?.GetFullMetadataName() != NUnitV4FrameworkConstants.FullNameOfSomeItemsConstraintGeneric)) |
There was a problem hiding this comment.
Consider caching the result of constraintPart.Root?.Type?.GetFullMetadataName() in a local variable to avoid multiple method calls and improve readability.
There was a problem hiding this comment.
I agree with Copilot or alternatively use patterns:
|| (constraintPart.Root?.Type?.GetFullMetadataName() is not NUnitFrameworkConstants.FullNameOfSomeItemsConstraint
and not NUnitV4FrameworkConstants.FullNameOfSomeItemsConstraintGeneric))Was this to make existing tests pass?
I don't see a new nunit test that uses UsingPropertiesComparer nor code treating this as matching types
There was a problem hiding this comment.
I'll use the pattern you suggested
Was this to make existing tests pass?
Yes - I guess due to nunit/nunit#4925 - we had the following test failures in SomeItemsIncompatibleTypesAnalyzerTests for both Does.Contain and Contains.Item
- AnalyzeWhenActualIsCollectionTask
- AnalyzeWhenInvalidCollectionActualArgumentProvided
- AnalyzeWhenNonCollectionActualArgumentProvided
- AnalyzeWhenTypedCustomComparerProvided
with something like
Error Message:
Gu.Roslyn.Asserts.AssertException : Expected and actual diagnostics do not match.
Expected:
NUnit2026 The 'Contains.Item' constraint cannot be used with actual argument of type 'string[]' and expected argument of type 'int'
Actual: <no diagnostics>
Stack Trace:
at Gu.Roslyn.Asserts.RoslynAssert.VerifyDiagnostics(DiagnosticsAndSources diagnosticsAndSources, IReadOnlyList`1 selectedDiagnostics, IReadOnlyList`1 allDiagnostics)
at Gu.Roslyn.Asserts.RoslynAssert.Diagnostics(DiagnosticAnalyzer analyzer, DiagnosticsAndSources diagnosticsAndSources, Settings settings)
1) at Gu.Roslyn.Asserts.RoslynAssert.VerifyDiagnostics(DiagnosticsAndSources diagnosticsAndSources, IReadOnlyList`1 selectedDiagnostics, IReadOnlyList`1 allDiagnostics)
at Gu.Roslyn.Asserts.RoslynAssert.Diagnostics(DiagnosticAnalyzer analyzer, DiagnosticsAndSources diagnosticsAndSources, Settings settings)
manfred-brands
left a comment
manfred-brands
left a comment
There was a problem hiding this comment.
You are on a roll @mikkelbu.
Should we add support and tests on UsingPropertiesComparer for SomeItem?
| if ((!IsDoesContain(constraintPart) && !IsContainsItem(constraintPart)) | ||
| || constraintPart.Root?.Type?.GetFullMetadataName() != NUnitFrameworkConstants.FullNameOfSomeItemsConstraint) | ||
| || (constraintPart.Root?.Type?.GetFullMetadataName() != NUnitFrameworkConstants.FullNameOfSomeItemsConstraint | ||
| && constraintPart.Root?.Type?.GetFullMetadataName() != NUnitV4FrameworkConstants.FullNameOfSomeItemsConstraintGeneric)) |
There was a problem hiding this comment.
I agree with Copilot or alternatively use patterns:
|| (constraintPart.Root?.Type?.GetFullMetadataName() is not NUnitFrameworkConstants.FullNameOfSomeItemsConstraint
and not NUnitV4FrameworkConstants.FullNameOfSomeItemsConstraintGeneric))Was this to make existing tests pass?
I don't see a new nunit test that uses UsingPropertiesComparer nor code treating this as matching types
manfred-brands
left a comment
manfred-brands
left a comment
There was a problem hiding this comment.
Thanks @mikkelbu
Nothing further.
As part of this bumping the tests to NUnit version
4.4.0-alpha.0.22.Fixes #857