Add missing analyzers docs #50819
Add missing analyzers docs #50819
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds documentation for seven MSTest analyzer rules (MSTEST0056-MSTEST0062) that were missing from the documentation set. These analyzers will be released in MSTest 4.1 (with two from 4.0.0). The documentation follows the established pattern for analyzer rule documentation with frontmatter, property tables, examples, and suppression guidance.
Key changes:
- Adds 7 new analyzer rule documentation files with comprehensive examples and guidance
- Updates the TOC to include navigation entries for the new rules
- Extends the usage-rules.md summary table with descriptions of the new analyzers
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/navigate/devops-testing/toc.yml | Adds navigation entries for MSTEST0056-MSTEST0062 |
| docs/core/testing/mstest-analyzers/usage-rules.md | Updates the analyzer summary table with 7 new rule entries |
| docs/core/testing/mstest-analyzers/mstest0056.md | Documents the DisplayName property usage rule |
| docs/core/testing/mstest-analyzers/mstest0057.md | Documents the source information propagation rule for custom attributes |
| docs/core/testing/mstest-analyzers/mstest0058.md | Documents avoiding assertions in catch blocks |
| docs/core/testing/mstest-analyzers/mstest0059.md | Documents the Parallelize/DoNotParallelize conflict rule |
| docs/core/testing/mstest-analyzers/mstest0060.md | Documents avoiding duplicate test method attributes |
| docs/core/testing/mstest-analyzers/mstest0061.md | Documents using OSCondition attribute instead of runtime checks |
| docs/core/testing/mstest-analyzers/mstest0062.md | Documents avoiding out/ref parameters in test methods |
Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com>
|
@gewarren thank you for your review. I have left 2 comments on parts where I would really like your input. |
|
|
||
| ## Rule description | ||
|
|
||
| When specifying a custom display name for a test method, you should use the <xref:Microsoft.VisualStudio.TestTools.UnitTesting.TestMethodAttribute.DisplayName> property instead of passing a string as a constructor argument. This ensures consistent usage across the MSTest framework and improves code readability. |
There was a problem hiding this comment.
It's not about readability. It's to alert about the breaking change from MSTest 3.x where code might continue to compile but won't do the right thing.
|
|
||
| ## When to suppress warnings | ||
|
|
||
| Do not suppress warnings from this rule. Using the `DisplayName` property is the correct and recommended way to specify custom display names for test methods. |
There was a problem hiding this comment.
| Do not suppress warnings from this rule. Using the `DisplayName` property is the correct and recommended way to specify custom display names for test methods. | |
| Do not suppress warnings from this rule. Using the `DisplayName` property is the only way to specify custom display names for test methods. |
- probably extend with something along the lines of "if the intent is to pass a value to caller file path, you can suppress the warning".
|
|
||
| ## Rule description | ||
|
|
||
| When creating custom test method attributes that derive from <xref:Microsoft.VisualStudio.TestTools.UnitTesting.TestMethodAttribute>, you should propagate source information using caller information attributes. This allows MSTest to correctly track the source file and line number for test methods, improving diagnostics and test result reporting. |
There was a problem hiding this comment.
| When creating custom test method attributes that derive from <xref:Microsoft.VisualStudio.TestTools.UnitTesting.TestMethodAttribute>, you should propagate source information using caller information attributes. This allows MSTest to correctly track the source file and line number for test methods, improving diagnostics and test result reporting. | |
| When creating custom test method attributes that derive from <xref:Microsoft.VisualStudio.TestTools.UnitTesting.TestMethodAttribute>, you should propagate source information using caller information attributes. This allows MSTest to correctly track the source file and line number for test methods, improving diagnostics, test result reporting, and Test Explorer behavior. |
| public void TestMethod() | ||
| { | ||
| try | ||
| { | ||
| // Code that might throw. | ||
| DoSomethingThatMightThrow(); | ||
| } | ||
| catch | ||
| { | ||
| Assert.Fail("Exception was thrown"); // Violation | ||
| } | ||
| } |
There was a problem hiding this comment.
Example doesn't look correct. That example strictly asserts that the method doesn't throw, and transforming that to Assert.Throws is not correct. A better example would be catching a specific exception and asserting its message in the catch block.
|
|
||
| ## How to fix violations | ||
|
|
||
| Use `Assert.ThrowsException` or related assertion methods to test for exceptions. |
There was a problem hiding this comment.
We shouldn't mention the old ThrowsException, I think.
| ```csharp | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
|
|
||
| [assembly: Parallelize(Workers = 2, Scope = ExecutionScope.MethodLevel)] // Violation |
There was a problem hiding this comment.
nit: Should we show the example using Workers = 0?
| public class TestClass | ||
| { | ||
| [TestMethod] | ||
| [DataTestMethod] // Violation |
There was a problem hiding this comment.
Probably better to not mention DataTestMethodAttribute here and pick something else. It has [EditorBrowsable(EditorBrowsableState.Never)] and is discouraged to use.
|
|
||
| ## Rule description | ||
|
|
||
| Test methods should not have `out` or `ref` parameters because these modifiers are incompatible with data-driven testing. Data sources provide values to test methods through standard parameter passing, and the `out` and `ref` modifiers create conflicts with this mechanism. Using regular parameters makes tests more maintainable and compatible with parameterized test features. |
There was a problem hiding this comment.
A more clear description (IMO) would be a long the lines of "MSTest is responsible for calling test methods, and is never going to read the values that are passed by reference after the test method finishes. Removing the 'out' and 'ref' modifiers makes clearer tests."
| [TestMethod] | ||
| public void TestMethod(string s, string s2) | ||
| { | ||
| // Test code | ||
| } |
There was a problem hiding this comment.
This example still isn't correct as the test method isn't parameterized. Same as previous example showing the violation.
| [TestClass] | ||
| public class TestClass | ||
| { | ||
| [DataTestMethod] |
There was a problem hiding this comment.
This should switch to [TestMethod]
|
|
||
| ## When to suppress warnings | ||
|
|
||
| You might suppress this warning if you have a specific advanced scenario that requires `out` or `ref` parameters. However, this is rare, and you should consider alternative test designs first. |
There was a problem hiding this comment.
I can't see any scenario where this would be valid.
Summary
Add the 2 missing shipped analyzers docs + all the rules we will soon be shipping (MSTest 4.1)
cc @Youssef1313
Internal previews
catchblocks