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

Add an easy way to increase analyzer severity for rules that are good default, but a breaking change #3325

Open
nohwnd opened this issue Jul 24, 2024 · 1 comment · May be fixed by #4712
Open

Add an easy way to increase analyzer severity for rules that are good default, but a breaking change #3325

nohwnd opened this issue Jul 24, 2024 · 1 comment · May be fixed by #4712
Assignees
@Youssef1313
Labels
Area: Analyzers In-PR
Milestone
MSTest 3.8 / Plat...

Comments

@nohwnd
Copy link
Member

nohwnd commented Jul 24, 2024 •
edited by Evangelink
Loading

Summary

MSTest v3 ships with a set of analyzers which are balanced to avoid braking changes.

Add a way to increase the severity of such rules for new projects so they can start with stricter rule set.

Get Warning for the case above, and use the recommended solution of making the class abstract when it is just a base class.

Background and Motivation

As an example: MSTEST0030, which reports info on non-abstract test class that has [TestMethod] but is not decorated with [TestClass]. This leads to the TestMethod1 being skipped by the test framework.

public class UnitTest1
{
    [TestMethod]
    public void TestMethod1()
    {
    }
}

The rule cannot be set to warning by default, because this is also a common pattern to create a base class with tests that are inherited, but are not intended to run. A recommended solution for this case is to mark the class abstract.

In my test base I would like this rule, and other such rules to be warnings for any new project, without breaking my old projects that also use mstest v3.

Proposed Feature

Add build property or other knob to enable more strict analyzer rules for new projects, and make this the default in dotnet new templates, so new projects start with stricter rules, because they are more likely to embrace our recommended fixes and workarounds.

Possibly take advantage of analysis level https://learn.microsoft.com/dotnet/fundamentals/code-analysis/overview?tabs=net-8#enable-additional-rules

Alternative Designs
@Youssef1313 Youssef1313 self-assigned this Jan 3, 2025
@Youssef1313
Copy link
Member

Detailed proposal

We will introduce a new MSBuild property, MSTestAnalysisMode, the allowed values are similar to AnalysisMode for Code Quality (aka CAxxxx) analyzers:

  • None: Disables all analyzers. User will need to opt-in to all analyzers.
  • Default: Follow the current behavior. Rules that are enabled by default as warnings remain enabled as warnings. Anything else isn't enabled by default.
  • Recommended: Enables everything from Default, plus anything that's enabled by default as "info" will be elevated to warning.
  • All: Enables all rules as build warnings, including those that are disabled by default. Certain rules can be excluded from All similar to what roslyn-analyzers does. Relevant parts of implementation: Globalconfig generation, Custom tags handling. We may not follow exactly the same implementation details for roslyn-analyzers (e.g, they are relying on dynamic nuspec generation which isn't a good idea).
  • We will not have Minimum for now as the four levels None, Default, Recommended, and All should be sufficient for us.

Basic implementation overview

We will ship globalconfigs within the MSTest.Analyzers package, and depending on the value of MSTestAnalysisMode, we will inject the right globalconfig file. Those globalconfigs should have the right level so that user can override severities.

The globalconfigs will be checked in source control, and we will have a tool that auto-updates it based on the rules noted earlier. The CI will have a step that makes sure the current globalconfigs are up-to-date.

The tool will work by simply creating an AnalyzerFileReference from the MSTest.Analyzers.dll, get all analyzers, determine the current severity and whether or not rule is enabled by default, then determine if the analyzer should be enabled for the mode that the tool is currently generating for.

@Youssef1313 Youssef1313 linked a pull request Jan 18, 2025 that will close this issue
Draft
@Youssef1313 Youssef1313 added this to the MSTest 3.8 / Platform 1.6 milestone Jan 18, 2025
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
Projects
None yet
Milestone
MSTest 3.8 / Platform 1.6
Development

Successfully merging a pull request may close this issue.

Implement MSTestAnalysisMode Youssef1313/testfx
3 participants
@nohwnd @Evangelink @Youssef1313