Skip to content

feat: Add analyzer PH2162 and code fix for TestTimeouts class accessibility#964

Draft
Copilot wants to merge 7 commits intomainfrom
copilot/fix-c142eff0-214c-41c9-a481-d35a6ed03507
Draft

feat: Add analyzer PH2162 and code fix for TestTimeouts class accessibility#964
Copilot wants to merge 7 commits intomainfrom
copilot/fix-c142eff0-214c-41c9-a481-d35a6ed03507

Conversation

Copy link
Contributor

Copilot AI commented Sep 2, 2025

This PR implements a new Roslyn analyzer that enforces proper accessibility modifiers for classes named "TestTimeouts" used in MSTest projects.

Problem

Classes named TestTimeouts that contain test timeout constants are often declared with inappropriate accessibility modifiers, which can lead to:

  • Unnecessary public exposure of test infrastructure
  • Inconsistent accessibility patterns across test projects
  • Potential conflicts when classes are marked as internal static
  • Conflicts with the MSTest analyzer that restricts all classes in test projects from being public unless they have the [TestClass] attribute

Solution

Added TestTimeoutsClassAccessibilityAnalyzer (PH2162) that detects and provides automatic fixes for:

  • public class TestTimeoutsinternal class TestTimeouts
  • public sealed class TestTimeoutsinternal sealed class TestTimeouts
  • internal static class TestTimeoutsinternal class TestTimeouts

The analyzer ignores classes that are already properly declared as internal (non-static) and only targets classes specifically named "TestTimeouts".

Example

Before:

public class TestTimeouts
{
    public const int CiAppropriate = 1000;
    public const int Integration = 5000;
}

After (with code fix applied):

internal class TestTimeouts
{
    public const int CiAppropriate = 1000;
    public const int Integration = 5000;
}

Changes Made

  • DiagnosticId.cs: Added TestTimeoutsClassShouldBeInternal = 2162
  • TestTimeoutsClassAccessibilityAnalyzer.cs: New analyzer that detects problematic accessibility modifiers
  • TestTimeoutsClassAccessibilityCodeFixProvider.cs: Automatic code fix that corrects the modifiers
  • TestTimeoutsClassAccessibilityAnalyzerTest.cs: Comprehensive test coverage for all scenarios
  • PH2162.md: Documentation explaining the rule and how it helps avoid MSTest analyzer conflicts

The analyzer is enabled by default and includes full test coverage with 6 test cases covering all supported scenarios. All existing tests (369 MSTest-related tests) continue to pass, ensuring no regressions.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 3 commits September 2, 2025 18:36
Co-authored-by: ajbarga <99690046+ajbarga@users.noreply.github.com>
Co-authored-by: ajbarga <99690046+ajbarga@users.noreply.github.com>
… tests

Co-authored-by: ajbarga <99690046+ajbarga@users.noreply.github.com>
Copilot AI changed the title [WIP] Create a CodeFixProvider that makes any class called "TestTimeouts" "internal" if it is public / public sealed / internal static" Add analyzer and code fix for TestTimeouts class accessibility Sep 2, 2025
@ajbarga Alex Barga (ajbarga) changed the title Add analyzer and code fix for TestTimeouts class accessibility feat: Add analyzer and code fix for TestTimeouts class accessibility Sep 2, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 2, 2025


## Introduction

Projects commonly define a `TestTimeouts` class that centralizes timeout constants (e.g., `CiAppropriate`, `Integration`). This analyzer enforces that any class named `TestTimeouts` is declared as `internal` and not `static`. Public exposure or use of `static` on this coordination class is discouraged.

Choose a reason for hiding this comment

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

Perhaps mention how this rule helps avoid conflict with the MSTEST analyzer that says all classes in a test project cannot be public unless it's a [TestClass]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added mention of the MSTEST analyzer conflict avoidance in the documentation. The rule now explains how it helps avoid conflicts with the MSTest analyzer that restricts all classes in test projects from being public unless they have the [TestClass] attribute. Implemented in commit 439b4e0.

AvoidUnusedToString = 2153,
AvoidUnlicensedPackages = 2155,
AvoidPkcsPaddingWithRsaEncryption = 2158,
TestTimeoutsClassShouldBeInternal = 2159,

Choose a reason for hiding this comment

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

Let's use 2162 instead of 2159. 2159, 2160, and 2161 are claimed in other PRs. Remember to update the PH2159.md file name and contents as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated diagnostic ID from PH2159 to PH2162 and renamed documentation file accordingly. All references in DiagnosticId.cs and PH2162.md have been updated. Implemented in commit 439b4e0.

@bcollamore
Copy link
Member

Copilot see comments above

…ation

Co-authored-by: bcollamore <57269455+bcollamore@users.noreply.github.com>
@bcollamore Brian Collamore (bcollamore) changed the title feat: Add analyzer and code fix for TestTimeouts class accessibility feat: Add analyzer PH2162 and code fix for TestTimeouts class accessibility Sep 7, 2025
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.

3 participants