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

Replace UpdateStringFormatToFormattableString with String.Format #720

Closed
Bartleby2718 opened this issue Apr 7, 2024 · 5 comments · Fixed by #725
Closed

Replace UpdateStringFormatToFormattableString with String.Format #720

Bartleby2718 opened this issue Apr 7, 2024 · 5 comments · Fixed by #725
Assignees
Milestone

Comments

@Bartleby2718
Copy link
Contributor

Consider the following (mostly similar) tests:

        [Test]
        public void VerifyAreEqualFixWithMessageAndParamsInStandardOrder1()
        {
            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            ↓ClassicAssert.AreEqual(""expected"", ""actual"", ""{0}"", ""first"");
        }");
            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            Assert.That(""actual"", Is.EqualTo(""expected""), $""{""first""}"");
        }");
            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
        }

        [Test]
        public void VerifyAreEqualFixWithMessageAndParamsInStandardOrder2()
        {
            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            ↓ClassicAssert.AreEqual(""expected"", ""actual"", ""{0}"", new[] { ""first"" });
        }");
            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            Assert.That(""actual"", Is.EqualTo(""expected""), $""{""first""}"");
        }");
            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
        }

        [Test]
        public void VerifyAreEqualFixWithMessageAndParamsInStandardOrder3()
        {
            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            var args = new[] { ""first"", ""second"" };
            ↓ClassicAssert.AreEqual(""expected"", ""actual"", ""{0}, {1}"", args);
        }");
            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            var args = new[] { ""first"", ""second"" };
            Assert.That(""actual"", Is.EqualTo(""expected""), $""{""first""}, {""second""}"");
        }");
            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
        }

        [Test]
        public void VerifyAreEqualFixWithMessageAndParamsInStandardOrder4()
        {
            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            ↓ClassicAssert.AreEqual(""expected"", ""actual"", ""{0}, {1}"", ""first"", ""second"");
        }");
            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            Assert.That(""actual"", Is.EqualTo(""expected""), $""{""first""}, {""second""}"");
        }");
            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
        }

        [Test]
        public void VerifyAreEqualFixWithMessageAndParamsInStandardOrder5()
        {
            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            ↓ClassicAssert.AreEqual(""expected"", ""actual"", ""{0}, {1}"", new[] { ""first"", ""second"" });
        }");
            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            Assert.That(""actual"", Is.EqualTo(""expected""), $""{""first""}, {""second""}"");
        }");
            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
        }

        [Test]
        public void VerifyAreEqualFixWithMessageAndParamsInNonstandardOrder1()
        {
            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            ↓ClassicAssert.AreEqual(args: ""first"", actual: ""actual"", message: ""{0}"", expected: ""expected"");
        }");
            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            Assert.That(actual: ""actual"", Is.EqualTo(expected: ""expected""), $""{""first""}"");
        }");
            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
        }

        [Test]
        public void VerifyAreEqualFixWithMessageAndParamsInNonstandardOrder2()
        {
            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            ↓ClassicAssert.AreEqual(args: new[] { ""first"" }, actual: ""actual"", message: ""{0}"", expected: ""expected"");
        }");
            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            Assert.That(actual: ""actual"", Is.EqualTo(expected: ""expected""), $""{""first""}"");
        }");
            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
        }

        [Test]
        public void VerifyAreEqualFixWithMessageAndParamsInNonstandardOrder3()
        {
            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            ↓ClassicAssert.AreEqual(args: new[] { ""first"", ""second"" }, actual: ""actual"", message: ""{0}, {1}"", expected: ""expected"");
        }");
            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            Assert.That(actual: ""actual"", Is.EqualTo(expected: ""expected""), $""{""first""}, {""second""}"");
        }");
            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
        }

        [Test]
        public void VerifyAreEqualFixWithMessageAndParamsInNonstandardOrder4()
        {
            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            ↓ClassicAssert.AreEqual(args: new[] { ""first"", ""second"", ""third"" }, actual: ""actual"", message: ""{0}, {1}, {2}"", expected: ""expected"");
        }");
            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
        public void TestMethod()
        {
            Assert.That(actual: ""actual"", Is.EqualTo(expected: ""expected""), $""{""first""}, {""second""}, {""third""}"");
        }");
            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
        }

Some of these fail, even after #716. In order to fix InStandardOrder3, in particular, I believe we should replace GetInterpolatedMessageArgumentOrDefault with String.Format:

            var stringFormat = SyntaxFactory.MemberAccessExpression(
                SyntaxKind.SimpleMemberAccessExpression,
                SyntaxFactory.IdentifierName("String"),
                SyntaxFactory.IdentifierName("Format"));

            var argumentList = SyntaxFactory.ArgumentList(
                SyntaxFactory.SeparatedList<ArgumentSyntax>(
                    new SyntaxNodeOrToken[] { messageArgument.WithNameColon(null) }.Concat(
                        args.SelectMany((arg, _) => new SyntaxNodeOrToken[] { SyntaxFactory.Token(SyntaxKind.CommaToken), arg.WithNameColon(null) }))));

            return SyntaxFactory.Argument(SyntaxFactory.InvocationExpression(stringFormat, argumentList));

I think this will be come truly startable once dotnet/roslyn#68469 is done and released.

@manfred-brands
Copy link
Member

@Bartleby2718 The difference between an InterpolatedString and String.Format is that the first is lazy, i.e. the string is only formatted when the test fails. The latter will always be formatted before the test is run, making the test call slower.

For completeness your tests are valid and in case a NUnit test actually uses an args array, instead of separate arguments, we cannot even convert it if the args array is not-created in-line but in a separate statement.

So yes we would need to convert:

object[] args = new[] { ""first"", ""second"" };
ClassicAssert.AreEqual(args: args, actual: ""actual"", message: ""{0}, {1}"", expected: ""expected"");

into:

object[] args = new[] { ""first"", ""second"" };
Assert.That(actual: ""actual"", Is.EqualTo(expected: ""expected""), () => string.Format(""{0}, {1}"", args);

Note the addition of () => to make the call lazy.

@Bartleby2718
Copy link
Contributor Author

@manfred-brands
Copy link
Member

manfred-brands commented Apr 10, 2024

This issue is not just limited to 'named:' arguments, the current code will fail if the parameters for the format string are in an array.

[TestCase(3.0, 4)]
public void Test(object actual, object expected)
{
    object[] args = { expected, actual };
    Assert.AreEqual(expected, actual, "Expected: {0} Got: {1}", args);
}

The AreEqual gets converted into:

Assert.AreEqual(expected, actual, "Expected: {0} Got: {1}", args);

But should be converted into:

Assert.That(actual, Is.EqualTo(expected), () => string.Format("Expected: {0} Got: {1}", args));

@manfred-brands
Copy link
Member

Similar the following fails:

ClassicAssert.AreEqual(expected, actual, GetLocalizedFormatSpecification(), expected, actual);

@manfred-brands
Copy link
Member

I have implemented this on top of #716 and will rebase when that is merged.

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 a pull request may close this issue.

3 participants