From 0483d85f2be7c35ac7c5c2b8dab86796d2e89116 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Mon, 3 Aug 2026 10:40:52 +0200 Subject: [PATCH] Clear the efficiency-improver backlog: OTel tags and analyzer member lookup Two of the four code-level items on the [efficiency-improver] August backlog are worth acting on; the rest are closed out as won't-fix on the issue. OpenTelemetryResultHandler.GetTestInitialInfo walked the property bag three times per test on the OTel path - SingleOrDefault, SingleOrDefault and OfType, the last of which materializes a TProperty[] purely to enumerate it once. It now uses the struct enumerator: one pass for the two singleton properties and one for the metadata, which has to stay separate because the metadata tags are emitted after the identifier and file-location blocks. Two walks instead of three, and no array. The duplicate-property detection SingleOrDefault provided is kept explicitly, matching what SetResultDetails already does right below, and is now covered by tests. DynamicDataShouldBeValidAnalyzer.TryGetMemberCore scanned the candidate members twice (FirstOrDefault for a property, then Where(...).ToImmutableArray() for the methods) and allocated an ImmutableArray per [DynamicData] attribute. A single switch over the members finds the property, the first method and the more-than-one-method case in one pass. Analyzers re-run on every keystroke in the IDE, so this is not only compile-time work. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a158d927-2e59-4c53-92be-d17b1ec8200b --- .../DynamicDataShouldBeValidAnalyzer.cs | 38 ++++++++++++----- .../Telemetry/OpenTelemetryResultHandler.cs | 42 +++++++++++++++++-- .../OpenTelemetryResultHandlerTests.cs | 25 ++++++++++- 3 files changed, 90 insertions(+), 15 deletions(-) diff --git a/src/Analyzers/MSTest.Analyzers/DynamicDataShouldBeValidAnalyzer.cs b/src/Analyzers/MSTest.Analyzers/DynamicDataShouldBeValidAnalyzer.cs index a34ea62743..d14d6ce2bb 100644 --- a/src/Analyzers/MSTest.Analyzers/DynamicDataShouldBeValidAnalyzer.cs +++ b/src/Analyzers/MSTest.Analyzers/DynamicDataShouldBeValidAnalyzer.cs @@ -179,20 +179,36 @@ private static (ISymbol? Member, bool AreTooMany) TryGetMember(INamedTypeSymbol return (null, false); } - ISymbol? potentialProperty = potentialMembers.FirstOrDefault(m => m.Kind == SymbolKind.Property); - if (potentialProperty is not null) + // Single pass over the members: a property always wins, otherwise the first method is used and + // more than one method is an error. Doing this with FirstOrDefault + Where().ToImmutableArray() + // scanned the members twice and allocated an array per [DynamicData] attribute per compilation. + ISymbol? firstMethod = null; + bool hasMultipleMethods = false; + foreach (ISymbol member in potentialMembers) { - return (potentialProperty, false); - } - - var candidateMethods = potentialMembers.Where(m => m.Kind == SymbolKind.Method).ToImmutableArray(); - if (candidateMethods.Length > 1) - { - // If there are multiple methods with the same name, report a diagnostic. This is not a supported scenario. - return (null, true); + switch (member.Kind) + { + case SymbolKind.Property: + return (member, false); + + case SymbolKind.Method: + if (firstMethod is null) + { + firstMethod = member; + } + else + { + hasMultipleMethods = true; + } + + break; + } } - return (candidateMethods.IsEmpty ? potentialMembers[0] : candidateMethods[0], false); + // If there are multiple methods with the same name, report a diagnostic. This is not a supported scenario. + return hasMultipleMethods + ? (null, true) + : (firstMethod ?? potentialMembers[0], false); } } diff --git a/src/Platform/Microsoft.Testing.Platform/Telemetry/OpenTelemetryResultHandler.cs b/src/Platform/Microsoft.Testing.Platform/Telemetry/OpenTelemetryResultHandler.cs index ac2ebfe7af..7304aa1ca8 100644 --- a/src/Platform/Microsoft.Testing.Platform/Telemetry/OpenTelemetryResultHandler.cs +++ b/src/Platform/Microsoft.Testing.Platform/Telemetry/OpenTelemetryResultHandler.cs @@ -318,7 +318,35 @@ private static string GetRunResultStatus(int failedTests, int exitCode) } } - if (testNode.Properties.SingleOrDefault() is { } identifierProperty) + // Single pass over the property bag for the two singleton properties below: two separate + // SingleOrDefault() calls walked the whole bag once each. + TestMethodIdentifierProperty? identifierProperty = null; + TestFileLocationProperty? testLocationProperty = null; + PropertyBag.PropertyBagEnumerator enumerator = testNode.Properties.GetStructEnumerator(); + while (enumerator.MoveNext()) + { + switch (enumerator.Current) + { + case TestMethodIdentifierProperty identifier: + if (identifierProperty is not null) + { + throw new InvalidOperationException($"Found multiple properties of type '{typeof(TestMethodIdentifierProperty)}'."); + } + + identifierProperty = identifier; + break; + case TestFileLocationProperty location: + if (testLocationProperty is not null) + { + throw new InvalidOperationException($"Found multiple properties of type '{typeof(TestFileLocationProperty)}'."); + } + + testLocationProperty = location; + break; + } + } + + if (identifierProperty is not null) { // code.function.name is defined as the *fully qualified* name; there is no separate namespace // attribute (code.namespace is deprecated upstream). @@ -335,7 +363,7 @@ private static string GetRunResultStatus(int failedTests, int exitCode) } } - if (testNode.Properties.SingleOrDefault() is { } testLocationProperty) + if (testLocationProperty is not null) { yield return new(TestingPlatformSemanticConventions.Attributes.CodeFilePath, testLocationProperty.FilePath); yield return new(TestingPlatformSemanticConventions.Attributes.CodeLineNumber, testLocationProperty.LineSpan.Start.Line); @@ -348,8 +376,16 @@ private static string GetRunResultStatus(int failedTests, int exitCode) } } - foreach (TestMetadataProperty metadata in testNode.Properties.OfType()) + // The metadata is yielded after the blocks above, so it needs its own pass. OfType() + // would materialize a TProperty[] just to enumerate it once; the struct enumerator allocates nothing. + PropertyBag.PropertyBagEnumerator metadataEnumerator = testNode.Properties.GetStructEnumerator(); + while (metadataEnumerator.MoveNext()) { + if (metadataEnumerator.Current is not TestMetadataProperty metadata) + { + continue; + } + yield return new KeyValuePair($"{TestingPlatformSemanticConventions.Attributes.TestMetadataPrefix}{metadata.Key}", metadata.Value); if (_options.EmitLegacyAttributes) { diff --git a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Telemetry/OpenTelemetryResultHandlerTests.cs b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Telemetry/OpenTelemetryResultHandlerTests.cs index 50062b0704..60207daf45 100644 --- a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Telemetry/OpenTelemetryResultHandlerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Telemetry/OpenTelemetryResultHandlerTests.cs @@ -721,6 +721,29 @@ public void NotifyInProgress_ForTheGlobalNamespace_DoesNotEmitALeadingDot(string } private IEnumerable>? CaptureInProgressTags(TestMethodIdentifierProperty identifierProperty) + => CaptureInProgressTags(new PropertyBag(identifierProperty)); + + [TestMethod] + public void NotifyInProgress_WithDuplicateIdentifierProperty_Throws() + { + InvalidOperationException exception = Assert.ThrowsExactly(() => CaptureInProgressTags(new PropertyBag( + new TestMethodIdentifierProperty("MyAssembly", "My.Namespace", "MyClass", "MyMethod", 0, [], "void"), + new TestMethodIdentifierProperty("MyAssembly", "My.Namespace", "MyClass", "MyOtherMethod", 0, [], "void")))); + + Assert.AreEqual($"Found multiple properties of type '{typeof(TestMethodIdentifierProperty)}'.", exception.Message); + } + + [TestMethod] + public void NotifyInProgress_WithDuplicateFileLocationProperty_Throws() + { + InvalidOperationException exception = Assert.ThrowsExactly(() => CaptureInProgressTags(new PropertyBag( + new TestFileLocationProperty("first.cs", new LinePositionSpan(new LinePosition(1, 0), new LinePosition(2, 0))), + new TestFileLocationProperty("second.cs", new LinePositionSpan(new LinePosition(3, 0), new LinePosition(4, 0)))))); + + Assert.AreEqual($"Found multiple properties of type '{typeof(TestFileLocationProperty)}'.", exception.Message); + } + + private IEnumerable>? CaptureInProgressTags(PropertyBag properties) { IEnumerable>? capturedTags = null; _otelService.Setup(s => s.StartActivity( @@ -736,7 +759,7 @@ public void NotifyInProgress_ForTheGlobalNamespace_DoesNotEmitALeadingDot(string { Uid = new TestNodeUid("fqn"), DisplayName = "Test", - Properties = new PropertyBag(identifierProperty), + Properties = properties, }, null);