From 8f7dffeb552976417fecf50e25f2e1cd0b20ec9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Mon, 13 Jul 2026 14:33:05 +0200 Subject: [PATCH 01/15] Update version prefixes to 4.3.3 and 2.3.3 (#9903) --- eng/Versions.props | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/eng/Versions.props b/eng/Versions.props index 11284a3738..cea9f3d2d7 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -1,9 +1,9 @@ - 4.3.2 + 4.3.3 - 2.3.2 + 2.3.3 preview From 3c8bdb97ae3ba84bb747d78cfd8c3df8b953b1fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Tue, 14 Jul 2026 17:10:10 +0200 Subject: [PATCH 02/15] [rel/4.3] Restore InternalsVisibleTo for deprecated MSTest.Engine to fix MethodAccessException (#9769) (#9939) --- .../Microsoft.Testing.Platform.csproj | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj b/src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj index 3fd6572bc4..2d247d3102 100644 --- a/src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj +++ b/src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj @@ -63,10 +63,22 @@ This package provides the core platform and the .NET implementation of the proto + + + From 509cbe9c9db05f99671799794e158e4dd93c2319 Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Wed, 15 Jul 2026 20:10:01 +0200 Subject: [PATCH 03/15] Fix MSTEST0037 incorrectly rewriting non-generic IDictionary.Contains by @Evangelink in #9968 (backport to rel/4.3) (#9979) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé --- .../Helpers/WellKnownTypeNames.cs | 3 +- ...eProperAssertMethodsAnalyzer.Collection.cs | 53 +++++++++++++++++++ ...operAssertMethodsAnalyzer.IsTrueIsFalse.cs | 4 +- .../UseProperAssertMethodsAnalyzer.cs | 9 ++-- .../UseProperAssertMethodsAnalyzerTests.cs | 32 +++++++++++ 5 files changed, 94 insertions(+), 7 deletions(-) diff --git a/src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs b/src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs index 1336545d01..f6ba7d7089 100644 --- a/src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs +++ b/src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs @@ -46,8 +46,8 @@ internal static class WellKnownTypeNames public const string MicrosoftVisualStudioTestToolsUnitTestingWorkItemAttribute = "Microsoft.VisualStudio.TestTools.UnitTesting.WorkItemAttribute"; public const string System = "System"; - public const string SystemRuntimeInteropServicesRuntimeInformation = "System.Runtime.InteropServices.RuntimeInformation"; public const string SystemCollectionsGenericIEnumerable1 = "System.Collections.Generic.IEnumerable`1"; + public const string SystemCollectionsIDictionary = "System.Collections.IDictionary"; public const string SystemDescriptionAttribute = "System.ComponentModel.DescriptionAttribute"; public const string SystemFunc1 = "System.Func`1"; public const string SystemIAsyncDisposable = "System.IAsyncDisposable"; @@ -58,6 +58,7 @@ internal static class WellKnownTypeNames public const string SystemReflectionMethodInfo = "System.Reflection.MethodInfo"; public const string SystemRuntimeCompilerServicesCallerFilePathAttribute = "System.Runtime.CompilerServices.CallerFilePathAttribute"; public const string SystemRuntimeCompilerServicesCallerLineNumberAttribute = "System.Runtime.CompilerServices.CallerLineNumberAttribute"; + public const string SystemRuntimeInteropServicesRuntimeInformation = "System.Runtime.InteropServices.RuntimeInformation"; public const string SystemThreadingCancellationToken = "System.Threading.CancellationToken"; public const string SystemThreadingCancellationTokenSource = "System.Threading.CancellationTokenSource"; public const string SystemThreadingTasksTask = "System.Threading.Tasks.Task"; diff --git a/src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.Collection.cs b/src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.Collection.cs index b5be1ed7aa..cc5ced3ffc 100644 --- a/src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.Collection.cs +++ b/src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.Collection.cs @@ -37,6 +37,47 @@ private static bool IsBCLCollectionType(ITypeSymbol type, INamedTypeSymbol objec i.OriginalDefinition.SpecialType == SpecialType.System_Collections_IEnumerable) && IsBCLSymbol(type, objectTypeSymbol); + /// + /// Returns when the invoked Contains method is the non-generic + /// (or an implementation of it). + /// That method checks for a matching key, whereas Assert.Contains enumerates the + /// dictionary (yielding items), so the two are not + /// equivalent and the code fix would silently change behavior. + /// + private static bool IsNonGenericDictionaryContains(IMethodSymbol containsMethod, INamedTypeSymbol? iDictionaryTypeSymbol) + { + if (iDictionaryTypeSymbol is null) + { + return false; + } + + INamedTypeSymbol containingType = containsMethod.ContainingType; + + // Direct call through the interface, e.g. 'IDictionary dict; dict.Contains(key)'. + if (SymbolEqualityComparer.Default.Equals(containingType.OriginalDefinition, iDictionaryTypeSymbol)) + { + return true; + } + + // Call through a concrete type that implements IDictionary (e.g. Hashtable), where the invoked + // 'Contains' is the implementation of 'IDictionary.Contains'. + if (!containingType.AllInterfaces.Any(i => SymbolEqualityComparer.Default.Equals(i.OriginalDefinition, iDictionaryTypeSymbol))) + { + return false; + } + + foreach (IMethodSymbol dictionaryContains in iDictionaryTypeSymbol.GetMembers("Contains").OfType()) + { + if (containingType.FindImplementationForInterfaceMember(dictionaryContains) is IMethodSymbol implementation && + SymbolEqualityComparer.Default.Equals(implementation.OriginalDefinition, containsMethod.OriginalDefinition)) + { + return true; + } + } + + return false; + } + /// /// Returns when is one of , /// , or . @@ -69,6 +110,7 @@ private static CollectionCheckStatus RecognizeCollectionMethodCheck( IOperation operation, INamedTypeSymbol objectTypeSymbol, INamedTypeSymbol? enumerableTypeSymbol, + INamedTypeSymbol? iDictionaryTypeSymbol, out SyntaxNode? collectionExpression, out SyntaxNode? itemExpression, out SyntaxNode? comparerExpression) @@ -89,6 +131,17 @@ private static CollectionCheckStatus RecognizeCollectionMethodCheck( ITypeSymbol? enumerableElementType = invocation.TargetMethod.ContainingType.OriginalDefinition.AllInterfaces.FirstOrDefault( i => i.OriginalDefinition.SpecialType == SpecialType.System_Collections_Generic_IEnumerable_T)?.TypeArguments[0]; + if (enumerableElementType is null && IsNonGenericDictionaryContains(invocation.TargetMethod, iDictionaryTypeSymbol)) + { + // Non-generic 'System.Collections.IDictionary.Contains(object key)' checks for a matching *key*, + // whereas 'Assert.Contains' enumerates the dictionary (yielding 'DictionaryEntry' items). + // These have different semantics, so suggesting 'Assert.Contains' here would change behavior. + collectionExpression = null; + itemExpression = null; + comparerExpression = null; + return CollectionCheckStatus.Unknown; + } + if (enumerableElementType is null || enumerableElementType.Equals(containsParameterType, SymbolEqualityComparer.Default)) { // If enumerableElementType is null, we expect that this is a non-generic IEnumerable. So we simply report the diagnostic. diff --git a/src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.IsTrueIsFalse.cs b/src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.IsTrueIsFalse.cs index 70ec333b30..eca644dc2f 100644 --- a/src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.IsTrueIsFalse.cs +++ b/src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.IsTrueIsFalse.cs @@ -16,7 +16,7 @@ namespace MSTest.Analyzers; public sealed partial class UseProperAssertMethodsAnalyzer { - private static void AnalyzeIsTrueOrIsFalseInvocation(OperationAnalysisContext context, IOperation conditionArgument, bool isTrueInvocation, INamedTypeSymbol objectTypeSymbol, INamedTypeSymbol? enumerableTypeSymbol, INamedTypeSymbol? iComparableOfTSymbol) + private static void AnalyzeIsTrueOrIsFalseInvocation(OperationAnalysisContext context, IOperation conditionArgument, bool isTrueInvocation, INamedTypeSymbol objectTypeSymbol, INamedTypeSymbol? enumerableTypeSymbol, INamedTypeSymbol? iComparableOfTSymbol, INamedTypeSymbol? iDictionaryTypeSymbol) { RoslynDebug.Assert(context.Operation is IInvocationOperation, "Expected IInvocationOperation."); @@ -107,7 +107,7 @@ private static void AnalyzeIsTrueOrIsFalseInvocation(OperationAnalysisContext co } // Check for collection method patterns: myCollection.Contains(...) - CollectionCheckStatus collectionMethodStatus = RecognizeCollectionMethodCheck(conditionArgument, objectTypeSymbol, enumerableTypeSymbol, out SyntaxNode? collectionExpr, out SyntaxNode? itemExpr, out SyntaxNode? comparerExpr); + CollectionCheckStatus collectionMethodStatus = RecognizeCollectionMethodCheck(conditionArgument, objectTypeSymbol, enumerableTypeSymbol, iDictionaryTypeSymbol, out SyntaxNode? collectionExpr, out SyntaxNode? itemExpr, out SyntaxNode? comparerExpr); if (collectionMethodStatus != CollectionCheckStatus.Unknown) { if (collectionMethodStatus == CollectionCheckStatus.Contains) diff --git a/src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.cs b/src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.cs index c3c0853530..2ddd409e83 100644 --- a/src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.cs +++ b/src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.cs @@ -262,12 +262,13 @@ public override void Initialize(AnalysisContext context) INamedTypeSymbol objectTypeSymbol = context.Compilation.GetSpecialType(SpecialType.System_Object); context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemLinqEnumerable, out INamedTypeSymbol? enumerableTypeSymbol); INamedTypeSymbol? iComparableOfTSymbol = context.Compilation.GetTypeByMetadataName("System.IComparable`1"); + context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsIDictionary, out INamedTypeSymbol? iDictionaryTypeSymbol); - context.RegisterOperationAction(context => AnalyzeInvocationOperation(context, assertTypeSymbol, objectTypeSymbol, enumerableTypeSymbol, iComparableOfTSymbol), OperationKind.Invocation); + context.RegisterOperationAction(context => AnalyzeInvocationOperation(context, assertTypeSymbol, objectTypeSymbol, enumerableTypeSymbol, iComparableOfTSymbol, iDictionaryTypeSymbol), OperationKind.Invocation); }); } - private static void AnalyzeInvocationOperation(OperationAnalysisContext context, INamedTypeSymbol assertTypeSymbol, INamedTypeSymbol objectTypeSymbol, INamedTypeSymbol? enumerableTypeSymbol, INamedTypeSymbol? iComparableOfTSymbol) + private static void AnalyzeInvocationOperation(OperationAnalysisContext context, INamedTypeSymbol assertTypeSymbol, INamedTypeSymbol objectTypeSymbol, INamedTypeSymbol? enumerableTypeSymbol, INamedTypeSymbol? iComparableOfTSymbol, INamedTypeSymbol? iDictionaryTypeSymbol) { var operation = (IInvocationOperation)context.Operation; IMethodSymbol targetMethod = operation.TargetMethod; @@ -284,11 +285,11 @@ private static void AnalyzeInvocationOperation(OperationAnalysisContext context, switch (targetMethod.Name) { case "IsTrue": - AnalyzeIsTrueOrIsFalseInvocation(context, firstArgument, isTrueInvocation: true, objectTypeSymbol, enumerableTypeSymbol, iComparableOfTSymbol); + AnalyzeIsTrueOrIsFalseInvocation(context, firstArgument, isTrueInvocation: true, objectTypeSymbol, enumerableTypeSymbol, iComparableOfTSymbol, iDictionaryTypeSymbol); break; case "IsFalse": - AnalyzeIsTrueOrIsFalseInvocation(context, firstArgument, isTrueInvocation: false, objectTypeSymbol, enumerableTypeSymbol, iComparableOfTSymbol); + AnalyzeIsTrueOrIsFalseInvocation(context, firstArgument, isTrueInvocation: false, objectTypeSymbol, enumerableTypeSymbol, iComparableOfTSymbol, iDictionaryTypeSymbol); break; case "AreEqual": diff --git a/test/UnitTests/MSTest.Analyzers.UnitTests/UseProperAssertMethodsAnalyzerTests.cs b/test/UnitTests/MSTest.Analyzers.UnitTests/UseProperAssertMethodsAnalyzerTests.cs index 257e36c083..e1e6782089 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/UseProperAssertMethodsAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/UseProperAssertMethodsAnalyzerTests.cs @@ -1395,6 +1395,38 @@ protected override int GetKeyForItem(int item) await VerifyCS.VerifyCodeFixAsync(code, code); } + [TestMethod] + public async Task WhenAssertIsTrueOrIsFalseWithNonGenericIDictionaryContains_NoDiagnostic() + { + // 'System.Collections.IDictionary.Contains(object)' checks for a matching *key*, whereas + // 'Assert.Contains' enumerates the dictionary (yielding 'DictionaryEntry' items). These have + // different semantics, so the analyzer must not suggest 'Assert.Contains' here. + // See https://github.com/microsoft/testfx/issues/9966. + string code = """ + using System.Collections; + using System.Collections.Generic; + + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTests + { + [TestMethod] + public void Contains() + { + IDictionary dict = new Dictionary() { { "a", "b" } }; + Assert.IsTrue(dict.Contains("a")); + Assert.IsFalse(dict.Contains("a")); + + Hashtable hashtable = new Hashtable(); + Assert.IsTrue(hashtable.Contains("a")); + Assert.IsFalse(hashtable.Contains("a")); + } + } + """; + await VerifyCS.VerifyCodeFixAsync(code, code); + } + #region New test cases for string methods [TestMethod] From be18ca169963b0cc6c2f8137c4a0576ce7554189 Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Thu, 16 Jul 2026 14:15:11 +0200 Subject: [PATCH 04/15] Don't report MSTEST0065 on collection types that declare their own equality by @Evangelink in #9978 (backport to rel/4.3) (#10003) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...voidAssertAreEqualOnCollectionsAnalyzer.cs | 174 ++++- .../Helpers/WellKnownTypeNames.cs | 2 + ...ssertAreEqualOnCollectionsAnalyzerTests.cs | 738 ++++++++++++++++++ 3 files changed, 912 insertions(+), 2 deletions(-) diff --git a/src/Analyzers/MSTest.Analyzers/AvoidAssertAreEqualOnCollectionsAnalyzer.cs b/src/Analyzers/MSTest.Analyzers/AvoidAssertAreEqualOnCollectionsAnalyzer.cs index fddde8e247..abf8dc94d8 100644 --- a/src/Analyzers/MSTest.Analyzers/AvoidAssertAreEqualOnCollectionsAnalyzer.cs +++ b/src/Analyzers/MSTest.Analyzers/AvoidAssertAreEqualOnCollectionsAnalyzer.cs @@ -53,11 +53,16 @@ public override void Initialize(AnalysisContext context) return; } - context.RegisterOperationAction(context => AnalyzeInvocation(context, assertSymbol, genericEnumerableSymbol), OperationKind.Invocation); + // May be null on very old target frameworks; when it is, only the IEquatable<self> check becomes a + // no-op — the object.Equals override detection still runs. + INamedTypeSymbol? equatableSymbol = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemIEquatable1); + INamedTypeSymbol? equalityComparerSymbol = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsGenericEqualityComparer1); + + context.RegisterOperationAction(context => AnalyzeInvocation(context, assertSymbol, genericEnumerableSymbol, equatableSymbol, equalityComparerSymbol), OperationKind.Invocation); }); } - private static void AnalyzeInvocation(OperationAnalysisContext context, INamedTypeSymbol assertSymbol, INamedTypeSymbol genericEnumerableSymbol) + private static void AnalyzeInvocation(OperationAnalysisContext context, INamedTypeSymbol assertSymbol, INamedTypeSymbol genericEnumerableSymbol, INamedTypeSymbol? equatableSymbol, INamedTypeSymbol? equalityComparerSymbol) { var invocation = (IInvocationOperation)context.Operation; IMethodSymbol targetMethod = invocation.TargetMethod; @@ -89,6 +94,23 @@ targetMethod.Name is not ("AreEqual" or "AreNotEqual") || // patterns where the caller widened to a non-collection type (e.g. `Assert.AreEqual(arr1, arr2)` // or `Assert.AreEqual((object)arr1, (object)arr2)`) which would otherwise silently use reference equality. ITypeSymbol comparedType = targetMethod.TypeArguments[0]; + + // Opt out when the compared type declares its own equality (implements IEquatable<itself> or overrides + // object.Equals). The author has then deliberately chosen a custom, non-reference equality that Assert.AreEqual + // honors via EqualityComparer<T>.Default, so suggesting a sequence/structural comparison would second-guess + // a deliberate decision (see issue #9971). Two things are important here: + // * The check must be based on the *selected generic type argument* T, not the argument's runtime collection + // type. Widening to a base type (e.g. Assert.AreEqual<object>(collection, collection)) discards the + // collection's IEquatable<self> and falls back to reference equality — exactly the footgun the rule targets. + // * It must not apply when the caller supplies a *custom* comparer, because then EqualityComparer<T>.Default + // (and therefore the type's own equality) is not used at all. A `null` comparer or an explicit + // `EqualityComparer<T>.Default` argument is equivalent to the parameterless overload (Assert falls back to + // the default comparer — see Assert.AreEqual.cs), so those keep the opt-out. + if (!HasCustomComparerArgument(invocation, comparedType, equalityComparerSymbol) && DeclaresOwnEquality(comparedType, equatableSymbol)) + { + return; + } + ITypeSymbol? reportedType = ShouldReport(comparedType, genericEnumerableSymbol) ? comparedType : GetCollectionArgumentType(invocation, firstParameterName, genericEnumerableSymbol) @@ -104,6 +126,45 @@ targetMethod.Name is not ("AreEqual" or "AreNotEqual") || context.ReportDiagnostic(invocation.CreateDiagnostic(Rule, methodName, comparedTypeDisplay)); } + // Returns true only when a `comparer` argument is supplied that is not provably the default comparer for the + // selected type argument T. `null` and `EqualityComparer.Default` both dispatch to EqualityComparer.Default at + // runtime, so they must keep the equality opt-out; any other comparer bypasses the type's own equality and should + // re-enable the diagnostic. + private static bool HasCustomComparerArgument(IInvocationOperation invocation, ITypeSymbol comparedType, INamedTypeSymbol? equalityComparerSymbol) + { + foreach (IArgumentOperation argument in invocation.Arguments) + { + if (argument.Parameter?.Name != "comparer") + { + continue; + } + + // Only peel built-in conversions (identity, implicit reference, boxing). A user-defined conversion actually + // executes and can turn a null source into a custom comparer, so it must be treated as opaque — otherwise a + // null underneath a user-defined operator would be misclassified as the default comparer. + IOperation value = argument.Value.WalkDownBuiltInConversion(); + + // Any constant-null comparer (null literal, `default`, `default(IEqualityComparer)`, a const null field, …) + // falls back to EqualityComparer.Default in Assert, and a reference to `EqualityComparer.Default` is that + // same default comparer — neither bypasses the type's own equality. + // + // The reference must be `EqualityComparer.Default` for the *selected* T: `IEqualityComparer` is + // contravariant, so `Assert.AreEqual(d1, d2, EqualityComparer.Default)` compiles, but that base + // comparer dispatches to Base's equality — not Derived's — so it is a custom comparer for T = Derived. + bool isDefaultComparer = + value.ConstantValue is { HasValue: true, Value: null } + || (value is IPropertyReferenceOperation { Property: { Name: "Default", IsStatic: true } property } + && equalityComparerSymbol is not null + && SymbolEqualityComparer.Default.Equals(property.ContainingType.OriginalDefinition, equalityComparerSymbol) + && property.ContainingType.TypeArguments.Length == 1 + && SymbolEqualityComparer.Default.Equals(property.ContainingType.TypeArguments[0], comparedType)); + + return !isDefaultComparer; + } + + return false; + } + private static ITypeSymbol? GetCollectionArgumentType(IInvocationOperation invocation, string parameterName, INamedTypeSymbol genericEnumerableSymbol) { IArgumentOperation? argument = invocation.Arguments.FirstOrDefault(arg => arg.Parameter?.Name == parameterName); @@ -122,6 +183,115 @@ private static bool ShouldReport(ITypeSymbol comparedType, INamedTypeSymbol gene => comparedType.SpecialType != SpecialType.System_String && ImplementsGenericEnumerable(comparedType, genericEnumerableSymbol); + private static bool DeclaresOwnEquality(ITypeSymbol type, INamedTypeSymbol? equatableSymbol) + => type switch + { + INamedTypeSymbol namedType => ImplementsSelfEquatable(namedType, namedType, equatableSymbol) || OverridesObjectEquals(namedType), + + // The equality target is the type parameter T itself (the type whose EqualityComparer.Default is used). + ITypeParameterSymbol typeParameter => TypeParameterDeclaresOwnEquality(typeParameter, typeParameter, equatableSymbol), + + _ => false, + }; + + // EqualityComparer<T>.Default honors a `where T : IEquatable` constraint, and a class constraint that overrides + // object.Equals is inherited by every T. A bare `where T : IEnumerable<...>` constraint has neither, so it stays the + // reference-equality footgun we still want to flag. + // + // `equalityTarget` stays the original type parameter T while we traverse constraints, including transitive + // type-parameter constraints (`where T : U where U : ...`). The IEquatable check must be against T: for + // `where T : ISelf` with `ISelf : IEquatable`, a concrete T need not implement IEquatable<T>, so it would + // still use reference equality. An object.Equals override on any reachable class constraint is inherited by T and so + // is honored regardless of the equality target. + private static bool TypeParameterDeclaresOwnEquality(ITypeParameterSymbol typeParameter, ITypeSymbol equalityTarget, INamedTypeSymbol? equatableSymbol) + { + foreach (ITypeSymbol constraintType in typeParameter.ConstraintTypes) + { + switch (constraintType) + { + case INamedTypeSymbol namedConstraint when ImplementsSelfEquatable(namedConstraint, equalityTarget, equatableSymbol) || OverridesObjectEquals(namedConstraint): + return true; + + case ITypeParameterSymbol nestedTypeParameter when TypeParameterDeclaresOwnEquality(nestedTypeParameter, equalityTarget, equatableSymbol): + return true; + } + } + + return false; + } + + // Returns true when `candidate` is or implements IEquatable<equalityType>, i.e. the equality contract that + // EqualityComparer<equalityType>.Default would dispatch to. `equalityType` is the type whose default comparer + // is used (the compared type or the type parameter); `candidate` is the type (or constraint) we inspect. + private static bool ImplementsSelfEquatable(INamedTypeSymbol candidate, ITypeSymbol equalityType, INamedTypeSymbol? equatableSymbol) + { + if (equatableSymbol is null) + { + return false; + } + + // The candidate can be the IEquatable<T> interface itself (e.g. a `where T : IEquatable` constraint) + // or a type that lists it among its implemented interfaces (e.g. `class C : IEquatable`). + if (IsEquatableOf(candidate, equalityType, equatableSymbol)) + { + return true; + } + + foreach (INamedTypeSymbol implemented in candidate.AllInterfaces) + { + if (IsEquatableOf(implemented, equalityType, equatableSymbol)) + { + return true; + } + } + + return false; + } + + private static bool IsEquatableOf(INamedTypeSymbol type, ITypeSymbol equalityType, INamedTypeSymbol equatableSymbol) + => SymbolEqualityComparer.Default.Equals(type.OriginalDefinition, equatableSymbol) + && type.TypeArguments.Length == 1 + && SymbolEqualityComparer.Default.Equals(type.TypeArguments[0], equalityType); + + private static bool OverridesObjectEquals(INamedTypeSymbol type) + { + // Stop before object AND ValueType: System.ValueType overrides object.Equals, so walking into it would treat + // every struct that implements IEnumerable as declaring its own equality even when it has no Equals of its + // own (its backing array/list field would then be compared by reference). An explicit struct override is found + // on the concrete type before we reach ValueType. + for (INamedTypeSymbol? current = type; + current is not null && current.SpecialType is not (SpecialType.System_Object or SpecialType.System_ValueType or SpecialType.System_Enum); + current = current.BaseType) + { + foreach (ISymbol member in current.GetMembers(nameof(Equals))) + { + if (member is IMethodSymbol { IsOverride: true, Parameters.Length: 1, ReturnType.SpecialType: SpecialType.System_Boolean } method && + method.Parameters[0].Type.SpecialType == SpecialType.System_Object && + OverrideRootIsObjectEquals(method)) + { + return true; + } + } + } + + return false; + } + + // `IsOverride` only proves the method overrides *some* virtual slot. A base class can declare + // `new virtual bool Equals(object)`, and a derived override of that member is not an override of + // object.Equals — EqualityComparer<T>.Default still dispatches the unchanged object.Equals slot + // (reference equality). Follow the override chain to its root and require it to be object.Equals. + private static bool OverrideRootIsObjectEquals(IMethodSymbol method) + { + IMethodSymbol root = method; + while (root.OverriddenMethod is { } overridden) + { + root = overridden; + } + + return root.ContainingType?.SpecialType == SpecialType.System_Object; + } + private static bool HasNullLiteralArgument(IInvocationOperation invocation, string parameterName) { IArgumentOperation? argument = invocation.Arguments.FirstOrDefault(arg => arg.Parameter?.Name == parameterName); diff --git a/src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs b/src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs index f6ba7d7089..2f7ed7ce55 100644 --- a/src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs +++ b/src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs @@ -46,12 +46,14 @@ internal static class WellKnownTypeNames public const string MicrosoftVisualStudioTestToolsUnitTestingWorkItemAttribute = "Microsoft.VisualStudio.TestTools.UnitTesting.WorkItemAttribute"; public const string System = "System"; + public const string SystemCollectionsGenericEqualityComparer1 = "System.Collections.Generic.EqualityComparer`1"; public const string SystemCollectionsGenericIEnumerable1 = "System.Collections.Generic.IEnumerable`1"; public const string SystemCollectionsIDictionary = "System.Collections.IDictionary"; public const string SystemDescriptionAttribute = "System.ComponentModel.DescriptionAttribute"; public const string SystemFunc1 = "System.Func`1"; public const string SystemIAsyncDisposable = "System.IAsyncDisposable"; public const string SystemIDisposable = "System.IDisposable"; + public const string SystemIEquatable1 = "System.IEquatable`1"; public const string SystemLinqEnumerable = "System.Linq.Enumerable"; public const string SystemLinqExpressionsExpression1 = "System.Linq.Expressions.Expression`1"; public const string SystemOperatingSystem = "System.OperatingSystem"; diff --git a/test/UnitTests/MSTest.Analyzers.UnitTests/AvoidAssertAreEqualOnCollectionsAnalyzerTests.cs b/test/UnitTests/MSTest.Analyzers.UnitTests/AvoidAssertAreEqualOnCollectionsAnalyzerTests.cs index ca2397781a..e66c872397 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/AvoidAssertAreEqualOnCollectionsAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/AvoidAssertAreEqualOnCollectionsAnalyzerTests.cs @@ -826,6 +826,744 @@ public sealed class Wrapper await VerifyCS.VerifyAnalyzerAsync(code); } + [TestMethod] + public async Task WhenUsingAssertAreEqualOnCollectionImplementingIEquatable_DoNotReportDiagnostic() + { + // The type is a collection but declares its own equality via IEquatable, so Assert.AreEqual + // honors that intentional equality. Suggesting a sequence comparison would second-guess the author (issue #9971). + string code = """ + #nullable enable + using System; + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = new(); + MyCollection c2 = new(); + Assert.AreEqual(c1, c2); + } + + private sealed class MyCollection : IEnumerable, IEquatable + { + public bool Equals(MyCollection? other) => true; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code); + } + + [TestMethod] + public async Task WhenUsingAssertAreNotEqualOnCollectionImplementingIEquatable_DoNotReportDiagnostic() + { + string code = """ + #nullable enable + using System; + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = new(); + MyCollection c2 = new(); + Assert.AreNotEqual(c1, c2); + } + + private sealed class MyCollection : IEnumerable, IEquatable + { + public bool Equals(MyCollection? other) => true; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnCollectionOverridingObjectEquals_DoNotReportDiagnostic() + { + // Overriding object.Equals is the same intentional-equality signal as IEquatable. + string code = """ + #nullable enable + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = new(); + MyCollection c2 = new(); + Assert.AreEqual(c1, c2); + } + + private sealed class MyCollection : IEnumerable + { + public override bool Equals(object? obj) => obj is MyCollection; + + public override int GetHashCode() => 0; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnCollectionImplementingIEquatableOfOtherType_ReportDiagnostic() + { + // IEquatable is not used by EqualityComparer.Default, so the type still + // falls back to reference equality — the footgun the rule targets. + string code = """ + #nullable enable + using System; + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = new(); + MyCollection c2 = new(); + {|#0:Assert.AreEqual(c1, c2)|}; + } + + private sealed class MyCollection : IEnumerable, IEquatable + { + public bool Equals(string? other) => false; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code, ExpectedDiagnostic("Assert.AreEqual", "MyCollection")); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnCollectionImplementingIEquatableButWidenedToObject_ReportDiagnostic() + { + // The collection declares its own equality via IEquatable, but the call is widened to object. + // EqualityComparer.Default ignores IEquatable and uses reference equality, so this + // is still the footgun the rule targets. The opt-out must key off the selected generic type argument. + string code = """ + #nullable enable + using System; + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = new(); + MyCollection c2 = new(); + {|#0:Assert.AreEqual(c1, c2)|}; + } + + private sealed class MyCollection : IEnumerable, IEquatable + { + public bool Equals(MyCollection? other) => true; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code, ExpectedDiagnostic("Assert.AreEqual", "MyCollection")); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnCollectionImplementingIEquatableWithCustomComparer_ReportDiagnostic() + { + // A genuinely custom comparer bypasses the type's own IEquatable, so the opt-out must not apply and + // MSTEST0065 should still fire. + string code = """ + #nullable enable + using System; + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = new(); + MyCollection c2 = new(); + {|#0:Assert.AreEqual(c1, c2, new CustomComparer())|}; + } + + private sealed class CustomComparer : IEqualityComparer + { + public bool Equals(MyCollection? x, MyCollection? y) => true; + + public int GetHashCode(MyCollection obj) => 0; + } + + private sealed class MyCollection : IEnumerable, IEquatable + { + public bool Equals(MyCollection? other) => true; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code, ExpectedDiagnostic("Assert.AreEqual", "MyCollection")); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnCollectionImplementingIEquatableWithExplicitDefaultComparer_DoNotReportDiagnostic() + { + // `EqualityComparer.Default` dispatches to the type's own IEquatable, so it is equivalent + // to the parameterless overload and keeps the opt-out. + string code = """ + #nullable enable + using System; + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = new(); + MyCollection c2 = new(); + Assert.AreEqual(c1, c2, EqualityComparer.Default); + } + + private sealed class MyCollection : IEnumerable, IEquatable + { + public bool Equals(MyCollection? other) => true; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnCollectionWithBaseTypeDefaultComparer_ReportDiagnostic() + { + // IEqualityComparer is contravariant, so EqualityComparer.Default compiles as the comparer for a + // Derived argument. But it dispatches to Base's equality, not Derived's IEquatable, so it is a custom + // comparer for T = Derived and MSTEST0065 must still fire. + string code = """ + #nullable enable + using System; + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + Derived c1 = new(); + Derived c2 = new(); + {|#0:Assert.AreEqual(c1, c2, EqualityComparer.Default)|}; + } + + private class Base + { + } + + private sealed class Derived : Base, IEnumerable, IEquatable + { + public bool Equals(Derived? other) => true; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code, ExpectedDiagnostic("Assert.AreEqual", "Derived")); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnCollectionImplementingIEquatableWithNullComparer_DoNotReportDiagnostic() + { + // A null comparer is replaced with EqualityComparer.Default by Assert, so it is equivalent to the + // parameterless overload and keeps the opt-out. + string code = """ + #nullable enable + using System; + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = new(); + MyCollection c2 = new(); + Assert.AreEqual(c1, c2, (IEqualityComparer)null!); + } + + private sealed class MyCollection : IEnumerable, IEquatable + { + public bool Equals(MyCollection? other) => true; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnCollectionImplementingIEquatableWithDefaultComparerExpression_DoNotReportDiagnostic() + { + // `default(IEqualityComparer)` constant-folds to null, which Assert replaces with + // EqualityComparer.Default, so it keeps the opt-out just like a null literal. + string code = """ + #nullable enable + using System; + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = new(); + MyCollection c2 = new(); + Assert.AreEqual(c1, c2, default(IEqualityComparer)!); + } + + private sealed class MyCollection : IEnumerable, IEquatable + { + public bool Equals(MyCollection? other) => true; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnTypeParameterConstrainedToIEquatable_DoNotReportDiagnostic() + { + // `where T : IEnumerable, IEquatable` guarantees EqualityComparer.Default honors IEquatable, + // so the comparison uses the constrained equality, not reference equality. + string code = """ + using System; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod(T a, T b) where T : IEnumerable, IEquatable + { + Assert.AreEqual(a, b); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnCollectionOverridingNewVirtualEquals_ReportDiagnostic() + { + // The base declares `new virtual bool Equals(object)` (a different slot from object.Equals), and the + // collection overrides that. EqualityComparer.Default still dispatches the unchanged + // object.Equals slot (reference equality), so this must NOT be treated as declaring its own equality. + string code = """ + #nullable enable + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = new(); + MyCollection c2 = new(); + {|#0:Assert.AreEqual(c1, c2)|}; + } + + private class Base + { + public new virtual bool Equals(object? obj) => true; + } + + private sealed class MyCollection : Base, IEnumerable + { + public override bool Equals(object? obj) => true; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code, ExpectedDiagnostic("Assert.AreEqual", "MyCollection")); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnStructCollectionWithoutOwnEquality_ReportDiagnostic() + { + // A struct implementing IEnumerable inherits ValueType.Equals but declares no equality of its own, + // so EqualityComparer.Default falls back to ValueType's field-wise (reflection-based) comparison rather + // than any intentional equality. The walk must stop before System.ValueType so this is still reported. + string code = """ + #nullable enable + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = default; + MyCollection c2 = default; + {|#0:Assert.AreEqual(c1, c2)|}; + } + + private struct MyCollection : IEnumerable + { + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code, ExpectedDiagnostic("Assert.AreEqual", "MyCollection")); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnStructCollectionOverridingObjectEquals_DoNotReportDiagnostic() + { + // A struct that explicitly overrides object.Equals is found on the concrete type before ValueType, so its + // intentional equality is honored and MSTEST0065 is suppressed. + string code = """ + #nullable enable + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = default; + MyCollection c2 = default; + Assert.AreEqual(c1, c2); + } + + private struct MyCollection : IEnumerable + { + public override bool Equals(object? obj) => true; + + public override int GetHashCode() => 0; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnTypeParameterConstrainedToSelfEquatingType_ReportDiagnostic() + { + // `where T : ISelf` with `ISelf : IEquatable` does NOT guarantee T implements IEquatable: + // a concrete T is only required to be assignable to ISelf. EqualityComparer.Default therefore may still + // use reference equality, so the diagnostic must be preserved (the constraint's own IEquatable is not T's). + string code = """ + using System; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod(T a, T b) where T : ISelf + { + {|#0:Assert.AreEqual(a, b)|}; + } + + public interface ISelf : IEnumerable, IEquatable + { + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code, ExpectedDiagnostic("Assert.AreEqual", "T")); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnTypeParameterConstrainedToClassOverridingObjectEquals_DoNotReportDiagnostic() + { + // A class constraint that overrides object.Equals is inherited by every T, so EqualityComparer.Default + // uses that intentional equality. + string code = """ + #nullable enable + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod(T a, T b) where T : BaseCollection + { + Assert.AreEqual(a, b); + } + + public class BaseCollection : IEnumerable + { + public override bool Equals(object? obj) => true; + + public override int GetHashCode() => 0; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnTypeParameterWithTransitiveClassConstraintOverridingObjectEquals_DoNotReportDiagnostic() + { + // Transitive type-parameter constraint: `where T : U where U : BaseCollection`. Every concrete T derives from + // BaseCollection and inherits its object.Equals override, so EqualityComparer.Default uses that equality. + // The constraint traversal must follow the nested type parameter U to reach BaseCollection. + string code = """ + #nullable enable + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod(T a, T b) where T : U where U : BaseCollection + { + Assert.AreEqual(a, b); + } + + public class BaseCollection : IEnumerable + { + public override bool Equals(object? obj) => true; + + public override int GetHashCode() => 0; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnCollectionImplementingIEquatableOfBaseType_ReportDiagnostic() + { + // IEquatable is invariant: EqualityComparer.Default requires Derived to implement + // IEquatable exactly. A collection implementing only IEquatable (and not overriding + // object.Equals) still falls back to reference equality, so this must be reported. + string code = """ + #nullable enable + using System; + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + Derived c1 = new(); + Derived c2 = new(); + {|#0:Assert.AreEqual(c1, c2)|}; + } + + private class Base + { + } + + private sealed class Derived : Base, IEnumerable, IEquatable + { + public bool Equals(Base? other) => true; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code, ExpectedDiagnostic("Assert.AreEqual", "Derived")); + } + + [TestMethod] + public async Task WhenUsingAssertAreEqualOnCollectionWithExplicitInterfaceIEquatable_DoNotReportDiagnostic() + { + // Explicit interface implementation of IEquatable is still the equality EqualityComparer.Default + // dispatches to (via AllInterfaces), so the opt-out applies. + string code = """ + #nullable enable + using System; + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = new(); + MyCollection c2 = new(); + Assert.AreEqual(c1, c2); + } + + private sealed class MyCollection : IEnumerable, IEquatable + { + bool IEquatable.Equals(MyCollection? other) => true; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code); + } + + [TestMethod] + public async Task WhenUsingAssertAreNotEqualOnCollectionImplementingIEquatableButWidenedToObject_ReportDiagnostic() + { + // AreNotEqual mirror of the widened-to-object case: EqualityComparer.Default ignores + // IEquatable, so this is still the reference-equality footgun and must be reported. + string code = """ + #nullable enable + using System; + using System.Collections; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void MyTestMethod() + { + MyCollection c1 = new(); + MyCollection c2 = new(); + {|#0:Assert.AreNotEqual(c1, c2)|}; + } + + private sealed class MyCollection : IEnumerable, IEquatable + { + public bool Equals(MyCollection? other) => true; + + public IEnumerator GetEnumerator() => new List().GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(code, ExpectedDiagnostic("Assert.AreNotEqual", "MyCollection")); + } + private static DiagnosticResult ExpectedDiagnostic(string methodName, string typeName) => VerifyCS.Diagnostic().WithLocation(0).WithArguments(methodName, typeName); From 826e8c21ed8c0d7b60669e332ad1581f37358cf7 Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Fri, 17 Jul 2026 21:57:56 +0200 Subject: [PATCH 05/15] Don't flag Assert.AreEqual self-comparison for user-defined equality by @Evangelink in #10008 (backport to rel/4.3) (#10043) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé --- .../Helpers/AssertConditionAnalyzerHelper.cs | 135 ++++- ...rtFailOverAlwaysFalseConditionsAnalyzer.cs | 8 +- ...ReviewAlwaysTrueAssertConditionAnalyzer.cs | 8 +- ...lOverAlwaysFalseConditionsAnalyzerTests.cs | 206 +++++++ ...wAlwaysTrueAssertConditionAnalyzerTests.cs | 522 ++++++++++++++++++ 5 files changed, 872 insertions(+), 7 deletions(-) diff --git a/src/Analyzers/MSTest.Analyzers/Helpers/AssertConditionAnalyzerHelper.cs b/src/Analyzers/MSTest.Analyzers/Helpers/AssertConditionAnalyzerHelper.cs index 695caa312e..a013a7f796 100644 --- a/src/Analyzers/MSTest.Analyzers/Helpers/AssertConditionAnalyzerHelper.cs +++ b/src/Analyzers/MSTest.Analyzers/Helpers/AssertConditionAnalyzerHelper.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. using Microsoft.CodeAnalysis; @@ -28,6 +28,139 @@ internal static bool HasIdenticalExpectedAndActual(IInvocationOperation operatio && GetRawArgumentValueWithName(operation, ActualParameterName) is { } actualArgument && expectedArgument.IsEquivalentReferenceTo(actualArgument); + /// + /// Returns when expected/notExpected and actual are the + /// same side-effect-free reference (see ) and a + /// self-comparison of that value using the default equality comparer is provably always equal. + /// + internal static bool HasIdenticalExpectedAndActualWithBuiltInEquality(IInvocationOperation operation, string expectedOrNotExpectedParameterName) + => HasIdenticalExpectedAndActual(operation, expectedOrNotExpectedParameterName) + && IsProvablyReflexiveSelfEquality(GetComparedType(operation, expectedOrNotExpectedParameterName)); + + /// + /// Returns when the invoked Assert overload is passed a non-default + /// argument, in which case the comparison can + /// return any result and must not be treated as an always-true/always-false condition. A + /// comparer is treated by MSTest as , so it + /// does not change the equality semantics and is not considered a custom comparer here. + /// + internal static bool HasNonDefaultEqualityComparerArgument(IInvocationOperation operation) + { + IParameterSymbol? comparerParameter = operation.TargetMethod.Parameters.FirstOrDefault(static parameter => + parameter.Type is INamedTypeSymbol + { + Name: "IEqualityComparer", + ContainingNamespace: { Name: "Generic", ContainingNamespace: { Name: "Collections", ContainingNamespace: { Name: "System", ContainingNamespace.IsGlobalNamespace: true } } }, + }); + + if (comparerParameter is not { Type: INamedTypeSymbol { TypeArguments: [{ } comparerElementType] } }) + { + return false; + } + + // A null (or omitted) comparer, or an explicit EqualityComparer.Default, is equivalent to the default + // comparer and does not change the equality semantics. Any other comparer (including a non-constant one) + // can return an arbitrary result. Strip only built-in conversions so a user-defined conversion cannot hide + // a null source that it turns into a non-null comparer. + IOperation? comparerArgument = GetRawArgumentValueWithName(operation, comparerParameter.Name)?.WalkDownBuiltInConversion(); + return comparerArgument is not null + && comparerArgument.ConstantValue is not { HasValue: true, Value: null } + && !IsDefaultEqualityComparerReference(comparerArgument, comparerElementType); + } + + // EqualityComparer.Default is only the default comparer for T when X is T. IEqualityComparer is + // contravariant, so e.g. EqualityComparer.Default can be passed as IEqualityComparer, in + // which case it is a non-default comparer that may return a different result. + private static bool IsDefaultEqualityComparerReference(IOperation operation, ITypeSymbol comparerElementType) + => operation is IPropertyReferenceOperation { Property: { Name: "Default", ContainingType: INamedTypeSymbol { Name: "EqualityComparer", TypeArguments: [{ } elementType], ContainingNamespace: { Name: "Generic", ContainingNamespace: { Name: "Collections", ContainingNamespace: { Name: "System", ContainingNamespace.IsGlobalNamespace: true } } } } } } + && SymbolEqualityComparer.Default.Equals(elementType, comparerElementType); + + /// + /// Gets the type T whose the + /// invoked Assert.AreEqual/AreNotEqual overload uses to compare the values. For the generic + /// overloads this is the method type argument (not the operand's static type, which may differ through an + /// implicit conversion); for non-generic overloads it is the declared parameter type. + /// + private static ITypeSymbol? GetComparedType(IInvocationOperation operation, string expectedOrNotExpectedParameterName) + => operation.TargetMethod.TypeArguments is [{ } typeArgument] + ? typeArgument + : operation.TargetMethod.Parameters.FirstOrDefault(parameter => parameter.Name == expectedOrNotExpectedParameterName)?.Type; + + /// + /// Returns when a self-comparison of a value of using the + /// default equality comparer is provably always equal. + /// + /// + /// Assert.AreEqual/AreNotEqual compare using , + /// which dispatches to .Equals when the type implements it, otherwise to the + /// virtual . Reflexivity can only be proven conservatively: + /// + /// primitives, , and enums have reflexive built-in equality; + /// arrays and sealed reference types without customized equality use reflexive reference equality; + /// a non-sealed reference type, an interface, or a type parameter can hold an instance whose overridden + /// equality is not reflexive; + /// a non-primitive value type relying on the default field-based ValueType.Equals (or Nullable<T>, + /// which delegates to the underlying type) may compare a field whose equality is not reflexive, and a custom struct + /// override could itself be non-reflexive. + /// + /// + private static bool IsProvablyReflexiveSelfEquality(ITypeSymbol? type) + { + if (type is null || type.TypeKind is TypeKind.TypeParameter) + { + return false; + } + + if (type.IsReferenceType) + { + // Arrays always use reflexive reference equality (they never override Equals). Any other reference + // type is only provable when its runtime type is known exactly (sealed) and it does not customize + // equality with a potentially non-reflexive override. Non-sealed types, interfaces, and object can + // hold a derived instance whose overridden Equals is not reflexive. + return type.TypeKind is TypeKind.Array + || (type.IsSealed && !HasUserDefinedEquality(type)); + } + + // Value types: only primitives (and enums) have provably reflexive built-in equality. Nullable and + // structs relying on the default (or a custom) equality could be non-reflexive. + return type.OriginalDefinition.SpecialType is not SpecialType.System_Nullable_T + && (type.SpecialType is not SpecialType.None || type.TypeKind is TypeKind.Enum); + } + + private static bool HasUserDefinedEquality(ITypeSymbol type) + { + // string overrides Equals but its equality is reflexive; it is handled as a primitive by the caller. + if (type.SpecialType != SpecialType.None) + { + return false; + } + + // The == operator is never consulted by EqualityComparer.Default, so it is intentionally not checked here. + for (ITypeSymbol? current = type; + current is { SpecialType: not SpecialType.System_Object and not SpecialType.System_ValueType }; + current = current.BaseType) + { + foreach (ISymbol member in current.GetMembers(nameof(object.Equals))) + { + if (member is IMethodSymbol { IsOverride: true, Parameters: [{ Type.SpecialType: SpecialType.System_Object }] }) + { + return true; + } + } + } + + foreach (INamedTypeSymbol @interface in type.AllInterfaces) + { + if (@interface is { Name: "IEquatable", TypeArguments: [{ } typeArgument], ContainingNamespace: { Name: "System", ContainingNamespace.IsGlobalNamespace: true } } + && SymbolEqualityComparer.Default.Equals(typeArgument, type)) + { + return true; + } + } + + return false; + } + internal static IOperation? GetArgumentWithName(IInvocationOperation operation, string name) => operation.Arguments.FirstOrDefault(arg => arg.Parameter?.Name == name)?.Value.WalkDownConversion(); diff --git a/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs b/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs index 263131a272..12b9e8ea61 100644 --- a/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs +++ b/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs @@ -78,9 +78,11 @@ private static bool IsAlwaysFalse(IInvocationOperation operation) { "IsTrue" => AssertConditionAnalyzerHelper.GetConditionArgument(operation) is { ConstantValue: { HasValue: true, Value: false } }, "IsFalse" => AssertConditionAnalyzerHelper.GetConditionArgument(operation) is { ConstantValue: { HasValue: true, Value: true } }, - "AreEqual" => AssertConditionAnalyzerHelper.GetEqualityStatus(operation, AssertConditionAnalyzerHelper.ExpectedParameterName) == AssertConditionAnalyzerHelper.EqualityStatus.NotEqual, - "AreNotEqual" => AssertConditionAnalyzerHelper.GetEqualityStatus(operation, AssertConditionAnalyzerHelper.NotExpectedParameterName) == AssertConditionAnalyzerHelper.EqualityStatus.Equal - || AssertConditionAnalyzerHelper.HasIdenticalExpectedAndActual(operation, AssertConditionAnalyzerHelper.NotExpectedParameterName), + "AreEqual" => !AssertConditionAnalyzerHelper.HasNonDefaultEqualityComparerArgument(operation) + && AssertConditionAnalyzerHelper.GetEqualityStatus(operation, AssertConditionAnalyzerHelper.ExpectedParameterName) == AssertConditionAnalyzerHelper.EqualityStatus.NotEqual, + "AreNotEqual" => !AssertConditionAnalyzerHelper.HasNonDefaultEqualityComparerArgument(operation) + && (AssertConditionAnalyzerHelper.GetEqualityStatus(operation, AssertConditionAnalyzerHelper.NotExpectedParameterName) == AssertConditionAnalyzerHelper.EqualityStatus.Equal + || AssertConditionAnalyzerHelper.HasIdenticalExpectedAndActualWithBuiltInEquality(operation, AssertConditionAnalyzerHelper.NotExpectedParameterName)), "AreNotSame" => AssertConditionAnalyzerHelper.HasIdenticalExpectedAndActual(operation, AssertConditionAnalyzerHelper.NotExpectedParameterName), "IsNotNull" => AssertConditionAnalyzerHelper.GetValueArgument(operation) is { ConstantValue: { HasValue: true, Value: null } }, "IsNull" => AssertConditionAnalyzerHelper.GetValueArgument(operation) is { } valueArgumentOperation && AssertConditionAnalyzerHelper.IsNotNullableType(valueArgumentOperation), diff --git a/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs b/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs index ed2438ee61..4dfe5b3d40 100644 --- a/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs +++ b/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs @@ -67,9 +67,11 @@ private static bool IsAlwaysTrue(IInvocationOperation operation) { "IsTrue" => AssertConditionAnalyzerHelper.GetConditionArgument(operation) is { ConstantValue: { HasValue: true, Value: true } }, "IsFalse" => AssertConditionAnalyzerHelper.GetConditionArgument(operation) is { ConstantValue: { HasValue: true, Value: false } }, - "AreEqual" => AssertConditionAnalyzerHelper.GetEqualityStatus(operation, AssertConditionAnalyzerHelper.ExpectedParameterName) == AssertConditionAnalyzerHelper.EqualityStatus.Equal - || AssertConditionAnalyzerHelper.HasIdenticalExpectedAndActual(operation, AssertConditionAnalyzerHelper.ExpectedParameterName), - "AreNotEqual" => AssertConditionAnalyzerHelper.GetEqualityStatus(operation, AssertConditionAnalyzerHelper.NotExpectedParameterName) == AssertConditionAnalyzerHelper.EqualityStatus.NotEqual, + "AreEqual" => !AssertConditionAnalyzerHelper.HasNonDefaultEqualityComparerArgument(operation) + && (AssertConditionAnalyzerHelper.GetEqualityStatus(operation, AssertConditionAnalyzerHelper.ExpectedParameterName) == AssertConditionAnalyzerHelper.EqualityStatus.Equal + || AssertConditionAnalyzerHelper.HasIdenticalExpectedAndActualWithBuiltInEquality(operation, AssertConditionAnalyzerHelper.ExpectedParameterName)), + "AreNotEqual" => !AssertConditionAnalyzerHelper.HasNonDefaultEqualityComparerArgument(operation) + && AssertConditionAnalyzerHelper.GetEqualityStatus(operation, AssertConditionAnalyzerHelper.NotExpectedParameterName) == AssertConditionAnalyzerHelper.EqualityStatus.NotEqual, "AreSame" => AssertConditionAnalyzerHelper.HasIdenticalExpectedAndActual(operation, AssertConditionAnalyzerHelper.ExpectedParameterName), "IsNull" => AssertConditionAnalyzerHelper.GetValueArgument(operation) is { ConstantValue: { HasValue: true, Value: null } }, "IsNotNull" => AssertConditionAnalyzerHelper.GetValueArgument(operation) is { } valueArgumentOperation && AssertConditionAnalyzerHelper.IsNotNullableType(valueArgumentOperation), diff --git a/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs b/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs index ec98e934d3..7eaafadd56 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs @@ -1951,4 +1951,210 @@ private sealed class Wrapper await VerifyCS.VerifyCodeFixAsync(code, code); } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedSameLocalWithOverriddenEquals_NoDiagnostic() + { + // The type overrides object.Equals, so Assert.AreNotEqual routes through user code and the + // self-comparison is a legitimate way to exercise the equality contract (see issue #9972). + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new MyType(); + Assert.AreNotEqual(x, x); + } + + private sealed class MyType + { + public override bool Equals(object obj) => false; + public override int GetHashCode() => 0; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedSameLocalWithoutCustomEquality_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new MyType(); + [|Assert.AreNotEqual(x, x)|]; + } + + private sealed class MyType + { + } + } + """; + string fixedCode = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new MyType(); + Assert.Fail(); + } + + private sealed class MyType + { + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, fixedCode); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedNonEqualConstantsWithCustomComparer_NoDiagnostic() + { + // A caller-supplied comparer can return any result, so the comparison is not provably always false. + string code = """ + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + Assert.AreEqual(1, 2, new AlwaysEqualComparer()); + } + + private sealed class AlwaysEqualComparer : IEqualityComparer + { + public bool Equals(int x, int y) => true; + public int GetHashCode(int obj) => 0; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedSameLocalWithCustomComparer_NoDiagnostic() + { + string code = """ + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + Assert.AreNotEqual(x, x, new AlwaysEqualComparer()); + } + + private sealed class AlwaysEqualComparer : IEqualityComparer + { + public bool Equals(int x, int y) => true; + public int GetHashCode(int obj) => 0; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedSameLocalWithNullComparer_Diagnostic() + { + // MSTest treats a null comparer as EqualityComparer.Default, so the self-comparison is still + // provably always false and must be flagged. + string code = """ + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + [|Assert.AreNotEqual(x, x, (IEqualityComparer)null)|]; + } + } + """; + string fixedCode = """ + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + Assert.Fail(); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, fixedCode); + } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedSameLocalWithDefaultEqualityComparer_Diagnostic() + { + // EqualityComparer.Default passed explicitly is the default comparer, so the self-comparison is + // still provably always false and must be flagged. + string code = """ + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + [|Assert.AreNotEqual(x, x, EqualityComparer.Default)|]; + } + } + """; + string fixedCode = """ + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + Assert.Fail(); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, fixedCode); + } } diff --git a/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs b/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs index fa674b91d7..c93cecb7bd 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs @@ -1510,4 +1510,526 @@ public void TestMethod() await VerifyCS.VerifyCodeFixAsync(code, code); } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalWithOverriddenEquals_NoDiagnostic() + { + // The type overrides object.Equals, so Assert.AreEqual routes through user code and the + // self-comparison is a legitimate way to exercise the equality contract (see issue #9972). + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new MyType(); + Assert.AreEqual(x, x); + } + + private sealed class MyType + { + public override bool Equals(object obj) => obj is MyType; + public override int GetHashCode() => 0; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalWithEquatable_NoDiagnostic() + { + // A sealed type implementing IEquatable (without overriding object.Equals) routes equality + // through user code, so it must not be flagged. This exercises the IEquatable detection branch. + string code = """ + using System; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new MyType(); + Assert.AreEqual(x, x); + } + + private sealed class MyType : IEquatable + { + public bool Equals(MyType other) => true; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalWithOnlyEqualityOperator_Diagnostic() + { + // EqualityComparer.Default (used by Assert.AreEqual) never calls operator ==; it uses + // IEquatable.Equals or the virtual object.Equals. A type that only overloads == (without + // overriding Equals or implementing IEquatable) still compares by reference, so the + // self-comparison is genuinely always true and must be flagged. + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new MyType(); + [|Assert.AreEqual(x, x)|]; + } + + #pragma warning disable CS0660, CS0661 + private sealed class MyType + { + public static bool operator ==(MyType left, MyType right) => true; + public static bool operator !=(MyType left, MyType right) => false; + } + #pragma warning restore CS0660, CS0661 + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalWithEquatableOfOtherType_Diagnostic() + { + // The type implements IEquatable, not IEquatable, so EqualityComparer.Default + // falls back to reference equality and the self-comparison is genuinely always true. + string code = """ + using System; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new MyType(); + [|Assert.AreEqual(x, x)|]; + } + + private sealed class MyType : IEquatable + { + public bool Equals(string other) => true; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameGenericTypeParameter_NoDiagnostic() + { + // T can be substituted with a type whose equality is not reflexive, so we cannot prove the + // comparison is always true. + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + Helper(1); + } + + private static void Helper(T value) + { + Assert.AreEqual(value, value); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalWithoutCustomEquality_Diagnostic() + { + // A reference type that does not customize equality falls back to reference equality, + // so a self-comparison is genuinely always true and should still be flagged. + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new MyType(); + [|Assert.AreEqual(x, x)|]; + } + + private sealed class MyType + { + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameArray_Diagnostic() + { + // Arrays use reference equality, so a self-comparison is genuinely always true. + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new int[0]; + [|Assert.AreEqual(x, x)|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalOfPolymorphicType_NoDiagnostic() + { + // A non-sealed reference type can hold a derived instance whose overridden Equals is not + // reflexive, so equality cannot be proven from the static type. + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + object x = new object(); + Assert.AreEqual(x, x); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalOfNonSealedType_NoDiagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new MyType(); + Assert.AreEqual(x, x); + } + + private class MyType + { + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedEqualConstantsWithCustomComparer_NoDiagnostic() + { + // A caller-supplied comparer can return any result, so the comparison is not provably always true. + string code = """ + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + Assert.AreEqual(1, 1, new NeverEqualComparer()); + } + + private sealed class NeverEqualComparer : IEqualityComparer + { + public bool Equals(int x, int y) => false; + public int GetHashCode(int obj) => 0; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalWithCustomComparer_NoDiagnostic() + { + string code = """ + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + Assert.AreEqual(x, x, new NeverEqualComparer()); + } + + private sealed class NeverEqualComparer : IEqualityComparer + { + public bool Equals(int x, int y) => false; + public int GetHashCode(int obj) => 0; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameDateTime_Diagnostic() + { + // DateTime is a primitive-like value type with reflexive built-in equality, so a self-comparison + // is genuinely always true and should still be flagged. + string code = """ + using System; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + DateTime x = DateTime.Now; + [|Assert.AreEqual(x, x)|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameValueWithPolymorphicComparerType_NoDiagnostic() + { + // The comparison uses EqualityComparer.Default (the method type argument), which invokes the + // non-reflexive IEquatable, even though the operand's static type is the sealed Derived. + string code = """ + using System; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new Derived(); + Assert.AreEqual(x, x); + } + + private class Base : IEquatable + { + public bool Equals(Base other) => false; + } + + private sealed class Derived : Base + { + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameStructWithNonReflexiveField_NoDiagnostic() + { + // A struct using the default field-based ValueType.Equals compares its fields via their equality, + // so a field whose Equals is not reflexive makes the self-comparison return false. + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new Wrapper(); + Assert.AreEqual(x, x); + } + + private struct Wrapper + { + public NeverEqual Field; + } + + private sealed class NeverEqual + { + public override bool Equals(object obj) => false; + public override int GetHashCode() => 0; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameNullable_NoDiagnostic() + { + // Nullable delegates equality to the underlying T, which may not be reflexive, so it is treated + // conservatively. + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int? x = 1; + Assert.AreEqual(x, x); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalWithNullComparer_Diagnostic() + { + // MSTest treats a null comparer as EqualityComparer.Default, so the self-comparison is still + // provably always true and must be flagged. + string code = """ + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + [|Assert.AreEqual(x, x, (IEqualityComparer)null)|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalWithDefaultComparer_Diagnostic() + { + // default(IEqualityComparer) is null, which MSTest treats as EqualityComparer.Default, so the + // self-comparison is still provably always true. Only built-in conversions are stripped when inspecting + // the comparer argument. + string code = """ + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + [|Assert.AreEqual(x, x, default(IEqualityComparer))|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalWithDefaultEqualityComparer_Diagnostic() + { + // EqualityComparer.Default passed explicitly is the default comparer, so the self-comparison is + // still provably always true and must be flagged. + string code = """ + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + [|Assert.AreEqual(x, x, EqualityComparer.Default)|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedContravariantDefaultComparer_NoDiagnostic() + { + // IEqualityComparer is contravariant, so EqualityComparer.Default can be passed where an + // IEqualityComparer is expected. That is NOT EqualityComparer.Default and can return a + // different (non-reflexive) result, so it must not be treated as the default comparer. + string code = """ + using System; + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new Derived(); + Assert.AreEqual(x, x, EqualityComparer.Default); + } + + private class Base : IEquatable + { + public bool Equals(Base other) => false; + } + + private sealed class Derived : Base + { + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } } From 4e8763b4a881ee8ed51ac4464f2894f81a0ad9b9 Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Tue, 21 Jul 2026 15:43:29 +0200 Subject: [PATCH 06/15] Invalidate generated source cache on MSBuild task updates by @Evangelink in #10082 (backport to rel/4.3) (#10111) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé --- ...Microsoft.Testing.Platform.MSBuild.targets | 22 ++++- .../MSBuildTests.GenerateEntryPoint.cs | 84 +++++++++++++++++++ 2 files changed, 104 insertions(+), 2 deletions(-) diff --git a/src/Platform/Microsoft.Testing.Platform.MSBuild/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.targets b/src/Platform/Microsoft.Testing.Platform.MSBuild/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.targets index a7d7de7216..4d003a905a 100644 --- a/src/Platform/Microsoft.Testing.Platform.MSBuild/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.targets +++ b/src/Platform/Microsoft.Testing.Platform.MSBuild/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.targets @@ -111,6 +111,22 @@ + + + + + + + + + + + + @@ -120,7 +136,7 @@ - + <_GenerateTestingPlatformEntryPointInputsCachFilePath>$(IntermediateOutputPath)$(MSBuildProjectName).gentestingplatformentrypointinputcache.cache @@ -131,6 +147,7 @@ <_GenerateTestingPlatformEntryPointInputsCacheToHash Include="@(TestingPlatformBuilderHook)"/> <_GenerateTestingPlatformEntryPointInputsCacheToHash Include="$(RootNamespace)"/> <_GenerateTestingPlatformEntryPointInputsCacheToHash Include="$(GenerateTestingPlatformEntryPoint)" /> + <_GenerateTestingPlatformEntryPointInputsCacheToHash Include="@(_TestingPlatformMSBuildTaskFileWithHash->'%(FileHash)')" /> @@ -213,7 +230,7 @@ - + <_GenerateSelfRegisteredExtensionsInputsCachFilePath>$(IntermediateOutputPath)$(MSBuildProjectName).genautoregisteredextensionsinputcache.cache @@ -224,6 +241,7 @@ <_GenerateSelfRegisteredExtensionsInputsCacheToHash Include="@(TestingPlatformBuilderHook)"/> <_GenerateSelfRegisteredExtensionsInputsCacheToHash Include="$(RootNamespace)"/> <_GenerateSelfRegisteredExtensionsInputsCacheToHash Include="$(GenerateSelfRegisteredExtensions)"/> + <_GenerateSelfRegisteredExtensionsInputsCacheToHash Include="@(_TestingPlatformMSBuildTaskFileWithHash->'%(FileHash)')" /> diff --git a/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MSBuildTests.GenerateEntryPoint.cs b/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MSBuildTests.GenerateEntryPoint.cs index 2fc0559686..07714d26e3 100644 --- a/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MSBuildTests.GenerateEntryPoint.cs +++ b/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MSBuildTests.GenerateEntryPoint.cs @@ -127,6 +127,90 @@ namespace MSBuildTests |> Async.AwaitTask |> Async.RunSynchronously'", "Fsc"); + [TestMethod] + public async Task GeneratedSourcesAreRegeneratedWhenMSBuildTaskChanges() + { + string sourceCode = CSharpSourceCode + .PatchCodeWithReplace("$TargetFrameworks$", TargetFrameworks.NetCurrent) + .PatchCodeWithReplace("$MicrosoftTestingPlatformVersion$", MicrosoftTestingPlatformVersion); + using TestAsset testAsset = await TestAsset.GenerateAssetAsync(nameof(GeneratedSourcesAreRegeneratedWhenMSBuildTaskChanges), sourceCode); + + DotnetMuxerResult buildResult = await DotnetCli.RunAsync( + $"build -c {BuildConfiguration.Debug} {testAsset.TargetAssetPath} -v:n -nr:false", + cancellationToken: TestContext.CancellationToken); + SL.Build binLog = SL.Serialization.Read(buildResult.BinlogPath!); + string taskAssembly = binLog.FindChildrenRecursive() + .Single(t => t.Name == "TestingPlatformEntryPointTask") + .FromAssembly; + + using TempDirectory taskDirectory = new(); + taskDirectory.CopyDirectory(Path.GetDirectoryName(taskAssembly)!, taskDirectory.Path); + string copiedTaskAssembly = Path.Combine(taskDirectory.Path, Path.GetFileName(taskAssembly)); + File.SetAttributes(copiedTaskAssembly, FileAttributes.Normal); + string taskFolderProperty = $"-p:MicrosoftTestingPlatformMSBuildTaskFolder={taskDirectory.Path}{Path.DirectorySeparatorChar}"; + + Directory.Delete(Path.Combine(testAsset.TargetAssetPath, "obj"), recursive: true); + buildResult = await DotnetCli.RunAsync( + $"build -c {BuildConfiguration.Debug} {taskFolderProperty} {testAsset.TargetAssetPath} -v:n -nr:false", + cancellationToken: TestContext.CancellationToken); + binLog = SL.Serialization.Read(buildResult.BinlogPath!); + + Assert.HasCount(1, binLog.FindChildrenRecursive().Where(t => t.Name == "TestingPlatformEntryPointTask")); + Assert.HasCount(1, binLog.FindChildrenRecursive().Where(t => t.Name == "TestingPlatformSelfRegisteredExtensions")); + + await using (FileStream stream = new(copiedTaskAssembly, FileMode.Append, FileAccess.Write, FileShare.None)) + { + stream.WriteByte(0); + } + + buildResult = await DotnetCli.RunAsync( + $"build -c {BuildConfiguration.Debug} {taskFolderProperty} {testAsset.TargetAssetPath} -v:n -nr:false", + cancellationToken: TestContext.CancellationToken); + binLog = SL.Serialization.Read(buildResult.BinlogPath!); + + Assert.HasCount(1, binLog.FindChildrenRecursive().Where(t => t.Name == "TestingPlatformEntryPointTask")); + Assert.HasCount(1, binLog.FindChildrenRecursive().Where(t => t.Name == "TestingPlatformSelfRegisteredExtensions")); + + buildResult = await DotnetCli.RunAsync( + $"build -c {BuildConfiguration.Debug} {taskFolderProperty} {testAsset.TargetAssetPath} -v:n -nr:false", + cancellationToken: TestContext.CancellationToken); + binLog = SL.Serialization.Read(buildResult.BinlogPath!); + + Assert.IsEmpty(binLog.FindChildrenRecursive().Where(t => t.Name == "TestingPlatformEntryPointTask")); + Assert.IsEmpty(binLog.FindChildrenRecursive().Where(t => t.Name == "TestingPlatformSelfRegisteredExtensions")); + AssertTargetSkippedAsUpToDate(binLog, "_GenerateTestingPlatformEntryPoint"); + AssertTargetSkippedAsUpToDate(binLog, "_GenerateSelfRegisteredExtensions"); + + Directory.Delete(Path.Combine(testAsset.TargetAssetPath, "obj"), recursive: true); + string selfRegistrationOnlyProperties = $"{taskFolderProperty} -p:GenerateTestingPlatformEntryPoint=false -p:OutputType=Library"; + await DotnetCli.RunAsync( + $"build -c {BuildConfiguration.Debug} {selfRegistrationOnlyProperties} {testAsset.TargetAssetPath} -v:n -nr:false", + cancellationToken: TestContext.CancellationToken); + + await using (FileStream stream = new(copiedTaskAssembly, FileMode.Append, FileAccess.Write, FileShare.None)) + { + stream.WriteByte(0); + } + + buildResult = await DotnetCli.RunAsync( + $"build -c {BuildConfiguration.Debug} {selfRegistrationOnlyProperties} {testAsset.TargetAssetPath} -v:n -nr:false", + cancellationToken: TestContext.CancellationToken); + binLog = SL.Serialization.Read(buildResult.BinlogPath!); + + Assert.IsEmpty(binLog.FindChildrenRecursive().Where(t => t.Name == "TestingPlatformEntryPointTask")); + Assert.HasCount(1, binLog.FindChildrenRecursive().Where(t => t.Name == "TestingPlatformSelfRegisteredExtensions")); + } + + private static void AssertTargetSkippedAsUpToDate(SL.Build binLog, string targetName) + { + SL.Target target = binLog.FindChildrenRecursive().Single(t => t.Name == targetName && t.Children.Count > 0); + Assert.HasCount( + 1, + target.FindChildrenRecursive().Where(m => m.Text.Contains( + $"Skipping target \"{targetName}\" because all output files are up-to-date with respect to the input files.", + StringComparison.OrdinalIgnoreCase))); + } + private async Task GenerateAndVerifyLanguageSpecificEntryPointAsync(string assetName, string sourceCode, string languageFileExtension, string tfm, BuildConfiguration compilationMode, Verb verb, string expectedEntryPoint, string cscProcessName) { From 4fb1a03f08fa06e85357104d591fd5b6fca5211c Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Tue, 21 Jul 2026 15:45:50 +0200 Subject: [PATCH 07/15] Fix IndexOutOfRangeException in VSTestBridge BuildFilter UID escaping by @azat-msft in #9771 (backport to rel/4.3) (#10112) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Azat Mukhametshin Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Amaury Levé Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../ObjectModel/ContextAdapterBase.cs | 32 +- .../RunContextAdapterFilterTests.cs | 294 ++++++++++++++++++ 2 files changed, 299 insertions(+), 27 deletions(-) create mode 100644 test/UnitTests/Microsoft.Testing.Extensions.VSTestBridge.UnitTests/ObjectModel/RunContextAdapterFilterTests.cs diff --git a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ContextAdapterBase.cs b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ContextAdapterBase.cs index df6c1b34b3..ff7c703644 100644 --- a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ContextAdapterBase.cs +++ b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ContextAdapterBase.cs @@ -152,33 +152,11 @@ private static void BuildFilter(TestNodeUid[] testNodesUid, StringBuilder filter TestNodeUid currentTestNodeUid = testNodesUid[i]; filter.Append("FullyQualifiedName="); - for (int k = 0; k < currentTestNodeUid.Value.Length; k++) - { - char currentChar = currentTestNodeUid.Value[k]; - switch (currentChar) - { - case '\\': - case '(': - case ')': - case '&': - case '|': - case '=': - case '!': - case '~': - // If the symbol is not escaped, add an escape character. - if (i - 1 < 0 || currentTestNodeUid.Value[k - 1] != '\\') - { - filter.Append('\\'); - } - - filter.Append(currentChar); - break; - - default: - filter.Append(currentChar); - break; - } - } + + // Use VSTest's canonical escaper rather than a hand-rolled loop. It escapes every filter + // operator ('\', '(', ')', '&', '|', '=', '!', '~') unconditionally, which is exactly what + // we need for a raw (un-escaped) test-node UID. + filter.Append(FilterHelper.Escape(currentTestNodeUid.Value)); } } } diff --git a/test/UnitTests/Microsoft.Testing.Extensions.VSTestBridge.UnitTests/ObjectModel/RunContextAdapterFilterTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.VSTestBridge.UnitTests/ObjectModel/RunContextAdapterFilterTests.cs new file mode 100644 index 0000000000..c3a9056ac9 --- /dev/null +++ b/test/UnitTests/Microsoft.Testing.Extensions.VSTestBridge.UnitTests/ObjectModel/RunContextAdapterFilterTests.cs @@ -0,0 +1,294 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Microsoft.Testing.Extensions.VSTestBridge.CommandLine; +using Microsoft.Testing.Extensions.VSTestBridge.ObjectModel; +using Microsoft.Testing.Platform.CommandLine; +using Microsoft.Testing.Platform.Extensions.Messages; +using Microsoft.Testing.Platform.Requests; +using Microsoft.VisualStudio.TestPlatform.ObjectModel; +using Microsoft.VisualStudio.TestPlatform.ObjectModel.Adapter; + +using Moq; + +namespace Microsoft.Testing.Extensions.VSTestBridge.UnitTests.ObjectModel; + +/// +/// Tests for the filter building logic in (exercised through +/// ), most notably the private BuildFilter method that turns a +/// coming from the server protocol into a VSTest +/// TestCaseFilter expression. +/// +[TestClass] +public sealed class RunContextAdapterFilterTests +{ + private const string EmptyRunSettings = +""" + + + + +"""; + + [TestMethod] + public void GetTestCaseFilter_WithSecondNodeContainingPipe_KeepsBothNodesAndEscapesPipe() + { + // A later node whose name contains '|' must be emitted as an escaped literal ('\|') so it + // stays part of its FullyQualifiedName value, while the '|' that joins the two clauses remains + // the OR operator. + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter("Namespace.MyClass.MyTest", "PrintArg(\"as|\")")); + string res = GetFilterValue(adapter); + + Assert.AreEqual("(FullyQualifiedName=Namespace.MyClass.MyTest|FullyQualifiedName=PrintArg\\(\"as\\|\"\\))", res); + } + + [TestMethod] + public void GetTestCaseFilter_WithNopFilter_AndNoRunSettingsOrCommandLineFilter_ReturnsNull() + { + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, new NopFilter()); + + Assert.IsNull(adapter.GetTestCaseFilter(null, _ => null)); + } + + [TestMethod] + public void GetTestCaseFilter_WithSingleFullyQualifiedNameNode_BuildsFullyQualifiedNameFilter() + { + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter("Namespace.MyClass.MyTest")); + + Assert.AreEqual("(FullyQualifiedName=Namespace.MyClass.MyTest)", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithEmptyNodeList_ThrowsOnEmptyGroup() + { + // An empty UID list is constructible and builds the empty group "()", which the VSTest filter + // parser rejects, so GetTestCaseFilter surfaces a format error. Pinning this guards the edge + // case now that this suite owns the filter-building coverage. + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter()); + + TestPlatformFormatException exception = Assert.ThrowsExactly(() => adapter.GetTestCaseFilter(null, _ => null)); + Assert.AreEqual("()", exception.FilterValue); + } + + [TestMethod] + public void GetTestCaseFilter_WithMultipleNodes_JoinsWithOrOperator() + { + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter("A.B.Test1", "C.D.Test2")); + + Assert.AreEqual("(FullyQualifiedName=A.B.Test1|FullyQualifiedName=C.D.Test2)", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithGuidNode_BuildsIdFilter() + { + var guid = new Guid("12345678-1234-1234-1234-1234567890ab"); + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter(guid.ToString())); + + Assert.AreEqual($"(Id={guid})", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithMixedGuidAndNameNodes_BuildsIdAndFullyQualifiedNameFilters() + { + var guid = new Guid("12345678-1234-1234-1234-1234567890ab"); + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter(guid.ToString(), "A.B.Test")); + + Assert.AreEqual($"(Id={guid}|FullyQualifiedName=A.B.Test)", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithSpecialCharactersInName_EscapesFilterOperators() + { + // The pipe, parentheses, ampersand, equals, bang, tilde and backslash are all TestCaseFilter + // operators and must be escaped so they are treated as literals (regression for tests whose + // display name contains such characters, e.g. NUnit [TestCase("as|")]). + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter("Ns.PrintArg(\"as|\")")); + + Assert.AreEqual("(FullyQualifiedName=Ns.PrintArg\\(\"as\\|\"\\))", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithAllOperatorCharacters_EscapesEachOfThem() + { + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter("a\\b(c)d&e|f=g!h~i")); + + Assert.AreEqual("(FullyQualifiedName=a\\\\b\\(c\\)d\\&e\\|f\\=g\\!h\\~i)", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithRunSettingsTestCaseFilter_UsesRunSettingsFilter() + { + string runSettings = +""" + + + Category=Fast + + +"""; + RunContextAdapter adapter = CreateAdapter(runSettings, new NopFilter()); + + Assert.AreEqual("(Category=Fast)", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithCommandLineFilter_UsesCommandLineFilter() + { + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, new NopFilter(), commandLineFilter: "Category=Slow"); + + Assert.AreEqual("(Category=Slow)", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithRunSettingsAndNodeFilter_CombinesWithAndOperator() + { + string runSettings = +""" + + + Category=Fast + + +"""; + RunContextAdapter adapter = CreateAdapter(runSettings, CreateUidFilter("A.B.Test")); + + Assert.AreEqual("(Category=Fast) & (FullyQualifiedName=A.B.Test)", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithRunSettingsAndCommandLineFilter_CombinesWithAndOperator() + { + string runSettings = +""" + + + Category=Fast + + +"""; + RunContextAdapter adapter = CreateAdapter(runSettings, new NopFilter(), commandLineFilter: "Priority=1"); + + Assert.AreEqual("(Category=Fast) & (Priority=1)", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithSingleNodeStartingWithSpecialCharacter_DoesNotThrowAndEscapes() + { + // Companion to the multi-node regression above, exhaustive over the index dimension: the + // single-node case (i == 0) never threw under the old code because the "i - 1 < 0" guard + // short-circuited before the bogus Value[k - 1] read. It must still escape the leading operator. + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter("(weird")); + + Assert.AreEqual("(FullyQualifiedName=\\(weird)", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithSecondNodeStartingWithSpecialCharacter_DoesNotThrowAndEscapes() + { + // Regression: the "already-escaped" guard in BuildFilter used the node index (i) instead of + // the character index (k) when looking back at the previous character. For any node after the + // first (i > 0) whose first character (k == 0) is a filter operator, this evaluated + // Value[k - 1] == Value[-1] and threw IndexOutOfRangeException. + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter("A.B.Test", "(weird")); + + Assert.AreEqual("(FullyQualifiedName=A.B.Test|FullyQualifiedName=\\(weird)", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithOperator_EscapesOperator() + { + // A bare operator ('|') in the name must be escaped so it stays a literal instead of being + // parsed as an OR that splits the clause. + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter("A.B|C")); + + Assert.AreEqual("(FullyQualifiedName=A.B\\|C)", GetFilterValue(adapter)); + } + + [TestMethod] + public void GetTestCaseFilter_WithBackslashFollowedBySpecialCharacter_EscapesBoth() + { + // Regression: the buggy "already-escaped" guard treated the operator following a literal + // backslash as if it were already escaped, so it emitted the operator un-escaped. A raw + // backslash in the name must be escaped to "\\" AND the following operator must still be + // escaped, otherwise the operator (e.g. '|') is parsed as an OR and the clause is split. + RunContextAdapter adapterBackslash = CreateAdapter(EmptyRunSettings, CreateUidFilter("A.B\\|C")); + + Assert.AreEqual("(FullyQualifiedName=A.B\\\\\\|C)", GetFilterValue(adapterBackslash)); + } + + [TestMethod] + public void GetTestCaseFilter_WithSpecialCharactersInName_RoundTripsAndMatchesOnlyExactName() + { + // Verify the full escape -> parse -> match path (not just the emitted string): a name full of + // filter operators must, once escaped, parse back to a filter that matches a test case whose + // FullyQualifiedName is exactly that name and rejects any other name. + const string name = "Ns.PrintArg(\"as|\")"; + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter(name)); + + Assert.IsTrue(MatchesFullyQualifiedName(adapter, name)); + Assert.IsFalse(MatchesFullyQualifiedName(adapter, "Ns.PrintArg(\"as\")")); + } + + [TestMethod] + public void GetTestCaseFilter_WithBackslashFollowedBySpecialCharacter_RoundTripsAndMatchesOnlyExactName() + { + // Regression guard for the round trip: if the backslash-then-operator sequence were under-escaped + // the parser would split the value at the operator, so the filter would no longer match the exact + // name (and might match a truncated one). + const string name = "A.B\\|C"; + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter(name)); + + Assert.IsTrue(MatchesFullyQualifiedName(adapter, name)); + Assert.IsFalse(MatchesFullyQualifiedName(adapter, "A.B")); + } + + [TestMethod] + public void GetTestCaseFilter_WithMultipleNodes_MatchesEitherNodeButNotOthers() + { + // The '|' joining the two clauses must stay an OR operator while a '|' inside a name stays a + // literal, so the filter matches each node's exact name (including the special-character one) and + // nothing else. + RunContextAdapter adapter = CreateAdapter(EmptyRunSettings, CreateUidFilter("A.B.Test1", "PrintArg(\"as|\")")); + + Assert.IsTrue(MatchesFullyQualifiedName(adapter, "A.B.Test1")); + Assert.IsTrue(MatchesFullyQualifiedName(adapter, "PrintArg(\"as|\")")); + Assert.IsFalse(MatchesFullyQualifiedName(adapter, "A.B.Test2")); + } + + private static TestNodeUidListFilter CreateUidFilter(params string[] uids) + => new([.. uids.Select(uid => new TestNodeUid(uid))]); + + private static string GetFilterValue(RunContextAdapter adapter) + { + ITestCaseFilterExpression? filterExpression = adapter.GetTestCaseFilter(null, _ => null); + Assert.IsNotNull(filterExpression); + return filterExpression.TestCaseFilterValue; + } + + private static bool MatchesFullyQualifiedName(RunContextAdapter adapter, string fullyQualifiedName) + { + ITestCaseFilterExpression? filterExpression = adapter.GetTestCaseFilter(null, _ => null); + Assert.IsNotNull(filterExpression); + + var testCase = new TestCase(fullyQualifiedName, new Uri("executor://mstest"), "source.dll"); + return filterExpression.MatchTestCase( + testCase, + propertyName => string.Equals(propertyName, "FullyQualifiedName", StringComparison.OrdinalIgnoreCase) + ? fullyQualifiedName + : null); + } + + private static RunContextAdapter CreateAdapter(string runSettingsXml, ITestExecutionFilter filter, string? commandLineFilter = null) + { + var runSettings = new Mock(); + runSettings.Setup(x => x.SettingsXml).Returns(runSettingsXml); + + var commandLineOptions = new Mock(); + string[]? commandLineFilterArguments = commandLineFilter is null ? null : [commandLineFilter]; + commandLineOptions + .Setup(x => x.TryGetOptionArgumentList(TestCaseFilterCommandLineOptionsProvider.TestCaseFilterOptionName, out commandLineFilterArguments)) + .Returns(commandLineFilter is not null); + + return new RunContextAdapter(commandLineOptions.Object, runSettings.Object, filter); + } +} From f5176667c4c960b387ae0e828756f0473cf75876 Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Tue, 21 Jul 2026 15:47:41 +0200 Subject: [PATCH 08/15] Fix MSTEST0063 to detect invalid constructors on derived TestClass attributes by @Evangelink in #9851 (backport to rel/4.3) (#10115) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...stClassConstructorShouldBeValidAnalyzer.cs | 2 +- ...ssConstructorShouldBeValidAnalyzerTests.cs | 94 +++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/src/Analyzers/MSTest.Analyzers/TestClassConstructorShouldBeValidAnalyzer.cs b/src/Analyzers/MSTest.Analyzers/TestClassConstructorShouldBeValidAnalyzer.cs index e2e1ba256c..70bf91d22f 100644 --- a/src/Analyzers/MSTest.Analyzers/TestClassConstructorShouldBeValidAnalyzer.cs +++ b/src/Analyzers/MSTest.Analyzers/TestClassConstructorShouldBeValidAnalyzer.cs @@ -60,7 +60,7 @@ private static void AnalyzeSymbol(SymbolAnalysisContext context, INamedTypeSymbo if (namedTypeSymbol.TypeKind != TypeKind.Class || namedTypeSymbol.IsAbstract || namedTypeSymbol.IsStatic - || !namedTypeSymbol.GetAttributes().Any(attr => SymbolEqualityComparer.Default.Equals(attr.AttributeClass, testClassAttributeSymbol))) + || !namedTypeSymbol.IsTestClass(testClassAttributeSymbol)) { return; } diff --git a/test/UnitTests/MSTest.Analyzers.UnitTests/TestClassConstructorShouldBeValidAnalyzerTests.cs b/test/UnitTests/MSTest.Analyzers.UnitTests/TestClassConstructorShouldBeValidAnalyzerTests.cs index 37af3eda2b..f3d5564915 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/TestClassConstructorShouldBeValidAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/TestClassConstructorShouldBeValidAnalyzerTests.cs @@ -343,4 +343,98 @@ public static class MyTestClass await VerifyCS.VerifyCodeFixAsync(code, code); } + + [TestMethod] + public async Task WhenDerivedTestClassAttributeHasPrivateConstructor_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [STATestClass] + public class {|#0:MyTestClass|} + { + private MyTestClass() + { + } + } + """; + + await VerifyCS.VerifyCodeFixAsync( + code, + VerifyCS.Diagnostic(TestClassConstructorShouldBeValidAnalyzer.TestClassConstructorShouldBeValidRule) + .WithLocation(0) + .WithArguments("MyTestClass"), + code); + } + + [TestMethod] + public async Task WhenDerivedTestClassAttributeHasPublicParameterlessConstructor_NoDiagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [STATestClass] + public class MyTestClass + { + public MyTestClass() + { + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenCustomDerivedTestClassAttributeHasInternalConstructor_Diagnostic() + { + string code = """ + using System; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [AttributeUsage(AttributeTargets.Class)] + public class MyTestClassAttribute : TestClassAttribute + { + } + + [MyTestClass] + public class {|#0:MyTestClass|} + { + internal MyTestClass() + { + } + } + """; + + await VerifyCS.VerifyCodeFixAsync( + code, + VerifyCS.Diagnostic(TestClassConstructorShouldBeValidAnalyzer.TestClassConstructorShouldBeValidRule) + .WithLocation(0) + .WithArguments("MyTestClass"), + code); + } + + [TestMethod] + public async Task WhenCustomDerivedTestClassAttributeHasPublicConstructor_NoDiagnostic() + { + string code = """ + using System; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [AttributeUsage(AttributeTargets.Class)] + public class MyTestClassAttribute : TestClassAttribute + { + } + + [MyTestClass] + public class MyTestClass + { + public MyTestClass() + { + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } } From e57e52b092cd06a8ce66f4350a07db2f3e3303ed Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Tue, 21 Jul 2026 16:58:26 +0200 Subject: [PATCH 09/15] Fix test property lifecycle scope by @Evangelink in #10080 (backport to rel/4.3) (#10110) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé --- .../Execution/TestExecutionManager.Runner.cs | 6 +- .../Execution/UnitTestRunner.RunSingleTest.cs | 39 +++++- .../Execution/UnitTestRunner.TestFilter.cs | 8 +- .../TestContextPropertyFlowTests.cs | 118 +++++++++++++++++- .../Execution/TestExecutionManagerTests.cs | 88 +++++++++++++ 5 files changed, 246 insertions(+), 13 deletions(-) diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Runner.cs b/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Runner.cs index dd43d20b47..9a9e17ade0 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Runner.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Runner.cs @@ -95,6 +95,8 @@ private async Task ExecuteTestsWithTestRunnerAsync( ? new RemotingMessageLogger(testExecutionRecorder) : testExecutionRecorder; + Dictionary lifecycleContextProperties = [with(sourceLevelParameters!)]; + foreach (TestCase currentTest in orderedTests) { _testRunCancellationToken?.ThrowIfCancellationRequested(); @@ -128,12 +130,12 @@ private async Task ExecuteTestsWithTestRunnerAsync( // Alternatively, if we want to use RunSingleTestAsync for the case of STA, we should have: // 1. A custom single threaded synchronization context that keeps us in STA. // 2. Use ConfigureAwait(true). - unitTestResult = testRunner.RunSingleTest(unitTestElement, testContextProperties, remotingMessageLogger); + unitTestResult = testRunner.RunSingleTest(unitTestElement, testContextProperties, lifecycleContextProperties, remotingMessageLogger); #pragma warning restore VSTHRD103 // Call async methods when in an async method } else { - unitTestResult = await testRunner.RunSingleTestAsync(unitTestElement, testContextProperties, remotingMessageLogger).ConfigureAwait(false); + unitTestResult = await testRunner.RunSingleTestAsync(unitTestElement, testContextProperties, lifecycleContextProperties, remotingMessageLogger).ConfigureAwait(false); } if (PlatformServiceProvider.Instance.AdapterTraceLogger.IsInfoEnabled) diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.RunSingleTest.cs b/src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.RunSingleTest.cs index 8537205e91..ca97ec85bf 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.RunSingleTest.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.RunSingleTest.cs @@ -17,6 +17,15 @@ internal sealed partial class UnitTestRunner internal TestResult[] RunSingleTest(UnitTestElement unitTestElement, IDictionary testContextProperties, IMessageLogger messageLogger) => RunSingleTestAsync(unitTestElement, testContextProperties, messageLogger).GetAwaiter().GetResult(); + // Task cannot cross app domains. + // For now, TestExecutionManager will call this sync method which is hacky. + internal TestResult[] RunSingleTest( + UnitTestElement unitTestElement, + IDictionary testContextProperties, + IDictionary lifecycleContextProperties, + IMessageLogger messageLogger) + => RunSingleTestAsync(unitTestElement, testContextProperties, lifecycleContextProperties, messageLogger).GetAwaiter().GetResult(); + /// /// Runs a single test. /// @@ -25,6 +34,21 @@ internal TestResult[] RunSingleTest(UnitTestElement unitTestElement, IDictionary /// The message logger. /// The . internal async Task RunSingleTestAsync(UnitTestElement unitTestElement, IDictionary testContextProperties, IMessageLogger messageLogger) + => await RunSingleTestAsync(unitTestElement, testContextProperties, testContextProperties, messageLogger).ConfigureAwait(false); + + /// + /// Runs a single test. + /// + /// The test method. + /// Properties scoped to this test. + /// Properties scoped to assembly and class lifecycle methods. + /// The message logger. + /// The . + internal async Task RunSingleTestAsync( + UnitTestElement unitTestElement, + IDictionary testContextProperties, + IDictionary lifecycleContextProperties, + IMessageLogger messageLogger) { if (unitTestElement is null) { @@ -36,6 +60,11 @@ internal async Task RunSingleTestAsync(UnitTestElement unitTestEle throw new ArgumentNullException(nameof(testContextProperties)); } + if (lifecycleContextProperties is null) + { + throw new ArgumentNullException(nameof(lifecycleContextProperties)); + } + TestMethod testMethod = unitTestElement.TestMethod; ITestContext? testContextForTestExecution = null; ITestContext? testContextForAssemblyInit = null; @@ -56,7 +85,7 @@ internal async Task RunSingleTestAsync(UnitTestElement unitTestEle { return await FinishFilteredOutTestAsync( testMethod, - testContextProperties, + lifecycleContextProperties, messageLogger, filterResult, testContextForTestExecution).ConfigureAwait(false); @@ -84,7 +113,7 @@ internal async Task RunSingleTestAsync(UnitTestElement unitTestEle } else { - testContextForAssemblyInit = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, null, testContextProperties, messageLogger, testContextForTestExecution.Context.CurrentTestOutcome); + testContextForAssemblyInit = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, null, lifecycleContextProperties, messageLogger, testContextForTestExecution.Context.CurrentTestOutcome); assemblyInitializeResult = await RunAssemblyInitializeIfNeededAsync(testMethodInfo, testContextForAssemblyInit).ConfigureAwait(false); } @@ -116,7 +145,7 @@ internal async Task RunSingleTestAsync(UnitTestElement unitTestEle } else { - testContextForClassInit = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, testMethod.FullClassName, testContextProperties, messageLogger, UnitTestOutcome.InProgress); + testContextForClassInit = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, testMethod.FullClassName, lifecycleContextProperties, messageLogger, UnitTestOutcome.InProgress); // Flow properties set during AssemblyInitialize into the class-init context so the // ClassInitialize method observes them. @@ -170,7 +199,7 @@ internal async Task RunSingleTestAsync(UnitTestElement unitTestEle { // Defer TestContextImplementation allocation to only the last test in each class, // saving one dict-copy + CancellationTokenRegistration per non-last test. - testContextForClassCleanup = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, testMethod.FullClassName, testContextProperties, messageLogger, testContextForTestExecution.Context.CurrentTestOutcome); + testContextForClassCleanup = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, testMethod.FullClassName, lifecycleContextProperties, messageLogger, testContextForTestExecution.Context.CurrentTestOutcome); if (testMethodInfo is not null) { @@ -208,7 +237,7 @@ internal async Task RunSingleTestAsync(UnitTestElement unitTestEle // testContextForClassCleanup is guaranteed non-null here: ShouldRunEndOfAssemblyCleanup // becomes true only after MarkClassComplete, which is called exclusively inside the // isLastTestInClass block above — where testContextForClassCleanup is allocated. - testContextForAssemblyCleanup = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, null, testContextProperties, messageLogger, testContextForClassCleanup.Context.CurrentTestOutcome); + testContextForAssemblyCleanup = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, null, lifecycleContextProperties, messageLogger, testContextForClassCleanup.Context.CurrentTestOutcome); TestResult? assemblyCleanupResult = await RunAssemblyCleanupAsync(testContextForAssemblyCleanup, _typeCache, result).ConfigureAwait(false); if (assemblyCleanupResult is not null) diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.TestFilter.cs b/src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.TestFilter.cs index 8b0ba506c4..7c4850452c 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.TestFilter.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.TestFilter.cs @@ -143,13 +143,13 @@ private static TestFilterContext CreateFilterContext(UnitTestElement element) /// /// Handles the bookkeeping (class-cleanup countdown, class cleanup, end-of-assembly cleanup) for a /// test that was filtered out by a . Mirrors the tail of - /// . The filtered-out test never loaded its own type, but if a + /// normal test execution path. The filtered-out test never loaded its own type, but if a /// sibling test of the same class already ran in this worker the class was initialized and still /// owes its [ClassCleanup], so it is executed here when this is the last test of the class. /// private async Task FinishFilteredOutTestAsync( TestMethod testMethod, - IDictionary testContextProperties, + IDictionary lifecycleContextProperties, IMessageLogger messageLogger, TestResult[] filterResult, ITestContext testContextForTestExecution) @@ -172,7 +172,7 @@ private async Task FinishFilteredOutTestAsync( TestMethodInfo? testMethodInfo = _typeCache.GetTestMethodInfo(lastRunnableTest.TestMethod); if (testMethodInfo is not null) { - ITestContext testContextForClassCleanup = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, testMethod.FullClassName, testContextProperties, messageLogger, testContextForTestExecution.Context.CurrentTestOutcome); + ITestContext testContextForClassCleanup = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, testMethod.FullClassName, lifecycleContextProperties, messageLogger, testContextForTestExecution.Context.CurrentTestOutcome); try { // Flow properties set during AssemblyInitialize and ClassInitialize so the @@ -212,7 +212,7 @@ private async Task FinishFilteredOutTestAsync( ITestContext? testContextForAssemblyCleanup = null; try { - testContextForAssemblyCleanup = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, null, testContextProperties, messageLogger, testContextForTestExecution.Context.CurrentTestOutcome); + testContextForAssemblyCleanup = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, null, lifecycleContextProperties, messageLogger, testContextForTestExecution.Context.CurrentTestOutcome); TestResult? assemblyCleanupResult = await RunAssemblyCleanupAsync(testContextForAssemblyCleanup, _typeCache, filterResult).ConfigureAwait(false); if (assemblyCleanupResult is not null) diff --git a/test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestContextPropertyFlowTests.cs b/test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestContextPropertyFlowTests.cs index 780499b99d..73c9d93839 100644 --- a/test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestContextPropertyFlowTests.cs +++ b/test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestContextPropertyFlowTests.cs @@ -26,8 +26,22 @@ public async Task PropertiesSetInAssemblyInitAndClassInitAreVisibleEverywhere(st // PropertyFlowTests: TestMethodOne + TestMethodTwo = 2 // SecondClassTests: TestMethod = 1 // DataRowFlowTests: two data rows = 2 - // Total = 5 - testHostResult.AssertOutputContainsSummary(failed: 0, passed: 5, skipped: 0); + // PerTestPropertyScopeTests: TestWithFirstProperties + TestWithSecondProperties = 2 + // Total = 7 + testHostResult.AssertOutputContainsSummary(failed: 0, passed: 7, skipped: 0); + } + + [TestMethod] + [DynamicData(nameof(TargetFrameworks.AllForDynamicData), typeof(TargetFrameworks))] + public async Task PerTestPropertiesAreScopedToTheirTest(string tfm) + { + var testHost = TestHost.LocateFrom(AssetFixture.ProjectPath, TestAssetFixture.ProjectName, tfm); + TestHostResult testHostResult = await testHost.ExecuteAsync( + "--filter ClassName=PerTestPropertyScopeTests", + cancellationToken: TestContext.CancellationToken); + + testHostResult.AssertExitCodeIs(0); + testHostResult.AssertOutputContainsSummary(failed: 0, passed: 2, skipped: 0); } public sealed class TestAssetFixture() : TestAssetFixtureBase() @@ -70,6 +84,7 @@ public sealed class PropertyFlowTests [AssemblyInitialize] public static void AssemblyInit(TestContext context) { + PropertyScopeAssertions.AssertNoPerTestProperties(context); context.Properties["AssemblyInitKey"] = "AssemblyInitValue"; context.Properties["SharedKey"] = "FromAssemblyInit"; } @@ -129,6 +144,7 @@ public static void ClassCleanup(TestContext context) [AssemblyCleanup] public static void AssemblyCleanup(TestContext context) { + PropertyScopeAssertions.AssertNoPerTestProperties(context); // AssemblyCleanup must see AssemblyInit-set properties, including any override the // AssemblyInit itself made. It must NOT see ClassInit-set properties (those are class-scoped). Assert.AreEqual("AssemblyInitValue", context.Properties["AssemblyInitKey"]); @@ -143,6 +159,104 @@ public static void AssemblyCleanup(TestContext context) } } +internal static class PropertyScopeAssertions +{ + private static readonly string[] PerTestPropertyKeys = + [ + "FirstTestProperty", + "FirstTestCategory", + "SecondTestProperty", + "SecondTestCategory", + ]; + + public static void AssertNoPerTestProperties(TestContext context) + { + foreach (string key in PerTestPropertyKeys) + { + Assert.IsFalse( + context.Properties.ContainsKey(key), + $"Per-test property '{key}' must not be visible in a lifecycle or sibling-test context."); + } + } +} + +// Regression coverage for https://github.com/microsoft/testfx/issues/10041. The test that +// triggers ClassInitialize may carry method-level properties, categories, or host metadata. +// Those values belong only to that test's execution context and must not be captured in the +// class lifecycle snapshot or flow to sibling tests. +[TestClass] +public sealed class PerTestPropertyScopeTests : System.IDisposable +{ + private readonly TestContext _testContext; + + public PerTestPropertyScopeTests(TestContext testContext) + { + _testContext = testContext; + AssertExpectedPerTestProperties(testContext); + } + + [ClassInitialize] + public static void ClassInit(TestContext context) + { + Assert.AreEqual("AssemblyInitValue", context.Properties["AssemblyInitKey"]); + PropertyScopeAssertions.AssertNoPerTestProperties(context); + context.Properties["PerTestScopeClassInitKey"] = "PerTestScopeClassInitValue"; + } + + [TestInitialize] + public void TestInit() => AssertExpectedPerTestProperties(_testContext); + + [TestMethod] + [TestProperty("FirstTestProperty", "FirstTestValue")] + [TestCategory("FirstTestCategory")] + public void TestWithFirstProperties() => AssertExpectedPerTestProperties(_testContext); + + [TestMethod] + [TestProperty("SecondTestProperty", "SecondTestValue")] + [TestCategory("SecondTestCategory")] + public void TestWithSecondProperties() => AssertExpectedPerTestProperties(_testContext); + + [TestCleanup] + public void TestCleanup() => AssertExpectedPerTestProperties(_testContext); + + public void Dispose() => AssertExpectedPerTestProperties(_testContext); + + [ClassCleanup] + public static void ClassCleanup(TestContext context) + { + Assert.AreEqual("AssemblyInitValue", context.Properties["AssemblyInitKey"]); + Assert.AreEqual("PerTestScopeClassInitValue", context.Properties["PerTestScopeClassInitKey"]); + PropertyScopeAssertions.AssertNoPerTestProperties(context); + } + + private static void AssertExpectedPerTestProperties(TestContext context) + { + Assert.AreEqual("AssemblyInitValue", context.Properties["AssemblyInitKey"]); + Assert.AreEqual("PerTestScopeClassInitValue", context.Properties["PerTestScopeClassInitKey"]); + + switch (context.TestName) + { + case nameof(TestWithFirstProperties): + Assert.AreEqual("FirstTestValue", context.Properties["FirstTestProperty"]); + Assert.IsTrue(context.Properties.ContainsKey("FirstTestCategory")); + Assert.IsFalse(context.Properties.ContainsKey("SecondTestProperty")); + Assert.IsFalse(context.Properties.ContainsKey("SecondTestCategory")); + break; + + case nameof(TestWithSecondProperties): + Assert.AreEqual("SecondTestValue", context.Properties["SecondTestProperty"]); + Assert.IsTrue(context.Properties.ContainsKey("SecondTestCategory")); + Assert.IsFalse(context.Properties.ContainsKey("FirstTestProperty")); + Assert.IsFalse(context.Properties.ContainsKey("FirstTestCategory")); + break; + + default: + Assert.Fail($"Unexpected test name '{context.TestName}'."); + break; + } + } +} + // Second class to verify that the assembly-init snapshot flows here too, AND that the // FIRST class's class-init snapshot does NOT leak into THIS class's contexts. [TestClass] diff --git a/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestExecutionManagerTests.cs b/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestExecutionManagerTests.cs index 18cfc4734a..379dd13fbc 100644 --- a/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestExecutionManagerTests.cs +++ b/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestExecutionManagerTests.cs @@ -319,6 +319,36 @@ public async Task RunTestsForTestShouldPassInTcmPropertiesAsPropertiesToTheTest( VerifyTcmProperties(DummyTestClass.TestContextProperties, testCase); } + public async Task RunTestsShouldKeepTcmPropertiesOutOfClassLifecycleAndSiblingTestContexts() + { + TestCase testWithTcmProperty = GetTestCase(typeof(DummyTestClassWithScopedTcmProperties), nameof(DummyTestClassWithScopedTcmProperties.TestWithTcmProperty)); + testWithTcmProperty.SetPropertyValue(EngineConstants.TestCaseIdProperty, 1401); + TestCase siblingTest = GetTestCase(typeof(DummyTestClassWithScopedTcmProperties), nameof(DummyTestClassWithScopedTcmProperties.SiblingTest)); + + TestablePlatformServiceProvider testablePlatformService = SetupTestablePlatformService(); + testablePlatformService.MockSettingsProvider + .Setup(sp => sp.GetProperties(It.IsAny())) + .Returns(new Dictionary { ["SourceProperty"] = "SourceValue" }); + _runContext.MockRunSettings.Setup(rs => rs.SettingsXml).Returns( + """ + + + True + + + """); + + await _testExecutionManager.RunTestsAsync( + [testWithTcmProperty, siblingTest], + _runContext, + _frameworkHandle, + new TestRunCancellationToken()); + + _frameworkHandle.TestCaseEndList.Should().Equal( + $"{nameof(DummyTestClassWithScopedTcmProperties.TestWithTcmProperty)}:Passed", + $"{nameof(DummyTestClassWithScopedTcmProperties.SiblingTest)}:Passed"); + } + public async Task RunTestsForTestShouldPassInDeploymentInformationAsPropertiesToTheTest() { TestCase testCase = GetTestCase(typeof(DummyTestClass), "PassingTest"); @@ -984,6 +1014,64 @@ internal class DummyTestClass public void IgnoredTest() => Assert.Fail(); } + [DummyTestClass] + [SuppressMessage("ApiDesign", "RS0030:Do not use banned APIs", Justification = "This is a MSTest sample class so it's expected to use MSTest assertions")] + private sealed class DummyTestClassWithScopedTcmProperties : IDisposable + { + private readonly TestContext _testContext; + + public DummyTestClassWithScopedTcmProperties(TestContext testContext) + { + _testContext = testContext; + AssertExpectedTestProperties(testContext); + } + + [ClassInitialize] + public static void ClassInitialize(TestContext context) + { + AssertSourceProperty(context); + Assert.IsFalse(context.Properties.ContainsKey(EngineConstants.TestCaseIdProperty.Id)); + } + + [TestInitialize] + public void TestInitialize() => AssertExpectedTestProperties(_testContext); + + [TestMethod] + public void TestWithTcmProperty() => AssertExpectedTestProperties(_testContext); + + [TestMethod] + public void SiblingTest() => AssertExpectedTestProperties(_testContext); + + [TestCleanup] + public void TestCleanup() => AssertExpectedTestProperties(_testContext); + + public void Dispose() => AssertExpectedTestProperties(_testContext); + + [ClassCleanup] + public static void ClassCleanup(TestContext context) + { + AssertSourceProperty(context); + Assert.IsFalse(context.Properties.ContainsKey(EngineConstants.TestCaseIdProperty.Id)); + } + + private static void AssertSourceProperty(TestContext context) + => Assert.AreEqual("SourceValue", context.Properties["SourceProperty"]); + + private static void AssertExpectedTestProperties(TestContext context) + { + AssertSourceProperty(context); + if (context.TestName == nameof(TestWithTcmProperty)) + { + Assert.AreEqual(1401, context.Properties[EngineConstants.TestCaseIdProperty.Id]); + } + else + { + Assert.AreEqual(nameof(SiblingTest), context.TestName); + Assert.IsFalse(context.Properties.ContainsKey(EngineConstants.TestCaseIdProperty.Id)); + } + } + } + [DummyTestClass] private class DummyTestClassWithFailingCleanupMethods { From 0310eb477c3d904e3e9d5dcc4d14ec5bce3976be Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Tue, 21 Jul 2026 17:02:59 +0200 Subject: [PATCH 10/15] Prevent FileLogger shutdown crash under thread-pool starvation by @Evangelink in #9802 (backport to rel/4.3) (#10114) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com> --- .../InternalAPI/InternalAPI.Unshipped.txt | 32 +++ .../Logging/FileLogger.cs | 109 +++++++- .../Logging/FileLoggerProvider.cs | 17 +- .../SingleConsumerUnboundedChannel.cs | 31 +++ .../Resources/PlatformResources.resx | 4 +- .../Resources/xlf/PlatformResources.cs.xlf | 6 +- .../Resources/xlf/PlatformResources.de.xlf | 6 +- .../Resources/xlf/PlatformResources.es.xlf | 6 +- .../Resources/xlf/PlatformResources.fr.xlf | 6 +- .../Resources/xlf/PlatformResources.it.xlf | 6 +- .../Resources/xlf/PlatformResources.ja.xlf | 6 +- .../Resources/xlf/PlatformResources.ko.xlf | 6 +- .../Resources/xlf/PlatformResources.pl.xlf | 6 +- .../Resources/xlf/PlatformResources.pt-BR.xlf | 6 +- .../Resources/xlf/PlatformResources.ru.xlf | 6 +- .../Resources/xlf/PlatformResources.tr.xlf | 6 +- .../xlf/PlatformResources.zh-Hans.xlf | 6 +- .../xlf/PlatformResources.zh-Hant.xlf | 6 +- .../Logging/FileLoggerTests.cs | 249 ++++++++++++++++++ 19 files changed, 462 insertions(+), 58 deletions(-) create mode 100644 src/Platform/Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt diff --git a/src/Platform/Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt b/src/Platform/Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt new file mode 100644 index 0000000000..348f70c72a --- /dev/null +++ b/src/Platform/Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt @@ -0,0 +1,32 @@ +#nullable enable +Microsoft.Testing.Platform.Logging.FileLogger.FileLogger(Microsoft.Testing.Platform.Logging.FileLoggerOptions! options, Microsoft.Testing.Platform.Logging.LogLevel logLevel, Microsoft.Testing.Platform.Helpers.IClock! clock, Microsoft.Testing.Platform.Helpers.ITask! task, Microsoft.Testing.Platform.Helpers.IConsole! console, Microsoft.Testing.Platform.Helpers.IFileSystem! fileSystem, Microsoft.Testing.Platform.Helpers.IFileStreamFactory! fileStreamFactory, System.TimeSpan? flushTimeout = null) -> void +Microsoft.Testing.Platform.Logging.FileLogger.IsFileHandleReleased.get -> bool +Microsoft.Testing.Platform.Messages.SingleConsumerUnboundedChannel.WaitToRead() -> bool +const Microsoft.Testing.Platform.ServerMode.JsonRpcStrings.IsStateful = "isStateful" -> string! +Microsoft.Testing.Platform.ServerMode.ClientCapabilities.ClientCapabilities(bool DebuggerProvider, bool IsStateful) -> void +Microsoft.Testing.Platform.ServerMode.ClientCapabilities.Deconstruct(out bool DebuggerProvider, out bool IsStateful) -> void +Microsoft.Testing.Platform.ServerMode.ClientCapabilities.IsStateful.get -> bool +Microsoft.Testing.Platform.ServerMode.ClientCapabilities.IsStateful.init -> void +Microsoft.Testing.Platform.Services.ClientCapabilitiesService +Microsoft.Testing.Platform.Services.ClientCapabilitiesService.$() -> Microsoft.Testing.Platform.Services.ClientCapabilitiesService! +Microsoft.Testing.Platform.Services.ClientCapabilitiesService.ClientCapabilitiesService(bool IsStateful) -> void +Microsoft.Testing.Platform.Services.ClientCapabilitiesService.Deconstruct(out bool IsStateful) -> void +Microsoft.Testing.Platform.Services.ClientCapabilitiesService.Equals(Microsoft.Testing.Platform.Services.ClientCapabilitiesService? other) -> bool +Microsoft.Testing.Platform.Services.ClientCapabilitiesService.IsStateful.get -> bool +Microsoft.Testing.Platform.Services.ClientCapabilitiesService.IsStateful.init -> void +Microsoft.Testing.Platform.Services.ClientInfoService.Capabilities.get -> Microsoft.Testing.Platform.Services.IClientCapabilities! +Microsoft.Testing.Platform.Services.ClientInfoService.Capabilities.init -> void +Microsoft.Testing.Platform.Services.ClientInfoService.ClientInfoService(string! Id, string! Version, Microsoft.Testing.Platform.Services.IClientCapabilities! Capabilities) -> void +Microsoft.Testing.Platform.Services.ClientInfoService.Deconstruct(out string! Id, out string! Version, out Microsoft.Testing.Platform.Services.IClientCapabilities! Capabilities) -> void +override Microsoft.Testing.Platform.Services.ClientCapabilitiesService.Equals(object? obj) -> bool +override Microsoft.Testing.Platform.Services.ClientCapabilitiesService.GetHashCode() -> int +override Microsoft.Testing.Platform.Services.ClientCapabilitiesService.ToString() -> string! +static Microsoft.Testing.Platform.Services.ClientCapabilitiesService.operator !=(Microsoft.Testing.Platform.Services.ClientCapabilitiesService? left, Microsoft.Testing.Platform.Services.ClientCapabilitiesService? right) -> bool +static Microsoft.Testing.Platform.Services.ClientCapabilitiesService.operator ==(Microsoft.Testing.Platform.Services.ClientCapabilitiesService? left, Microsoft.Testing.Platform.Services.ClientCapabilitiesService? right) -> bool +*REMOVED*Microsoft.Testing.Platform.ServerMode.ClientCapabilities.ClientCapabilities(bool DebuggerProvider) -> void +*REMOVED*Microsoft.Testing.Platform.Logging.FileLogger.FileLogger(Microsoft.Testing.Platform.Logging.FileLoggerOptions! options, Microsoft.Testing.Platform.Logging.LogLevel logLevel, Microsoft.Testing.Platform.Helpers.IClock! clock, Microsoft.Testing.Platform.Helpers.ITask! task, Microsoft.Testing.Platform.Helpers.IConsole! console, Microsoft.Testing.Platform.Helpers.IFileSystem! fileSystem, Microsoft.Testing.Platform.Helpers.IFileStreamFactory! fileStreamFactory) -> void +*REMOVED*Microsoft.Testing.Platform.ServerMode.ClientCapabilities.Deconstruct(out bool DebuggerProvider) -> void +*REMOVED*Microsoft.Testing.Platform.Services.ClientInfoService.ClientInfoService(string! Id, string! Version) -> void +*REMOVED*Microsoft.Testing.Platform.Services.ClientInfoService.Deconstruct(out string! Id, out string! Version) -> void +static Microsoft.Testing.Platform.IPC.Serializers.BaseSerializer.ReadFields(System.IO.Stream! stream, System.Func! tryReadField) -> void +static Microsoft.Testing.Platform.IPC.Serializers.BaseSerializer.WriteListPayload(System.IO.Stream! stream, ushort fieldId, T[]? list, System.Action! writeItem) -> void diff --git a/src/Platform/Microsoft.Testing.Platform/Logging/FileLogger.cs b/src/Platform/Microsoft.Testing.Platform/Logging/FileLogger.cs index 169ced3fd6..23b42eb82e 100644 --- a/src/Platform/Microsoft.Testing.Platform/Logging/FileLogger.cs +++ b/src/Platform/Microsoft.Testing.Platform/Logging/FileLogger.cs @@ -28,6 +28,7 @@ internal sealed class FileLogger : IDisposable private readonly IFileStream _fileStream; private readonly StreamWriter _writer; private readonly Task? _logLoop; + private readonly TimeSpan _flushTimeout; #if NETCOREAPP private readonly Channel? _channel; @@ -36,6 +37,14 @@ internal sealed class FileLogger : IDisposable #endif private bool _disposed; + /// + /// Gets a value indicating whether disposal fully drained the logs and released the underlying file handle. + /// This is when disposal timed out waiting for the flush: in that case the consumer loop + /// may still own the file, so callers that need exclusive access to it (e.g. to move the log file) must not + /// proceed. See the dispose methods and https://github.com/dotnet/sdk/issues/55215. + /// + public bool IsFileHandleReleased { get; private set; } + public FileLogger( FileLoggerOptions options, LogLevel logLevel, @@ -43,12 +52,14 @@ public FileLogger( ITask task, IConsole console, IFileSystem fileSystem, - IFileStreamFactory fileStreamFactory) + IFileStreamFactory fileStreamFactory, + TimeSpan? flushTimeout = null) { _options = options; _clock = clock; _logLevel = logLevel; _console = console; + _flushTimeout = flushTimeout ?? TimeoutHelper.DefaultHangTimeSpanTimeout; if (_options.SyncFlush) { @@ -74,8 +85,6 @@ public FileLogger( #else _channel = new SingleConsumerUnboundedChannel(); #endif - - _logLoop = task.Run(WriteLogToFileAsync, CancellationToken.None); } if (_options.FileName is not null) @@ -99,6 +108,26 @@ public FileLogger( { AutoFlush = true, }; + + // Start the consumer loop only after _writer is fully initialized. The loop dereferences _writer, so starting + // it earlier could race with the rest of the constructor and hit a null _writer (Task.Run may schedule the + // loop on another thread immediately). + if (!_options.SyncFlush) + { +#if NETCOREAPP + _logLoop = task.Run(WriteLogToFileAsync, CancellationToken.None); +#else + // On .NET Framework (the netstandard2.0 build) the FileLogger is disposed synchronously because there is + // no IAsyncDisposable / StreamWriter.DisposeAsync available at runtime, so Dispose blocks on this loop's + // task via _logLoop.Wait(). We therefore run a fully synchronous drain loop: passing a synchronous + // delegate to Task.Run means the whole loop runs to completion on a single worker thread and blocks on the + // channel without ever scheduling a continuation. This makes shutdown immune to thread-pool starvation + // (e.g. many test processes running concurrently on a CI / Helix agent), which could otherwise delay the + // async continuation past the flush timeout and crash the test host on an otherwise successful run. + // See https://github.com/dotnet/sdk/issues/55215. + _logLoop = task.Run(WriteLogToFile); +#endif + } } public string FileName { get; private set; } @@ -232,6 +261,7 @@ private void EnqueueLog(LogLevel logLevel, TState state, Exception? exce private string BuildLogEntry(LogLevel logLevel, TState state, Exception? exception, Func formatter, string category) => $"{_clock.UtcNow:O} {category} {logLevel.ToString().ToUpper(CultureInfo.InvariantCulture)} {formatter(state, exception)}"; +#if NETCOREAPP private async Task WriteLogToFileAsync() { // We do this check out of the try because we want to crash the process if the _channel is null. @@ -239,27 +269,43 @@ private async Task WriteLogToFileAsync() try { - // We don't need cancellation token because the task will be stopped when the Channel is completed thanks to the call to Complete() inside the Dispose method. -#if NETCOREAPP + // We don't need cancellation token because the task will be stopped when the Channel is completed thanks to the call to TryComplete() inside the Dispose/DisposeAsync method. while (await _channel.Reader.WaitToReadAsync().ConfigureAwait(false)) { await _writer.WriteLineAsync(await _channel.Reader.ReadAsync().ConfigureAwait(false)).ConfigureAwait(false); } + } + catch (Exception ex) + { + _console.WriteLine(string.Format(CultureInfo.InvariantCulture, PlatformResources.UnexpectedExceptionInFileLoggerErrorMessage, ex)); + } + } #else - while (await _channel.WaitToReadAsync(CancellationToken.None).ConfigureAwait(false)) + private void WriteLogToFile() + { + // We do this check out of the try because we want to crash the process if the _channel is null. + ApplicationStateGuard.Ensure(_channel is not null); + SingleConsumerUnboundedChannel channel = _channel; + + try + { + // We don't need a cancellation token because the loop stops when the channel is completed thanks to the + // call to Complete() inside the Dispose method. The wait and the writes are fully synchronous, so this + // loop never yields back to the thread pool and cannot be starved during shutdown. + while (channel.WaitToRead()) { - while (_channel.TryRead(out string message)) + while (channel.TryRead(out string message)) { - await _writer.WriteLineAsync(message).ConfigureAwait(false); + _writer.WriteLine(message); } } -#endif } catch (Exception ex) { _console.WriteLine(string.Format(CultureInfo.InvariantCulture, PlatformResources.UnexpectedExceptionInFileLoggerErrorMessage, ex)); } } +#endif [MemberNotNull(nameof(_channel), nameof(_logLoop))] private void EnsureAsyncLogObjectsAreNotNull() @@ -279,16 +325,29 @@ public void Dispose() { EnsureAsyncLogObjectsAreNotNull(); - // Wait for all logs to be written + // Signal the consumer that no more logs will be written, then wait for it to flush everything. #if NETCOREAPP _channel.Writer.TryComplete(); #else _channel.Complete(); #endif - if (!_logLoop.Wait(TimeoutHelper.DefaultHangTimeSpanTimeout)) + // A logger failing to flush must never crash an otherwise successful test run, so on timeout we warn and + // return instead of throwing. See https://github.com/dotnet/sdk/issues/55215. + if (!_logLoop.Wait(_flushTimeout)) { - throw new InvalidOperationException(string.Format(CultureInfo.InvariantCulture, PlatformResources.TimeoutFlushingLogsErrorMessage, TimeoutHelper.DefaultHangTimeoutSeconds)); + _console.WriteLine(string.Format(CultureInfo.InvariantCulture, PlatformResources.TimeoutFlushingLogsErrorMessage, _flushTimeout.TotalSeconds)); + + // The consumer loop is still running and owns _writer/_fileStream. Disposing them here would race + // with the loop (StreamWriter and its stream are not thread-safe), which could throw or truncate the + // log. We therefore leave them for the OS to reclaim at process exit and leave IsFileHandleReleased + // false so callers know the file handle is still held. Records already written are on disk (the writer + // uses AutoFlush); any records still queued may be lost if the process exits before the loop drains + // them. The semaphore is not shared with the consumer loop (it's only used on the SyncFlush path), so + // it's safe to dispose here. + _semaphore.Dispose(); + _disposed = true; + return; } } @@ -296,6 +355,7 @@ public void Dispose() _writer.Flush(); _writer.Dispose(); _fileStream.Dispose(); + IsFileHandleReleased = true; _disposed = true; } @@ -311,15 +371,36 @@ public async ValueTask DisposeAsync() { EnsureAsyncLogObjectsAreNotNull(); - // Wait for all logs to be written + // Wait for all logs to be written. A logger failing to flush must never crash an otherwise successful + // test run, so on timeout we warn and return instead of throwing. + // See https://github.com/dotnet/sdk/issues/55215. _channel.Writer.TryComplete(); - await _logLoop.TimeoutAfterAsync(TimeoutHelper.DefaultHangTimeSpanTimeout).ConfigureAwait(false); + try + { + await _logLoop.TimeoutAfterAsync(_flushTimeout).ConfigureAwait(false); + } + catch (TimeoutException) + { + _console.WriteLine(string.Format(CultureInfo.InvariantCulture, PlatformResources.TimeoutFlushingLogsErrorMessage, _flushTimeout.TotalSeconds)); + + // The consumer loop is still running and owns _writer/_fileStream. Disposing them here would race + // with the loop (StreamWriter and its stream are not thread-safe), which could throw or truncate the + // log. We therefore leave them for the OS to reclaim at process exit and leave IsFileHandleReleased + // false so callers know the file handle is still held. Records already written are on disk (the writer + // uses AutoFlush); any records still queued may be lost if the process exits before the loop drains + // them. The semaphore is not shared with the consumer loop (it's only used on the SyncFlush path), so + // it's safe to dispose here. + _semaphore.Dispose(); + _disposed = true; + return; + } } _semaphore.Dispose(); await _writer.FlushAsync().ConfigureAwait(false); await _writer.DisposeAsync().ConfigureAwait(false); await _fileStream.DisposeAsync().ConfigureAwait(false); + IsFileHandleReleased = true; _disposed = true; } #endif diff --git a/src/Platform/Microsoft.Testing.Platform/Logging/FileLoggerProvider.cs b/src/Platform/Microsoft.Testing.Platform/Logging/FileLoggerProvider.cs index b59a9bce0a..37745d4df4 100644 --- a/src/Platform/Microsoft.Testing.Platform/Logging/FileLoggerProvider.cs +++ b/src/Platform/Microsoft.Testing.Platform/Logging/FileLoggerProvider.cs @@ -53,11 +53,22 @@ public async Task CheckLogFolderAndMoveToTheNewIfNeededAsync(string testResultDi } string fileName = Path.GetFileName(FileLogger.FileName); - await DisposeHelper.DisposeAsync(FileLogger).ConfigureAwait(false); + FileLogger previousLogger = FileLogger; + string previousFileName = previousLogger.FileName; + await DisposeHelper.DisposeAsync(previousLogger).ConfigureAwait(false); - // Move the log file to the new directory - _fileSystem.MoveFile(FileLogger.FileName, Path.Combine(testResultDirectory, fileName)); + // If disposal completed cleanly, relocate the log file into the test result directory. If a flush timed out, + // the previous consumer loop may still own the file handle (the stream was opened with FileShare.Read, so a + // move would fail on Windows) — in that case we leave the old file in place and skip the move rather than + // turning a non-fatal flush timeout into a fatal IOException. See https://github.com/dotnet/sdk/issues/55215. + if (previousLogger.IsFileHandleReleased) + { + _fileSystem.MoveFile(previousFileName, Path.Combine(testResultDirectory, fileName)); + } + // Always install a fresh logger pointing at the test result directory so subsequent diagnostics keep working. + // The previous instance's channel is completed, so writing to it would throw; replacing it here keeps logging + // alive even on the degenerate timeout path (the old loop/handle are reclaimed at process exit). FileLogger = new FileLogger( new FileLoggerOptions(testResultDirectory, _options.LogPrefixName, fileName, _options.SyncFlush), LogLevel, diff --git a/src/Platform/Microsoft.Testing.Platform/Messages/SingleConsumerUnboundedChannel.cs b/src/Platform/Microsoft.Testing.Platform/Messages/SingleConsumerUnboundedChannel.cs index c2936dcfe3..353f69135e 100644 --- a/src/Platform/Microsoft.Testing.Platform/Messages/SingleConsumerUnboundedChannel.cs +++ b/src/Platform/Microsoft.Testing.Platform/Messages/SingleConsumerUnboundedChannel.cs @@ -43,6 +43,10 @@ public void Write(T item) _items.Enqueue(item); + // Wake up a consumer that is blocked inside the synchronous WaitToRead. + // This is a no-op when nobody is waiting on the monitor (e.g. consumers using WaitToReadAsync). + Monitor.Pulse(SyncObj); + // If WaitToReadAsync was called previously, we want to complete the task it returned. // We complete it with value true because we have an item that can be read now. if (_waitingReader is { } waitingReader) @@ -58,6 +62,29 @@ public void Write(T item) public bool TryRead(out T item) => _items.TryDequeue(out item); + /// + /// Synchronously blocks the calling thread until an item is available to read or the channel is completed. + /// Returns when there may be items to read, or when the channel + /// is completed and empty. + /// + /// + /// This is intended for a single dedicated consumer that drains the channel synchronously and therefore does not + /// use . Because the loop never yields back to the thread pool, it cannot be starved + /// (e.g. during process shutdown under heavy thread-pool contention). + /// + public bool WaitToRead() + { + lock (SyncObj) + { + while (_items.IsEmpty && !_completed) + { + Monitor.Wait(SyncObj); + } + + return !_items.IsEmpty; + } + } + public Task WaitToReadAsync(CancellationToken cancellationToken) { if (cancellationToken.IsCancellationRequested) @@ -110,6 +137,10 @@ public void Complete() { _completed = true; + // Wake up a consumer that is blocked inside the synchronous WaitToRead so it can observe completion. + // This is a no-op when nobody is waiting on the monitor (e.g. consumers using WaitToReadAsync). + Monitor.Pulse(SyncObj); + // If there was previously a call to WaitToReadAsync, and we had no items in the queue, and we are completing now. // Then there is nothing to read. So we set the task value to false. if (_waitingReader is { } waitingReader) diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/PlatformResources.resx b/src/Platform/Microsoft.Testing.Platform/Resources/PlatformResources.resx index eba7e4715e..2956cd5720 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/PlatformResources.resx +++ b/src/Platform/Microsoft.Testing.Platform/Resources/PlatformResources.resx @@ -338,9 +338,9 @@ {0} is the timeout in seconds. - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} Retry failed after {0} times diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.cs.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.cs.xlf index a8f18b45e4..04d3ea79cd 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.cs.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.cs.xlf @@ -966,11 +966,11 @@ Přečtěte si další informace o telemetrii Microsoft Testing Platform: https: - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - Ve FileLogger.WriteLogToFileAsync došlo k neočekávané výjimce. + Ve FileLogger.WriteLogToFileAsync došlo k neočekávané výjimce. {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.de.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.de.xlf index 3cc0f98381..45d2526dd4 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.de.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.de.xlf @@ -966,11 +966,11 @@ Weitere Informationen zu Microsoft Testing Platform-Telemetriedaten: https://aka - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - Unerwartete Ausnahme in "FileLogger.WriteLogToFileAsync". + Unerwartete Ausnahme in "FileLogger.WriteLogToFileAsync". {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.es.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.es.xlf index 9029a4b074..f212999a85 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.es.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.es.xlf @@ -966,11 +966,11 @@ Más información sobre la telemetría de la Plataforma de pruebas de Microsoft: - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - Se ha producido una excepción inesperada en “FileLogger.WriteLogToFileAsync”. + Se ha producido una excepción inesperada en “FileLogger.WriteLogToFileAsync”. {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.fr.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.fr.xlf index c8b87178f9..4864dccadf 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.fr.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.fr.xlf @@ -966,11 +966,11 @@ En savoir plus sur la télémétrie de la plateforme de tests Microsoft : https: - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - Une exception inattendue s’est produite dans « FileLogger.WriteLogToFileAsync ». + Une exception inattendue s’est produite dans « FileLogger.WriteLogToFileAsync ». {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.it.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.it.xlf index 0fc26c2e8e..48fed02e8d 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.it.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.it.xlf @@ -966,11 +966,11 @@ Altre informazioni sulla telemetria della piattaforma di test Microsoft: https:/ - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - Eccezione imprevista in 'FileLogger.WriteLogToFileAsync'. + Eccezione imprevista in 'FileLogger.WriteLogToFileAsync'. {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ja.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ja.xlf index 7b4ecde384..cff43c2c5c 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ja.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ja.xlf @@ -967,11 +967,11 @@ Microsoft Testing Platform テレメトリの詳細: https://aka.ms/testingplatf - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - 'FileLogger.WriteLogToFileAsync' で予期しない例外が発生しました。 + 'FileLogger.WriteLogToFileAsync' で予期しない例外が発生しました。 {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ko.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ko.xlf index b701d8459d..c214686f27 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ko.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ko.xlf @@ -966,11 +966,11 @@ Microsoft 테스트 플랫폼 원격 분석에 대해 자세히 알아보기: ht - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - 'FileLogger.WriteLogToFileAsync'에서 예기치 않은 예외가 발생했습니다. + 'FileLogger.WriteLogToFileAsync'에서 예기치 않은 예외가 발생했습니다. {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pl.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pl.xlf index 46ba603b81..1abdbcfc31 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pl.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pl.xlf @@ -966,11 +966,11 @@ Więcej informacji o telemetrii platformy testowej firmy Microsoft: https://aka. - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - Wystąpił nieoczekiwany wyjątek w elemencie „FileLogger.WriteLogToFileAsync”. + Wystąpił nieoczekiwany wyjątek w elemencie „FileLogger.WriteLogToFileAsync”. {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pt-BR.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pt-BR.xlf index af0ad62dbd..6d85d5fef4 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pt-BR.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pt-BR.xlf @@ -966,11 +966,11 @@ Leia mais sobre a telemetria da Plataforma de Testes da Microsoft: https://aka.m - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - Ocorreu uma exceção inesperada em "FileLogger.WriteLogToFileAsync". + Ocorreu uma exceção inesperada em "FileLogger.WriteLogToFileAsync". {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ru.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ru.xlf index a424a69ea7..fb81c8bb07 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ru.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ru.xlf @@ -966,11 +966,11 @@ Read more about Microsoft Testing Platform telemetry: https://aka.ms/testingplat - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - В "FileLogger.WriteLogToFileAsync" произошло непредвиденное исключение. + В "FileLogger.WriteLogToFileAsync" произошло непредвиденное исключение. {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.tr.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.tr.xlf index 3e1d58a17b..fd824f7f74 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.tr.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.tr.xlf @@ -966,11 +966,11 @@ Microsoft Test Platformu telemetrisi hakkında daha fazla bilgi edinin: https:// - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - 'FileLogger.WriteLogToFileAsync' içinde beklenmeyen bir özel durum oluştu. + 'FileLogger.WriteLogToFileAsync' içinde beklenmeyen bir özel durum oluştu. {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hans.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hans.xlf index 85a8ec4d5f..16f59335b8 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hans.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hans.xlf @@ -966,11 +966,11 @@ Microsoft 测试平台会收集使用数据，帮助我们改善体验。该数 - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - "FileLogger.WriteLogToFileAsync" 出现意外异常。 + "FileLogger.WriteLogToFileAsync" 出现意外异常。 {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hant.xlf b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hant.xlf index 0aea3dfde1..dcbd3db14f 100644 --- a/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hant.xlf +++ b/src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hant.xlf @@ -966,11 +966,11 @@ Microsoft 測試平台會收集使用量資料，以協助我們改善您的體 - An unexpected exception occurred in 'FileLogger.WriteLogToFileAsync'. + An unexpected exception occurred in the 'FileLogger' write loop. {0} - 'FileLogger.WriteLogToFileAsync' 發生未預期的例外狀況。 + 'FileLogger.WriteLogToFileAsync' 發生未預期的例外狀況。 {0} - {0} is the exception ToString output. {Locked="FileLogger.WriteLogToFileAsync"} + {0} is the exception ToString output. {Locked="FileLogger"} [ServerTestHost.OnTaskSchedulerUnobservedTaskException] Unhandled exception: {0} diff --git a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/FileLoggerTests.cs b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/FileLoggerTests.cs index cd6719e395..8802e283bb 100644 --- a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/FileLoggerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/FileLoggerTests.cs @@ -246,9 +246,258 @@ public void Log_WhenAsyncFlush_StreamWriterIsCalledOnlyWhenLogLevelAllowsIt(LogL } } + // Chaos test for https://github.com/dotnet/sdk/issues/55215. + // Stresses the async-flush path: many threads log concurrently and then the logger is disposed while messages are + // still queued. Repeated across many iterations to shake out races in consumer-loop startup and shutdown draining. + // The logger must never crash, must drain the whole queue on Dispose, and must not lose or corrupt any message. + [TestMethod] + public void Log_WhenAsyncFlush_ConcurrentLoggingIsDrainedOnDisposeWithoutLoss() + { + const int iterations = 50; + const int producerCount = 8; + const int messagesPerProducer = 50; + + var clock = new Mock(); + clock.Setup(x => x.UtcNow).Returns(new DateTimeOffset(2023, 5, 29, 3, 42, 13, TimeSpan.Zero)); + + for (int iteration = 0; iteration < iterations; iteration++) + { + using var memoryStream = new CustomMemoryStream(); + + var mockStream = new Mock(); + mockStream.Setup(x => x.Stream).Returns(memoryStream); + mockStream.Setup(x => x.Name).Returns(FileName); + mockStream.Setup(x => x.Dispose()); +#if NETCOREAPP + mockStream.Setup(x => x.DisposeAsync()).Returns(ValueTask.CompletedTask); +#endif + + var mockFileSystem = new Mock(); + mockFileSystem.Setup(x => x.ExistFile(It.IsAny())).Returns(false); + + var mockFileStreamFactory = new Mock(); + mockFileStreamFactory + .Setup(x => x.Create(It.IsAny(), FileMode.CreateNew, FileAccess.Write, FileShare.Read)) + .Returns(mockStream.Object); + + var fileLogger = new FileLogger( + new(LogFolder, LogPrefix, fileName: FileName, syncFlush: false), + LogLevel.Trace, + clock.Object, + new SystemTask(), + _mockConsole.Object, + mockFileSystem.Object, + mockFileStreamFactory.Object); + + // Release all producers at the same time to maximize contention right after construction. + using var startGate = new ManualResetEventSlim(false); + var producers = new Task[producerCount]; + for (int producer = 0; producer < producerCount; producer++) + { + int producerId = producer; + producers[producer] = Task.Run( + () => + { +#pragma warning disable CA1416 // ManualResetEventSlim.Wait is unsupported on 'browser' — this test never targets browser + startGate.Wait(TestContext.CancellationToken); +#pragma warning restore CA1416 + for (int message = 0; message < messagesPerProducer; message++) + { + fileLogger.Log(LogLevel.Trace, $"P{producerId}M{message}", null, Formatter, Category); + } + }, + TestContext.CancellationToken); + } + + startGate.Set(); +#pragma warning disable CA1416 // Task.WaitAll is unsupported on 'browser' — this test never targets browser + Task.WaitAll(producers, TestContext.CancellationToken); +#pragma warning restore CA1416 + + // Dispose must drain everything still sitting in the queue without crashing. + fileLogger.Dispose(); + + string content = Encoding.UTF8.GetString(memoryStream.ToArray()); + string[] lines = content.Split([Environment.NewLine], StringSplitOptions.RemoveEmptyEntries); + Assert.HasCount(producerCount * messagesPerProducer, lines, $"Iteration {iteration}: every queued message must be flushed exactly once."); + + // Compare against the exact set of expected messages. Each log line ends with the message payload + // (the log format is " "), so we extract the last token and + // require an exact set match. This detects loss/duplication even for prefix-overlapping IDs such as + // "P0M1" vs "P0M10", which a substring check would miss. + var actualMessages = new HashSet(lines.Select(line => line[(line.LastIndexOf(' ') + 1)..])); + for (int producer = 0; producer < producerCount; producer++) + { + for (int message = 0; message < messagesPerProducer; message++) + { + Assert.Contains($"P{producer}M{message}", actualMessages, $"Iteration {iteration}: message P{producer}M{message} was lost or corrupted."); + } + } + + Assert.HasCount(producerCount * messagesPerProducer, actualMessages, $"Iteration {iteration}: no duplicate or unexpected messages must be written."); + } + } + + // Deterministic guard for the fix in https://github.com/dotnet/sdk/issues/55215. + // On .NET Framework (the netstandard2.0 build) the consumer loop MUST be started with the synchronous + // ITask.Run(Action) overload so that Dispose()'s blocking Wait() can never be starved by the thread pool. + // The previous implementation used the asynchronous ITask.Run(Func, ...) overload, which is exactly the + // regression this test locks down. + [TestMethod] + public void FileLogger_WhenAsyncFlush_StartsConsumerLoopWithExpectedTaskOverload() + { + _mockFileSystem.Setup(x => x.ExistFile(It.IsAny())).Returns(false); + _mockFileStreamFactory + .Setup(x => x.Create(It.IsAny(), FileMode.CreateNew, FileAccess.Write, FileShare.Read)) + .Returns(_mockStream.Object); + + var recordingTask = new RecordingTask(); + using (FileLogger fileLogger = new( + new(LogFolder, LogPrefix, fileName: FileName, syncFlush: false), + LogLevel.Trace, + _mockClock.Object, + recordingTask, + _mockConsole.Object, + _mockFileSystem.Object, + _mockFileStreamFactory.Object)) + { + } + +#if NETCOREAPP + Assert.IsTrue(recordingTask.StartedAsynchronousLoop, "netcore must run the awaited async consumer loop."); + Assert.IsFalse(recordingTask.StartedSynchronousLoop); +#else + Assert.IsTrue(recordingTask.StartedSynchronousLoop, "netstandard must run a fully synchronous consumer loop that cannot be thread-pool starved during Dispose."); + Assert.IsFalse(recordingTask.StartedAsynchronousLoop); +#endif + } + + [TestMethod] + public void FileLogger_AfterSuccessfulDispose_ReportsFileHandleReleased() + { + _mockFileSystem.Setup(x => x.ExistFile(It.IsAny())).Returns(false); + _mockFileStreamFactory + .Setup(x => x.Create(It.IsAny(), FileMode.CreateNew, FileAccess.Write, FileShare.Read)) + .Returns(_mockStream.Object); + + FileLogger fileLogger = new( + new(LogFolder, LogPrefix, fileName: FileName, syncFlush: false), + LogLevel.Trace, + _mockClock.Object, + new SystemTask(), + _mockConsole.Object, + _mockFileSystem.Object, + _mockFileStreamFactory.Object); + + fileLogger.Log(LogLevel.Trace, Message, null, Formatter, Category); + fileLogger.Dispose(); + + Assert.IsTrue(fileLogger.IsFileHandleReleased); + } + + // Deterministic non-fatal-timeout test: the consumer loop never completes (simulating a hung flush), and a short + // injected flush timeout forces the timeout branch. Dispose must NOT throw, must warn, and must report that the + // file handle was not released so callers (e.g. FileLoggerProvider) can skip the file move. + [TestMethod] +#if NETCOREAPP + public async Task FileLogger_WhenFlushTimesOut_IsNonFatalAndReportsHandleNotReleased() +#else + public void FileLogger_WhenFlushTimesOut_IsNonFatalAndReportsHandleNotReleased() +#endif + { + _mockFileSystem.Setup(x => x.ExistFile(It.IsAny())).Returns(false); + _mockFileStreamFactory + .Setup(x => x.Create(It.IsAny(), FileMode.CreateNew, FileAccess.Write, FileShare.Read)) + .Returns(_mockStream.Object); + + FileLogger fileLogger = new( + new(LogFolder, LogPrefix, fileName: FileName, syncFlush: false), + LogLevel.Trace, + _mockClock.Object, + new NeverCompletingTask(), + _mockConsole.Object, + _mockFileSystem.Object, + _mockFileStreamFactory.Object, + flushTimeout: TimeSpan.FromMilliseconds(50)); + + fileLogger.Log(LogLevel.Trace, Message, null, Formatter, Category); + + // Must not throw even though the consumer loop never drains. +#if NETCOREAPP + await fileLogger.DisposeAsync(); +#else + fileLogger.Dispose(); +#endif + + Assert.IsFalse(fileLogger.IsFileHandleReleased, "A flush timeout must leave the file handle owned by the still-running consumer."); + _mockConsole.Verify(x => x.WriteLine(It.Is(s => s.Contains("Failed to flush logs"))), Times.Once); + } + void IDisposable.Dispose() => _memoryStream.Dispose(); + // ITask that records which overload was used to start the file-logger consumer loop, delegating the actual work + // to the real SystemTask. + private sealed class RecordingTask : ITask + { + private readonly ITask _inner = new SystemTask(); + + public bool StartedSynchronousLoop { get; private set; } + + public bool StartedAsynchronousLoop { get; private set; } + + public Task Run(Func function, CancellationToken cancellationToken) + { + StartedAsynchronousLoop = true; + return _inner.Run(function, cancellationToken); + } + + public Task Run(Action action) + { + StartedSynchronousLoop = true; + return _inner.Run(action); + } + + public Task Run(Func?> function, CancellationToken cancellationToken) + => _inner.Run(function, cancellationToken); + + [UnsupportedOSPlatform("browser")] + public Task RunLongRunning(Func action, string name, CancellationToken cancellationToken) + => _inner.RunLongRunning(action, name, cancellationToken); + + public Task WhenAll(params Task[] tasks) => _inner.WhenAll(tasks); + + public Task Delay(int millisecondDelay) => _inner.Delay(millisecondDelay); + + public Task Delay(TimeSpan timeSpan, CancellationToken cancellationToken) => _inner.Delay(timeSpan, cancellationToken); + } + + // ITask whose loop-starting overloads return a task that never completes, simulating a hung consumer so the + // dispose-time flush timeout is deterministically triggered without running any real loop. + private sealed class NeverCompletingTask : ITask + { + private readonly ITask _inner = new SystemTask(); + + public Task Run(Func function, CancellationToken cancellationToken) + => new TaskCompletionSource().Task; + + public Task Run(Action action) + => new TaskCompletionSource().Task; + + public Task Run(Func?> function, CancellationToken cancellationToken) + => _inner.Run(function, cancellationToken); + + [UnsupportedOSPlatform("browser")] + public Task RunLongRunning(Func action, string name, CancellationToken cancellationToken) + => _inner.RunLongRunning(action, name, cancellationToken); + + public Task WhenAll(params Task[] tasks) => _inner.WhenAll(tasks); + + public Task Delay(int millisecondDelay) => _inner.Delay(millisecondDelay); + + public Task Delay(TimeSpan timeSpan, CancellationToken cancellationToken) => _inner.Delay(timeSpan, cancellationToken); + } + private sealed class CustomMemoryStream : MemoryStream { private bool _shouldDispose; From c6a51c677fc8f1c9621439a06258bb7fa26f47da Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Wed, 22 Jul 2026 10:40:58 +0200 Subject: [PATCH 11/15] Inherit Arcade default DebugType instead of forcing embedded symbols by @Evangelink in #10006 (backport to rel/4.3) (#10125) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé --- Directory.Build.props | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Directory.Build.props b/Directory.Build.props index baeca8286a..005d777492 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -47,7 +47,13 @@ false true - embedded + 0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7 From 75514b609c06df6cb715314ecc98b2e70f76c487 Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Wed, 22 Jul 2026 13:36:46 +0200 Subject: [PATCH 12/15] Fix VSTestBridge BuildFilter misclassifying a GUID-shaped FullyQualifiedName as an Id filter by @Evangelink in #9794 (backport to rel/4.3) (#10113) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ObjectModel/ContextAdapterBase.cs | 23 ++++++--- .../ObjectModel/DiscoveryContextAdapter.cs | 7 ++- .../ObjectModel/RunContextAdapter.cs | 7 ++- ...TestDiscoverTestExecutionRequestFactory.cs | 2 +- .../VSTestRunTestExecutionRequestFactory.cs | 2 +- .../ObjectModel/RunContextAdapterTests.cs | 47 +++++++++++++++++++ 6 files changed, 77 insertions(+), 11 deletions(-) diff --git a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ContextAdapterBase.cs b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ContextAdapterBase.cs index ff7c703644..fc9f682436 100644 --- a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ContextAdapterBase.cs +++ b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ContextAdapterBase.cs @@ -15,6 +15,11 @@ namespace Microsoft.Testing.Extensions.VSTestBridge.ObjectModel; internal abstract class ContextAdapterBase { protected ContextAdapterBase(ICommandLineOptions commandLineOptions, IRunSettings runSettings, ITestExecutionFilter filter) + : this(commandLineOptions, runSettings, filter, useFullyQualifiedNameAsUid: false) + { + } + + protected ContextAdapterBase(ICommandLineOptions commandLineOptions, IRunSettings runSettings, ITestExecutionFilter filter, bool useFullyQualifiedNameAsUid) { RunSettings = runSettings; @@ -31,7 +36,7 @@ protected ContextAdapterBase(ICommandLineOptions commandLineOptions, IRunSetting filterFromCommandLineOption = filterExpressions[0]; } - HandleFilter(filter, filterFromRunsettings, filterFromCommandLineOption); + HandleFilter(filter, filterFromRunsettings, filterFromCommandLineOption, useFullyQualifiedNameAsUid); } public IRunSettings? RunSettings { get; } @@ -76,7 +81,7 @@ public bool MatchTestCase(TestCase testCase, Func propertyValue return new BridgeFilterExpression(new TestCaseFilterExpression(FilterExpressionWrapper)); } - private void HandleFilter(ITestExecutionFilter? filter, string? filterFromRunsettings, string? filterFromCommandLineOption) + private void HandleFilter(ITestExecutionFilter? filter, string? filterFromRunsettings, string? filterFromCommandLineOption, bool useFullyQualifiedNameAsUid) { // No filters at all, we can return immediately as there is nothing to do. if (filter is null or NopFilter @@ -94,7 +99,7 @@ private void HandleFilter(ITestExecutionFilter? filter, string? filterFromRunset if (filter is TestNodeUidListFilter testNodeUidListFilter) { StartFilter(filterBuilder); - BuildFilter(testNodeUidListFilter.TestNodeUids, filterBuilder); + BuildFilter(testNodeUidListFilter.TestNodeUids, filterBuilder, useFullyQualifiedNameAsUid); EndFilter(filterBuilder); } @@ -132,9 +137,13 @@ static void EndFilter(StringBuilder builder) => builder.Append(')'); } - // We use heuristic to understand if the filter should be a TestCaseId or FullyQualifiedName. - // We know that in VSTest TestCaseId is a GUID and FullyQualifiedName is a string. - private static void BuildFilter(TestNodeUid[] testNodesUid, StringBuilder filter) + // The UID value is produced by ObjectModelConverters.ToTestNode, which sets it to either + // TestCase.FullyQualifiedName or TestCase.Id depending on useFullyQualifiedNameAsUid. We use that + // same discriminator here to decide the clause type, rather than guessing from the value with + // Guid.TryParse. Guessing is wrong for a FullyQualifiedName that happens to be GUID-shaped (e.g. a + // data-driven test whose display name is exactly a GUID string), which would then be emitted as an + // Id= clause and select the wrong test (or no test). + private static void BuildFilter(TestNodeUid[] testNodesUid, StringBuilder filter, bool useFullyQualifiedNameAsUid) { for (int i = 0; i < testNodesUid.Length; i++) { @@ -143,7 +152,7 @@ private static void BuildFilter(TestNodeUid[] testNodesUid, StringBuilder filter filter.Append('|'); } - if (Guid.TryParse(testNodesUid[i].Value, out Guid guid)) + if (!useFullyQualifiedNameAsUid && Guid.TryParse(testNodesUid[i].Value, out Guid guid)) { filter.Append("Id="); filter.Append(guid.ToString()); diff --git a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/DiscoveryContextAdapter.cs b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/DiscoveryContextAdapter.cs index 10f44494d1..09391c902a 100644 --- a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/DiscoveryContextAdapter.cs +++ b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/DiscoveryContextAdapter.cs @@ -13,7 +13,12 @@ namespace Microsoft.Testing.Extensions.VSTestBridge.ObjectModel; internal sealed class DiscoveryContextAdapter : ContextAdapterBase, IDiscoveryContext { public DiscoveryContextAdapter(ICommandLineOptions commandLineOptions, IRunSettings runSettings, ITestExecutionFilter filter) - : base(commandLineOptions, runSettings, filter) + : this(commandLineOptions, runSettings, filter, useFullyQualifiedNameAsUid: false) + { + } + + public DiscoveryContextAdapter(ICommandLineOptions commandLineOptions, IRunSettings runSettings, ITestExecutionFilter filter, bool useFullyQualifiedNameAsUid) + : base(commandLineOptions, runSettings, filter, useFullyQualifiedNameAsUid) { } } diff --git a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/RunContextAdapter.cs b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/RunContextAdapter.cs index b15c57db8f..7a31eccf4c 100644 --- a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/RunContextAdapter.cs +++ b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/RunContextAdapter.cs @@ -14,7 +14,12 @@ namespace Microsoft.Testing.Extensions.VSTestBridge.ObjectModel; internal sealed class RunContextAdapter : ContextAdapterBase, IRunContext { public RunContextAdapter(ICommandLineOptions commandLineOptions, IRunSettings runSettings, ITestExecutionFilter filter) - : base(commandLineOptions, runSettings, filter) + : this(commandLineOptions, runSettings, filter, useFullyQualifiedNameAsUid: false) + { + } + + public RunContextAdapter(ICommandLineOptions commandLineOptions, IRunSettings runSettings, ITestExecutionFilter filter, bool useFullyQualifiedNameAsUid) + : base(commandLineOptions, runSettings, filter, useFullyQualifiedNameAsUid) { RoslynDebug.Assert(runSettings.SettingsXml is not null); diff --git a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/Requests/VSTestDiscoverTestExecutionRequestFactory.cs b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/Requests/VSTestDiscoverTestExecutionRequestFactory.cs index f55db4ceb0..e90315583c 100644 --- a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/Requests/VSTestDiscoverTestExecutionRequestFactory.cs +++ b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/Requests/VSTestDiscoverTestExecutionRequestFactory.cs @@ -34,7 +34,7 @@ public static VSTestDiscoverTestExecutionRequest CreateRequest( requestContext.ClientInfo, requestContext.LoggerFactory, messageLogger); - DiscoveryContextAdapter discoveryContext = new(requestContext.CommandLineOptions, runSettings, discoverTestExecutionRequest.Filter); + DiscoveryContextAdapter discoveryContext = new(requestContext.CommandLineOptions, runSettings, discoverTestExecutionRequest.Filter, adapterExtension.UseFullyQualifiedNameAsTestNodeUid); TestCaseDiscoverySinkAdapter discoverySink = new( adapterExtension, diff --git a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/Requests/VSTestRunTestExecutionRequestFactory.cs b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/Requests/VSTestRunTestExecutionRequestFactory.cs index 69937ade41..ee8e30411c 100644 --- a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/Requests/VSTestRunTestExecutionRequestFactory.cs +++ b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/Requests/VSTestRunTestExecutionRequestFactory.cs @@ -47,7 +47,7 @@ public static VSTestRunTestExecutionRequest CreateRequest( requestContext.ClientInfo, requestContext.LoggerFactory, frameworkHandlerAdapter); - RunContextAdapter runContext = new(requestContext.CommandLineOptions, runSettings, runTestExecutionRequest.Filter); + RunContextAdapter runContext = new(requestContext.CommandLineOptions, runSettings, runTestExecutionRequest.Filter, adapterExtension.UseFullyQualifiedNameAsTestNodeUid); return new(runTestExecutionRequest.Session, runTestExecutionRequest.Filter, testAssemblyPaths, runContext, frameworkHandlerAdapter); } diff --git a/test/UnitTests/Microsoft.Testing.Extensions.VSTestBridge.UnitTests/ObjectModel/RunContextAdapterTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.VSTestBridge.UnitTests/ObjectModel/RunContextAdapterTests.cs index ed5de3ba72..eed74505fc 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.VSTestBridge.UnitTests/ObjectModel/RunContextAdapterTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.VSTestBridge.UnitTests/ObjectModel/RunContextAdapterTests.cs @@ -3,6 +3,7 @@ using Microsoft.Testing.Extensions.VSTestBridge.ObjectModel; using Microsoft.Testing.Platform.CommandLine; +using Microsoft.Testing.Platform.Extensions.Messages; using Microsoft.Testing.Platform.Requests; using Microsoft.VisualStudio.TestPlatform.ObjectModel.Adapter; @@ -51,4 +52,50 @@ public void TestRunDirectory_IsNull_If_ResultsDirectory_IsNot_Provided() Assert.IsNull(runContextAdapter.TestRunDirectory); Assert.IsNotNull(runContextAdapter.RunSettings); } + + [TestMethod] + public void BuildFilter_WhenUsingFullyQualifiedNameAsUid_GuidShapedName_EmitsFullyQualifiedNameClause() + { + _runSettings.Setup(x => x.SettingsXml).Returns(""); + + // A data-driven test whose FullyQualifiedName happens to be exactly a GUID-shaped string. + const string GuidShapedFullyQualifiedName = "12345678-1234-1234-1234-1234567890ab"; + var filter = new TestNodeUidListFilter([new TestNodeUid(GuidShapedFullyQualifiedName)]); + + RunContextAdapter runContextAdapter = new(_commandLineOptions.Object, _runSettings.Object, filter, useFullyQualifiedNameAsUid: true); + + ITestCaseFilterExpression? filterExpression = runContextAdapter.GetTestCaseFilter(null, _ => null); + Assert.IsNotNull(filterExpression); + Assert.AreEqual($"(FullyQualifiedName={GuidShapedFullyQualifiedName})", filterExpression.TestCaseFilterValue); + } + + [TestMethod] + public void BuildFilter_WhenNotUsingFullyQualifiedNameAsUid_GuidValue_EmitsIdClause() + { + _runSettings.Setup(x => x.SettingsXml).Returns(""); + + const string GuidValue = "12345678-1234-1234-1234-1234567890ab"; + var filter = new TestNodeUidListFilter([new TestNodeUid(GuidValue)]); + + RunContextAdapter runContextAdapter = new(_commandLineOptions.Object, _runSettings.Object, filter, useFullyQualifiedNameAsUid: false); + + ITestCaseFilterExpression? filterExpression = runContextAdapter.GetTestCaseFilter(null, _ => null); + Assert.IsNotNull(filterExpression); + Assert.AreEqual($"(Id={GuidValue})", filterExpression.TestCaseFilterValue); + } + + [TestMethod] + public void BuildFilter_WhenUsingFullyQualifiedNameAsUid_NonGuidName_EmitsFullyQualifiedNameClause() + { + _runSettings.Setup(x => x.SettingsXml).Returns(""); + + const string FullyQualifiedName = "MyNamespace.MyClass.MyTest"; + var filter = new TestNodeUidListFilter([new TestNodeUid(FullyQualifiedName)]); + + RunContextAdapter runContextAdapter = new(_commandLineOptions.Object, _runSettings.Object, filter, useFullyQualifiedNameAsUid: true); + + ITestCaseFilterExpression? filterExpression = runContextAdapter.GetTestCaseFilter(null, _ => null); + Assert.IsNotNull(filterExpression); + Assert.AreEqual($"(FullyQualifiedName={FullyQualifiedName})", filterExpression.TestCaseFilterValue); + } } From 2e21c0894edd4863404809c484786a82b5a191cf Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Thu, 23 Jul 2026 01:31:38 +0200 Subject: [PATCH 13/15] Support comments in testconfig on .NET Framework by @Evangelink in #10144 (backport to rel/4.3) (#10148) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé --- ...JsonConfigurationFileParser.netstandard.cs | 1 + .../JsonRpc/Json/Jsonite/JsonReader.cs | 60 ++++++++++++++++--- .../JsonRpc/Json/Jsonite/JsonTypes.cs | 5 ++ .../ConfigurationManagerTests.cs | 6 ++ .../ServerMode/JsoniteTests.cs | 28 +++++++++ 5 files changed, 92 insertions(+), 8 deletions(-) diff --git a/src/Platform/Microsoft.Testing.Platform/Configurations/JsonConfigurationFileParser.netstandard.cs b/src/Platform/Microsoft.Testing.Platform/Configurations/JsonConfigurationFileParser.netstandard.cs index 78f5bdd36d..33248842f4 100644 --- a/src/Platform/Microsoft.Testing.Platform/Configurations/JsonConfigurationFileParser.netstandard.cs +++ b/src/Platform/Microsoft.Testing.Platform/Configurations/JsonConfigurationFileParser.netstandard.cs @@ -18,6 +18,7 @@ internal sealed class JsonConfigurationFileParser private readonly Stack _paths = new(); private readonly JsonSettings _settings = new() { + AllowComments = true, AllowTrailingCommas = true, }; diff --git a/src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Jsonite/JsonReader.cs b/src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Jsonite/JsonReader.cs index fe62182c0b..d91df0e19e 100644 --- a/src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Jsonite/JsonReader.cs +++ b/src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Jsonite/JsonReader.cs @@ -445,11 +445,7 @@ private object ParseNumber() } while (IsDigit(c)); } - // Skip any whitespaces after a value - while (IsWhiteSpace(c)) - { - NextChar(); - } + SkipWhitespacesAndComments(); // If we are expecting to parse only things into strings, early exit here if (settings.ParseValuesAsStrings) @@ -604,10 +600,58 @@ private void RaiseUnexpected(string message) private void NextCharSkipWhitespaces() { - do + NextChar(); + SkipWhitespacesAndComments(); + } + + private void SkipWhitespacesAndComments() + { + while (true) { - NextChar(); - } while (IsWhiteSpace(c)); + while (IsWhiteSpace(c)) + { + NextChar(); + } + + if (!settings.AllowComments || c != '/') + { + return; + } + + switch (Reader.Peek()) + { + case '/': + do + { + NextChar(); + } + while (!isEof && c is not ('\r' or '\n')); + break; + + case '*': + NextChar(); + while (true) + { + NextChar(); + if (isEof) + { + RaiseUnexpected("while parsing a comment. Expecting the end of a comment '*/'"); + } + + if (c == '*' && Reader.Peek() == '/') + { + NextChar(); + NextChar(); + break; + } + } + + break; + + default: + return; + } + } } [MethodImpl((MethodImplOptions)256)] diff --git a/src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Jsonite/JsonTypes.cs b/src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Jsonite/JsonTypes.cs index 9901899b7b..c2c1a0ff14 100644 --- a/src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Jsonite/JsonTypes.cs +++ b/src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Jsonite/JsonTypes.cs @@ -166,6 +166,11 @@ public JsonSettings() /// public bool ParseValuesAsStrings { get; set; } + /// + /// Gets or sets a value indicating whether to allow comments. + /// + public bool AllowComments { get; set; } + /// /// Gets or sets a value indicating whether to allow trailing commas in object and array declaration. /// diff --git a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Configuration/ConfigurationManagerTests.cs b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Configuration/ConfigurationManagerTests.cs index 4ccf182c50..293566ee26 100644 --- a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Configuration/ConfigurationManagerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Configuration/ConfigurationManagerTests.cs @@ -45,6 +45,12 @@ public async ValueTask GetConfigurationValueFromJson(string jsonFileConfig, stri yield return ("{\"platformOptions\": [1,2] }", "platformOptions:0", "1"); yield return ("{\"platformOptions\": [1,2] }", "platformOptions:1", "2"); yield return ("{\"platformOptions\": [1,2] }", "platformOptions", "[1,2]"); + yield return ("{// Configure crash dumps\n\"platformOptions\": {\"Troubleshooting\": {\"CrashDump\": {\"Enable\": true}}}}", "platformOptions:Troubleshooting:CrashDump:Enable", "True"); + yield return ("{// Comment containing \0 a null character\n\"platformOptions\": {\"Troubleshooting\": {\"CrashDump\": {\"Enable\": true}}}}", "platformOptions:Troubleshooting:CrashDump:Enable", "True"); + yield return ("{\"platformOptions\": {/* Configure crash dumps */\"Troubleshooting\": {\"CrashDump\": {\"Enable\": true}}}}", "platformOptions:Troubleshooting:CrashDump:Enable", "True"); + yield return ("{\"platformOptions\": {/* Comment containing \0 a null character */\"Troubleshooting\": {\"CrashDump\": {\"Enable\": true}}}}", "platformOptions:Troubleshooting:CrashDump:Enable", "True"); + yield return ("{\"platformOptions\": {\"Count\": 1 /* Number of retries */}}", "platformOptions:Count", "1"); + yield return ("{\"platformOptions\": {\"Url\": \"https://example.com\"}}", "platformOptions:Url", "https://example.com"); yield return ("{\"platformOptions\": { \"Array\" : [ {\"Key\" : \"Value\"} , {\"Key\" : 3} ] } }", "platformOptions:Array:0", null); yield return ("{\"platformOptions\": { \"Array\" : [ {\"Key\" : \"Value\"} , {\"Key\" : 3} ] } }", "platformOptions:Array:0:Key", "Value"); yield return ("{\"platformOptions\": { \"Array\" : [ {\"Key\" : \"Value\"} , {\"Key\" : 3} ] } }", "platformOptions:Array:1:Key", "3"); diff --git a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/ServerMode/JsoniteTests.cs b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/ServerMode/JsoniteTests.cs index 745ceef92f..bd44022f58 100644 --- a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/ServerMode/JsoniteTests.cs +++ b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/ServerMode/JsoniteTests.cs @@ -17,6 +17,34 @@ public void Serialize_DateTimeOffset() Assert.AreEqual("2023-01-01T01:01:01.0010000+00:00", actual.Trim('"')); } + [TestMethod] + public void Deserialize_CommentsAreDisallowedByDefault() + => Assert.ThrowsExactly(() => Jsonite.Json.Deserialize("{// Comment\n\"value\": true}")); + + [TestMethod] + public void Deserialize_CommentsCanBeAllowed() + { + Jsonite.JsonSettings settings = new() + { + AllowComments = true, + }; + + var result = (Jsonite.JsonObject)Jsonite.Json.Deserialize("{// Line comment\n\"value\": /* Block comment */ true}", settings); + + Assert.IsTrue((bool)result["value"]!); + } + + [TestMethod] + public void Deserialize_UnterminatedBlockCommentThrows() + { + Jsonite.JsonSettings settings = new() + { + AllowComments = true, + }; + + Assert.ThrowsExactly(() => Jsonite.Json.Deserialize("{/* Comment", settings)); + } + [TestMethod] public void SerializeJsoniteInvalidStringHighSurrogateAtTheEnd() { From 48a92eeb65da3132b388db22d5e4b5ed12e87299 Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Thu, 23 Jul 2026 16:41:37 +0200 Subject: [PATCH 14/15] Restore bounded string difference indicators by @Evangelink in #10145 (backport to rel/4.3) (#10172) Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com> --- changes.patch | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 changes.patch diff --git a/changes.patch b/changes.patch new file mode 100644 index 0000000000..151206ca99 --- /dev/null +++ b/changes.patch @@ -0,0 +1,83 @@ + + + + + + + Too many requests · GitHub + + + + + +
+ +

Too many requests

+

You have exceeded a secondary rate limit.

+ Please wait a few minutes before you try again;
+ in some cases this may take up to an hour.
+ Signing in may provide a higher rate limit if you are not already signed in.

+ For more on scraping GitHub and how it may affect your rights, please review our Terms of Service. +

+ + + + + + +
+ + From 44aa76e6a61d4908f06dfd77a51d4b7e3e7ce40f Mon Sep 17 00:00:00 2001 From: nohwnd-bot Date: Fri, 24 Jul 2026 16:25:29 +0200 Subject: [PATCH 15/15] Fix duplicate data consumer disposal during teardown by @Evangelink in #10195 (backport to rel/4.3) (#10197) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com> Copilot-Session: 336d2284-26e0-42ed-9772-cf91bc8eeb6b --- changes.patch | 83 ------------------- .../Hosts/CommonTestHost.cs | 2 +- .../Hosts/CommonHostTests.cs | 20 +++++ 3 files changed, 21 insertions(+), 84 deletions(-) delete mode 100644 changes.patch diff --git a/changes.patch b/changes.patch deleted file mode 100644 index 151206ca99..0000000000 --- a/changes.patch +++ /dev/null @@ -1,83 +0,0 @@ - - - - - - - Too many requests · GitHub - - - - - -
- -

Too many requests

-

You have exceeded a secondary rate limit.

- Please wait a few minutes before you try again;
- in some cases this may take up to an hour.
- Signing in may provide a higher rate limit if you are not already signed in.

- For more on scraping GitHub and how it may affect your rights, please review our Terms of Service. -

- - - - - - -
- - diff --git a/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs b/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs index 887fe2a83f..a97be879e5 100644 --- a/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs +++ b/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs @@ -408,7 +408,7 @@ IPlatformOpenTelemetryService or if (!alreadyDisposed.Contains(dataConsumer)) { await DisposeHelper.DisposeAsync(dataConsumer).ConfigureAwait(false); - alreadyDisposed.Add(service); + alreadyDisposed.Add(dataConsumer); } } } diff --git a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Hosts/CommonHostTests.cs b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Hosts/CommonHostTests.cs index b8d5f76a62..75d935c422 100644 --- a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Hosts/CommonHostTests.cs +++ b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Hosts/CommonHostTests.cs @@ -89,12 +89,32 @@ public async Task RunAsync_WhenTestHostApplicationLifetimeIsAsyncCleanable_Clean Assert.AreEqual(1, testApplicationLifetime.CleanupCount); } + [TestMethod] + public async Task DisposeServiceProviderAsync_WhenDataConsumerIsAlsoRegisteredAsService_DisposesOnce() + { + Mock dataConsumer = new(); + Mock disposableDataConsumer = dataConsumer.As(); + Mock messageBus = new(); + messageBus.SetupGet(x => x.DataConsumerServices).Returns([dataConsumer.Object]); + + ServiceProvider serviceProvider = new(); + serviceProvider.AddService(messageBus.Object); + serviceProvider.AddService(dataConsumer.Object); + + await TestableCommonHost.DisposeServiceProviderForTestingAsync(serviceProvider); + + disposableDataConsumer.Verify(x => x.Dispose(), Times.Once); + } + private sealed class TestableCommonHost(ServiceProvider serviceProvider, bool runTestApplicationLifeCycleCallbacks = false) : CommonHost(serviceProvider) { protected override string HostType => "TestHost"; protected override bool RunTestApplicationLifeCycleCallbacks => runTestApplicationLifeCycleCallbacks; + public static Task DisposeServiceProviderForTestingAsync(ServiceProvider serviceProvider) + => DisposeServiceProviderAsync(serviceProvider); + public static Task ExecuteRequestForTestingAsync( ProxyOutputDevice outputDevice, ITestSessionContext testSessionInfo,