Skip to content
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

Remove null message in AreEqual code fix #698

Merged
merged 2 commits into from
Mar 5, 2024
Merged

Remove null message in AreEqual code fix #698

merged 2 commits into from
Mar 5, 2024

Conversation

verdie-g
Copy link
Contributor

@verdie-g verdie-g commented Mar 4, 2024

Fixes #700

Assert.AreEqual(2d, 3d, null);

would generate

Assert.That(3d, Is.EqualTo(2d), null);

which does not compile

The call is ambiguous between the following methods or properties:
'Assert.That<TActual>(TActual, IResolveConstraint, FormattableString,
string, string)' and 'Assert.That<TActual>(TActual, IResolveConstraint,
Func<string>, string, string)'

```
Assert.AreEqual(2d, 3d, null);
```
would generate
```
Assert.That(3d, Is.EqualTo(2d), null);
```
which does not compile
```
The call is ambiguous between the following methods or properties:
'Assert.That<TActual>(TActual, IResolveConstraint, FormattableString,
string, string)' and 'Assert.That<TActual>(TActual, IResolveConstraint,
Func<string>, string, string)'
```
Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

With a small change, your fix would also work for the other use case.

Comment on lines 58 to 63
// Remove null message to avoid ambiguous calls.
if (arguments.Count == 3 && arguments[2].Expression.Kind() == SyntaxKind.NullLiteralExpression)
{
arguments.RemoveAt(2);
}

Copy link
Member

Choose a reason for hiding this comment

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

This needs to move after the call to UpdateArguments below so that it also works if there is a tolerance value specified.

@@ -56,6 +56,22 @@ public void TestMethod()
RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
}

[Test]
public void VerifyAreEqualFixWithNullMessage()
Copy link
Member

Choose a reason for hiding this comment

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

Please add another test with a tolerance value: ClassicAssert.AreEqual(2d, 3d, 0.1, null);

@manfred-brands
Copy link
Member

Thank you @verdie-g for the PR. For administration sake I have created an issue (#700) for this.

@@ -55,6 +55,12 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
// Now, replace the arguments.
List<ArgumentSyntax> arguments = invocationNode.ArgumentList.Arguments.ToList();

// Remove null message to avoid ambiguous calls.
if (arguments.Count == 3 && arguments[2].Expression.Kind() == SyntaxKind.NullLiteralExpression)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (arguments.Count == 3 && arguments[2].Expression.Kind() == SyntaxKind.NullLiteralExpression)
if (arguments.Count == 3 && arguments[2].Expression.IsKind(SyntaxKind.NullLiteralExpression))

@verdie-g verdie-g requested a review from manfred-brands March 4, 2024 23:50
Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

Thanks you for your quick fix. Nothing else.

@manfred-brands manfred-brands merged commit 47648c5 into nunit:master Mar 5, 2024
6 checks passed
@mikkelbu mikkelbu added this to the Release 4.1 milestone Mar 5, 2024
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.

CodeFix for Assert with null message causes ambiguous code.
3 participants