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.

Sign up for GitHub

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

Jump to bottom

new Analyzer+CodeFix: recommend against wrong assertion method (e.g. Assert.IsTrue(x != null)) #3315

Open
LukasGelke opened this issue Jul 23, 2024 · 5 comments · May be fixed by #4162
Open

new Analyzer+CodeFix: recommend against wrong assertion method (e.g. Assert.IsTrue(x != null)) #3315

LukasGelke opened this issue Jul 23, 2024 · 5 comments · May be fixed by #4162
Assignees
@Youssef1313
Labels
Area: Analyzers In-PR sprint Type: Feature
Milestone
MSTest 3.7 / Plat...

Comments

@LukasGelke
Copy link

LukasGelke commented Jul 23, 2024 •
edited by pavelhorak
Loading

Summary

As some assertions may be written in different ways, some of them provide a better experience: better/richer message, clearer intent, better readability/maintainability.

Background and Motivation

  • default Exception-Messages are better
string text = "Mouse";
Assert.IsTrue("House" == text);
//  Assert.IsTrue failed.
Assert.AreEqual("House", text);
// Assert.AreEqual failed. Expected:<House>. Actual:<Mouse>.

This is also useful when a test is failing in a pipeline, or you get the stack trace only. This way you may not have to dig too deep and "waste" time debugging, just to get to that point to know what text actually is. Or when you get a notification from the pipeline a la test method "VeryImportantTest" failed with message "Assert.IsTrue failed."

Proposed Feature

essentially all permutations of IsFalse/IsTrue with is/is not/==/!=

  • Assert.IsNotNull, e.g.

    • Assert.IsTrue(x != null) => Assert.IsNotNull(x)
    • Assert.IsTrue(null != x) => Assert.IsNotNull(x)
    • Assert.IsFalse(x != null) => Assert.IsNull(x)
    • Assert.IsTrue(x is not null) => Assert.IsNotNull(x)
    • ...
    • Assert.AreNotEqual(null, x) => Assert.IsNotNull(x)
    • ...
    • either side may be null literal

  • Assert.Are[Not]Equal, e.g.

    • Assert.IsTrue(x == y) => Assert.AreEqual(x, y)
    • Assert.IsTrue(x != y) => Assert.AreNotEqual(x, y)
    • Assert.IsFalse(x == y) => Assert.AreNotEqual(x, y)
    • Assert.IsFalse(x != y) => Assert.AreEqual(x, y)
    • ...
    • maybe don't be overly smart, and just let the 'const in expected' be handled by the other analyzer

  • Assert.IsTrue/IsFalse, e.g.

    • Assert.AreEqual(true, b) => Assert.IsTrue(b)
    • only when there is a bool const as one param
    • may be a cascaded result result from Assert.IsTrue(true == b), stupid, but possible; again, just let it do one step at a time, for simplicity?

Edit 1:
maybe also consider:

  • Assert.AreEqual(typeof(string), o.GetType()) => Assert.IsInstanceOfType<string>(o)

also wouldn't matter to me if they are all the same DiagnosticID, since those cases are all "wrong similarly"

Alternative Designs

As mentioned, our analyzer grew over time and is quite complex. And I'm not sure if we have a bug or a missing case. Having such an Analyzer+CodeFix in MsTest directly would also provide a wider audience for finding such "bugs".

AB#2200926
@LukasGelke LukasGelke changed the title new Analyzer+CodeFix: reccomend against wrong assertion method (e.g. Assert.IsTrue(x != null)) new Analyzer+CodeFix: recommend against wrong assertion method (e.g. Assert.IsTrue(x != null)) Jul 23, 2024
@Evangelink Evangelink added this to the MSTest 3.6 / Platform 1.4 milestone Jul 23, 2024
@LukasGelke
Copy link
Author

Even though it's more tedious than complex, I was given permission to share our existing code for this Analyzer and CodeFix if necessary, since it only affects MsTest.
I may also be able to contribute it directly, if I could get the build to work. However due to current planning, this is only possible on 2014-08-26 at the earliest. (and as per guide, would only start if this is approved and I'm allowed to do so)

@Evangelink
Copy link
Member

Hi @LukasGelke,

I have a good idea of how to implement this analyzer but if you are allowed to share the code, please feel free to paste it/enclose it here. If we haven't yet started by end of August, please feel free to do the contribution :)

Thanks again!

@LukasGelke
Copy link
Author

Hey @Evangelink, I'm sorry. But i just got around to start working on this.
All seems good, except one thing: .\Build.cmd and VS (.\MSTest.slnf) still complain that net5 is somehow missing.
Also the dev pack for net5 is not available for VS2022, so I'm not sure what to install, except the "plain" SDK.
The docs/dev-guide.md seems a bit out-of-date since Universal Windows Platform development and .Net Core cross-platform development are available as "workloads" anymore(?). I do however have the "individual component" Windows 10 SDK (10.0.20348.0) installed already. (My machine is still Win10)

I've successfully install net9 (from the "plain" installer) though; as complained at me by the global.json which now seems to work.

C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\Microsoft.Common.CurrentVersion.targets(12
59,5): error MSB3644: The reference assemblies for .NETCore,Version=v5.0 were not found. To resolve this, install the D
eveloper Pack (SDK/Targeting Pack) for this framework version or retarget your application. You can download .NET Frame
work Developer Packs at https://aka.ms/msbuild/developerpacks [D:\GitHub\testfx\src\Adapter\MSTestAdapter.PlatformServi
ces\MSTestAdapter.PlatformServices.csproj::TargetFramework=uap10.0.16299]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\Microsoft.Common.CurrentVersion.targets(12
59,5): error MSB3644: The reference assemblies for .NETCore,Version=v5.0 were not found. To resolve this, install the D
eveloper Pack (SDK/Targeting Pack) for this framework version or retarget your application. You can download .NET Frame
work Developer Packs at https://aka.ms/msbuild/developerpacks [D:\GitHub\testfx\src\Adapter\MSTest.TestAdapter\MSTest.T
estAdapter.csproj::TargetFramework=uap10.0.16299]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\Microsoft.Common.CurrentVersion.targets(12
59,5): error MSB3644: The reference assemblies for .NETCore,Version=v5.0 were not found. To resolve this, install the D
eveloper Pack (SDK/Targeting Pack) for this framework version or retarget your application. You can download .NET Frame
work Developer Packs at https://aka.ms/msbuild/developerpacks [D:\GitHub\testfx\src\TestFramework\TestFramework.Extensi
ons\TestFramework.Extensions.csproj::TargetFramework=uap10.0.16299]
> dotnet --list-sdks
5.0.408 [C:\Program Files\dotnet\sdk]
6.0.425 [C:\Program Files\dotnet\sdk]
7.0.102 [C:\Program Files\dotnet\sdk]
7.0.120 [C:\Program Files\dotnet\sdk]
8.0.108 [C:\Program Files\dotnet\sdk]
8.0.206 [C:\Program Files\dotnet\sdk]
8.0.400 [C:\Program Files\dotnet\sdk]
9.0.100-rc.1.24452.12 [C:\Program Files\dotnet\sdk]

> dotnet --list-runtimes
Microsoft.AspNetCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 6.0.33 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 7.0.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 7.0.20 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 8.0.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 8.0.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 9.0.0-rc.1.24452.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 6.0.33 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 7.0.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 7.0.20 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 8.0.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 8.0.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 9.0.0-rc.1.24431.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 6.0.33 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 7.0.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 7.0.20 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 8.0.6 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 8.0.8 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 9.0.0-rc.1.24452.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

@Youssef1313
Copy link
Member

Assert.AreEqual(typeof(string), o.GetType()) may be different from IsInstanceOfType in certain cases. This is because IsInstanceOfType doesn't check for the exact type, but uses IsAssignableFrom. For example, IsInstanceOfType<object> will always pass, but Assert.AreEqual(typeof(object), o.GetType()) may not.

@Youssef1313 Youssef1313 mentioned this issue Nov 12, 2024
Merged
@Youssef1313
Copy link
Member

The analyzer is now implemented. The issue is open to track implementing codefix

@Youssef1313 Youssef1313 linked a pull request Nov 26, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Youssef1313 Youssef1313

Labels
Area: Analyzers In-PR sprint Type: Feature
Projects
None yet
Milestone
MSTest 3.7 / Platform 1.5
Development

Successfully merging a pull request may close this issue.

Implement codefix for MSTEST0037: Use proper 'Assert' methods Youssef1313/testfx
4 participants
@Evangelink @engyebrahim @Youssef1313 @LukasGelke