Add MSTEST0070 analyzer validating [MemberCondition] member references#9076
Open
Evangelink wants to merge 10 commits into
Open
Add MSTEST0070 analyzer validating [MemberCondition] member references#9076Evangelink wants to merge 10 commits into
Evangelink wants to merge 10 commits into
Conversation
Adds a new `Microsoft.VisualStudio.TestTools.UnitTesting.ConditionAttribute` deriving from `ConditionBaseAttribute` that evaluates one or more static `bool` members (property, field, or parameterless method) referenced by `Type` + member name to decide whether a test class or test method runs. This mirrors the `[ConditionalFact]` / `[ConditionalTheory]` pattern from `Microsoft.DotNet.XUnitExtensions` used heavily by dotnet/runtime, dotnet/sdk, and dotnet/aspnetcore, removing the need to define a one-off `ConditionBaseAttribute` subclass per condition (e.g. `Is64BitProcessCondition`). Behavior: - Multiple member names within a single attribute are AND-combined. - Each attribute instance gets a unique `GroupName` derived from the type and members, so stacking multiple `[Condition]` attributes ANDs them across instances (matching xUnit's usage pattern). - `ConditionMode.Exclude` flips the semantics (skip when the condition holds). - Member resolution looks up public/non-public `static bool` property, field, or parameterless method (in that order). On any resolution failure (missing, non-bool, non-static, instance-only, or method with parameters), `IsConditionMet` throws `InvalidOperationException` so the test fails rather than silently skipping -- avoiding the classic "typo in member name silently disables the test" pitfall. Provides four CLS-friendly ctor overloads (`(Type, string)`, `(ConditionMode, Type, string)` plus `params`-array variants marked `[CLSCompliant(false)]`). Uses `DynamicallyAccessedMembers` annotations for trimming/AOT correctness. Targets all existing TFMs (netstandard2.0, net462, net8.0, net9.0). Adds 23 unit tests in `TestFramework.UnitTests` covering argument validation, member resolution variants (public/non-public, property/field/method), AND-semantics, `ConditionMode.Exclude`, error messages, and `GroupName` uniqueness. Fixes #9070. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per design review, restrict the resolver to `BindingFlags.Public | Static` (was `Public | NonPublic | Static`). Rationale: - The `conditionType` is effectively acting as a public API surface for the condition. Non-public members of another type don't belong in that contract. - Slimmer `DynamicallyAccessedMembers` annotations for trim/AOT (3 flags instead of 6) -- only public properties, fields, and methods are preserved. - Matches xUnit's `MemberData` resolution which defaults to `Public | Static`. - Avoids surprises with `InternalsVisibleTo` when the condition type lives in a different assembly than the test. Error messages updated to say "public static bool"; XML docs aligned. Test `IsConditionMet_NonPublicStaticProperty_IsResolved` flipped to `IsConditionMet_NonPublicStaticProperty_ThrowsInvalidOperation`. All 23 tests still pass on net8.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…emberNames - Wrap `_conditionMemberNames` in a cached `ReadOnlyCollection<string>` so `ConditionMemberNames` can no longer be downcast to `string[]` and mutated. - In `EvaluateMember`, reject indexer properties (those with index parameters) alongside non-bool/non-readable properties so they consistently surface as `InvalidOperationException` instead of `TargetParameterCountException`. Use `GetGetMethod(nonPublic: true)` to reliably detect missing getters. - Add `ConditionMemberNames_CannotBeDowncastToMutableArray` test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
…nd tests - `IsConditionMet` now resolves each member once and caches a `Func<bool>` delegate. Subsequent evaluations skip the per-call `GetProperty` / `GetField` / `GetMethod` lookups (~3xN reflection lookups per access). - `GroupName` now includes `Mode` so two `[Condition]` attributes with the same type/members but opposite modes no longer share a group (previously they would silently OR-combine and the test would always run). - `IsConditionMet` uses `All` for the AND short-circuit, addressing the CodeQL note. - Added `IsConditionMet_ParameterlessMethodWithNonBoolReturn_ThrowsInvalidOperation`, `GroupName_DifferentMode_AreDifferentGroups`, and tightened `IgnoreMessage_Exclude_HasExpectedText` to also assert the type FullName (matching the Include variant). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ssary [CLSCompliant(false)] - The previous name was too generic and clashed conceptually with the existing ConditionBaseAttribute / OSConditionAttribute / CIConditionAttribute family. MemberConditionAttribute better reflects that the attribute targets a static member by name. - [CLSCompliant(false)] was not needed on the params string[] overloads: the existing DataRowAttribute(params object?[]?) and DynamicDataAttribute(string, params object?[]) constructors compile cleanly without it in the same CLS-compliant assembly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add `BindingFlags.FlattenHierarchy` to the member lookup so `[MemberCondition(typeof(Derived), nameof(Base.SomeFlag))]` resolves the inherited static member via the derived type, matching how C# allows accessing public static base members through a derived type name. - Tighten the property validation to require a public getter (`GetGetMethod(nonPublic: false)`). A public static property with a private getter doesn't satisfy the documented `public static bool readable property` contract and isn't guaranteed to be preserved by the `PublicProperties` trimming annotation on `ConditionType`. - Add `IsConditionMet_InheritedStaticProperty_ResolvesViaDerivedType` and `IsConditionMet_PropertyWithPrivateGetter_ThrowsInvalidOperation` tests (plus matching fixture types). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Base automatically changed from
evangelink/issue-9070-condition-attribute
to
main
June 12, 2026 15:04
Adds `MemberConditionShouldBeValidAnalyzer` (MSTEST0070) which catches at build time the same errors that `MemberConditionAttribute.IsConditionMet` throws at runtime, so typos and refactors don't silently break test gating. Rules emitted (one diagnostic per member name in the attribute): - MemberNotFoundRule -- the named member doesn't exist on the type - MemberNotPublicRule -- member is not public - MemberNotStaticRule -- member is not static - MemberWrongKindRule -- member is an event/nested type/etc. - MemberWrongReturnTypeRule -- property/field/method doesn't return bool - MethodHasParametersRule -- referenced method has parameters - PropertyNotReadableRule -- property has no public getter The analyzer fires on any symbol carrying [MemberCondition] (test class or test method), walks each constructor argument, validates every member name, and matches the runtime resolution order (property -> field -> method) including inherited public static members. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Evangelink
force-pushed
the
evangelink/issue-9070-condition-analyzer
branch
from
53875bf to
4d9c906
Compare
Evangelink
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds the new [MemberCondition] feature end-to-end in the MSTest ecosystem: a runtime attribute (MemberConditionAttribute) plus the follow-up analyzer (MSTEST0070) that validates referenced members at build time, including resources/release-notes and unit tests.
Changes:
- Introduces
MemberConditionAttribute(public API) that gates tests on one or morepublic static boolmembers (property/field/parameterless method), with runtime validation and caching. - Adds
MSTEST0070 (MemberConditionShouldBeValidAnalyzer)to validate[MemberCondition]member references at compile time, with localized strings and analyzer release metadata. - Adds unit tests for the runtime attribute and analyzer behavior.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/TestFramework.UnitTests/Attributes/MemberConditionAttributeTests.cs | Adds runtime behavior/validation tests for MemberConditionAttribute. |
| test/UnitTests/MSTest.Analyzers.UnitTests/MemberConditionShouldBeValidAnalyzerTests.cs | Adds analyzer tests for MSTEST0070 diagnostics. |
| src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt | Records the new public API surface for MemberConditionAttribute. |
| src/TestFramework/TestFramework/Attributes/TestMethod/MemberConditionAttribute.cs | Implements the new MemberConditionAttribute runtime behavior. |
| src/Analyzers/MSTest.Analyzers/MemberConditionShouldBeValidAnalyzer.cs | Implements the MSTEST0070 analyzer for validating [MemberCondition] arguments. |
| src/Analyzers/MSTest.Analyzers/Resources.resx | Adds localized resource strings for MSTEST0070 title/description/messages. |
| src/Analyzers/MSTest.Analyzers/AnalyzerReleases.Unshipped.md | Documents the new MSTEST0070 rule in analyzer release notes. |
| src/Analyzers/MSTest.Analyzers/Helpers/DiagnosticIds.cs | Adds the new diagnostic ID constant MSTEST0070. |
| src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs | Adds MemberConditionAttribute to well-known type name constants. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlf | Propagates new MSTEST0070 strings to Czech localization file. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlf | Propagates new MSTEST0070 strings to German localization file. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlf | Propagates new MSTEST0070 strings to Spanish localization file. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlf | Propagates new MSTEST0070 strings to French localization file. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlf | Propagates new MSTEST0070 strings to Italian localization file. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlf | Propagates new MSTEST0070 strings to Japanese localization file. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlf | Propagates new MSTEST0070 strings to Korean localization file. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlf | Propagates new MSTEST0070 strings to Polish localization file. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlf | Propagates new MSTEST0070 strings to Portuguese (Brazil) localization file. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlf | Propagates new MSTEST0070 strings to Russian localization file. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlf | Propagates new MSTEST0070 strings to Turkish localization file. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlf | Propagates new MSTEST0070 strings to Chinese (Simplified) localization file. |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlf | Propagates new MSTEST0070 strings to Chinese (Traditional) localization file. |
Copilot's findings
- Files reviewed: 22/22 changed files
- Comments generated: 3
Comment on lines
+170
to
+174
| // Match the runtime resolution preference: property → field → method (first match wins). | ||
| ISymbol? selected = | ||
| candidates.FirstOrDefault(s => s.Kind == SymbolKind.Property) | ||
| ?? candidates.FirstOrDefault(s => s.Kind == SymbolKind.Field) | ||
| ?? candidates.FirstOrDefault(s => s.Kind == SymbolKind.Method); |
Member
Author
There was a problem hiding this comment.
Fixed in 1fccdc3. ValidateMember now first tries to find a candidate that the runtime would actually bind to (Public + Static + FlattenHierarchy, ordinary parameterless methods only), and only falls back to the previous first-by-kind heuristic when no such candidate exists. Added WhenMethodHasParameterlessAndParameterizedOverloads_PicksParameterless_NoDiagnostic and WhenDerivedHasInstanceMemberShadowingBaseStatic_PicksBaseStatic_NoDiagnostic covering both scenarios you flagged. Review reply handled.
Comment on lines
+236
to
+240
| if (property.GetMethod is null || property.GetMethod.DeclaredAccessibility != Accessibility.Public) | ||
| { | ||
| context.ReportDiagnostic(attributeSyntax.CreateDiagnostic(PropertyNotReadableRule, typeName, memberName)); | ||
| return; | ||
| } |
Member
Author
There was a problem hiding this comment.
Fixed in 1fccdc3. ValidateProperty now rejects indexer properties (property.IsIndexer) up front with MemberWrongKindRule, matching the runtime InvalidOperationException for properties with index parameters. The runtime-resolution filter in ValidateMember also already excludes indexers when looking for the preferred candidate. Review reply handled.
Comment on lines
+13
to
+17
| [TestMethod] | ||
| public async Task WhenMemberIsValidPublicStaticBoolProperty_NoDiagnostic() | ||
| { | ||
| string code = """ | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; |
Member
Author
There was a problem hiding this comment.
Added 2 of the 3 cases you suggested in 1fccdc3: WhenMethodHasParameterlessAndParameterizedOverloads_PicksParameterless_NoDiagnostic and WhenDerivedHasInstanceMemberShadowingBaseStatic_PicksBaseStatic_NoDiagnostic. Skipped the explicit indexer test since C# 13 static indexers (public static bool this[int]) are rare/new and the indexer path is now exercised end-to-end by ValidateProperty's IsIndexer guard (which would surface MSTEST0070 via MemberWrongKindRule for any indexer). Happy to add it on follow-up if you'd like coverage proof. Review reply handled.
…-condition-analyzer
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
…dexers
- `MemberConditionShouldBeValidAnalyzer`: candidate selection in
`ValidateMember` no longer just picks "first by Kind". It now first tries
to find a candidate that the runtime would actually bind to
(`GetProperty`/`GetField`/`GetMethod` with `Public | Static |
FlattenHierarchy`, methods restricted to `MethodKind.Ordinary` and
parameterless), and only falls back to the previous first-by-kind heuristic
when no such candidate exists. This fixes:
* Overloaded methods where a parameterless overload coexists with
parameterized ones (runtime binds to the parameterless one).
* An instance member on a derived type shadowing a `public static` base
member with the same name (runtime resolves via `FlattenHierarchy` to
the base static).
Behaviour for "no runtime-valid candidate" cases is preserved -- the
fallback still selects a candidate so the more specific diagnostic
(not-public/not-static/wrong-return-type) is reported.
- `ValidateProperty`: explicitly reject indexer properties
(`property.IsIndexer`) with `MemberWrongKindRule`, matching the runtime
`InvalidOperationException` for properties with index parameters. (The
analyzer was previously silent on this category and would let MSTEST0070
miss cases that would still throw at runtime.)
- Apply `Where` filter to the `argument.Values` enumeration to address
the github-code-quality LINQ note and avoid the implicit filtering pattern.
- Tests: add
`WhenMethodHasParameterlessAndParameterizedOverloads_PicksParameterless_NoDiagnostic`
and
`WhenDerivedHasInstanceMemberShadowingBaseStatic_PicksBaseStatic_NoDiagnostic`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
31
to
35
| public const string MicrosoftVisualStudioTestToolsUnitTestingInheritanceBehavior = "Microsoft.VisualStudio.TestTools.UnitTesting.InheritanceBehavior"; | ||
| public const string MicrosoftVisualStudioTestToolsUnitTestingMemberConditionAttribute = "Microsoft.VisualStudio.TestTools.UnitTesting.MemberConditionAttribute"; | ||
| public const string MicrosoftVisualStudioTestToolsUnitTestingOwnerAttribute = "Microsoft.VisualStudio.TestTools.UnitTesting.OwnerAttribute"; | ||
| public const string MicrosoftVisualStudioTestToolsUnitTestingParallelizeAttribute = "Microsoft.VisualStudio.TestTools.UnitTesting.ParallelizeAttribute"; | ||
| public const string MicrosoftVisualStudioTestToolsUnitTestingPriorityAttribute = "Microsoft.VisualStudio.TestTools.UnitTesting.PriorityAttribute"; |
Member
Author
There was a problem hiding this comment.
Fixed in fc21eee -- moved MicrosoftVisualStudioTestToolsUnitTestingOSConditionAttribute to its correct alphabetical position (between ...MemberConditionAttribute and ...OwnerAttribute). Review reply handled.
Comment on lines
+122
to
+125
| if (argument.Kind == TypedConstantKind.Type && argument.Value is INamedTypeSymbol typeValue) | ||
| { | ||
| conditionType = typeValue; | ||
| } |
Member
Author
There was a problem hiding this comment.
Fixed in fc21eee. AnalyzeAttribute now accepts any ITypeSymbol for the condition type. When the captured type isn't an INamedTypeSymbol (e.g. typeof(int[])), we report MSTEST0070 (MemberNotFoundRule) for each provided member name using ToDisplayString(MinimallyQualifiedFormat) for the type label, so the user sees the same diagnostic at edit time that the runtime would throw at first access. Added WhenConditionTypeIsArrayType_MemberNotFound covering [MemberCondition(typeof(int[]), "AnyName")]. Review reply handled.
…or non-named types, rename test - `WellKnownTypeNames.cs`: move `MicrosoftVisualStudioTestToolsUnitTestingOSConditionAttribute` to its alphabetical position between `Member...` and `Owner...` so the file matches its "Keep sorted alphabetically" header. - `MemberConditionShouldBeValidAnalyzer.AnalyzeAttribute`: accept any `ITypeSymbol` for the condition type rather than only `INamedTypeSymbol`. Non-named types (arrays, pointers, function pointers) can't carry user-declared static bool members and the runtime throws `InvalidOperationException`; the analyzer now surfaces that as MSTEST0070 (`MemberNotFoundRule`) instead of silently skipping the attribute. Uses `ToDisplayString(MinimallyQualifiedFormat)` for the type name when `conditionType.Name` is empty (as it is for `IArrayTypeSymbol`). - Rename `WhenStaticFieldIsInstance_MemberNotStatic` to `WhenFieldIsInstance_MemberNotStatic` -- the field under test is an instance field, not a static one, so the original name was misleading. - Add `WhenConditionTypeIsArrayType_MemberNotFound` test covering `[MemberCondition(typeof(int[]), "AnyName")]`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
🧪 Test quality grade — PR #907621 new test methods graded across 1 new file (
This advisory comment was generated automatically. Grades are heuristic
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #9071 (per design review).
Summary
Adds
MSTEST0070 — MemberConditionShouldBeValidAnalyzer, which catches at build time the same errors thatMemberConditionAttribute.IsConditionMetthrows at runtime, so typos and refactors don''t silently break test gating.This is the analyzer counterpart called out as a follow-up in #9071 and addresses the user feedback "add an analyzer to detect if the member targets (or type detected) is invalid".
Diagnostics
One diagnostic is produced per invalid member name on each
[MemberCondition]attribute.MemberNotFoundRuleMemberNotPublicRulepublic.MemberNotStaticRulestatic.MemberWrongKindRuleMemberWrongReturnTypeRulebool.MethodHasParametersRulePropertyNotReadableRuleAll seven diagnostics share id
MSTEST0070with different message formats, matching howMSTEST0018(DynamicData) is structured.Example
Coverage
AttributeUsage(Class | Method)).(Type, string)ctor, theparams string[]overload, and theConditionMode-prefixed overloads.[MemberCondition]attributes are each validated independently.BaseTypechain), matchingBindingFlags.Public | Static | FlattenHierarchy.property → field → method), so if a property and method share a name only the property is validated — consistent with what would actually be invoked.Tests
18 tests in
test/UnitTests/MSTest.Analyzers.UnitTests/MemberConditionShouldBeValidAnalyzerTests.cs:[MemberCondition]attributes on one method (each validated),ConditionMode.Excludector overload, attribute on class targetAll 18 pass on
net8.0(and the project''s full 1244-test suite stays green).Files touched
src/Analyzers/MSTest.Analyzers/MemberConditionShouldBeValidAnalyzer.cstest/UnitTests/MSTest.Analyzers.UnitTests/MemberConditionShouldBeValidAnalyzerTests.cssrc/Analyzers/MSTest.Analyzers/Helpers/DiagnosticIds.cs(addedMemberConditionShouldBeValidRuleId = "MSTEST0070")src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs(addedMemberConditionAttributeentry, sorted alphabetically)src/Analyzers/MSTest.Analyzers/Resources.resx+ 13.xlffilessrc/Analyzers/MSTest.Analyzers/AnalyzerReleases.Unshipped.mdNotes for reviewers
Warning, enabled by default — matchesMSTEST0018(DynamicData) which solves the same class of problem. Happy to downgrade toInfoif you''d prefer a softer rollout.https://learn.microsoft.com/dotnet/core/testing/mstest-analyzers/mstest0070will be authored as a separatedotnet/docsPR after this merges.DynamicDataShouldBeValidAnalyzer.