Add telemetry collection for MSTest usage analytics#7570
Add telemetry collection for MSTest usage analytics#7570Evangelink wants to merge 9 commits intomainfrom
Conversation
Add infrastructure to collect aggregated telemetry about MSTest usage: - Track assertion API usage (Assert, CollectionAssert, StringAssert) - Track attribute usage and custom types during discovery - Track MSTest configuration settings and source - Send telemetry via MTP telemetry collector on session exit Key implementation details: - TelemetryCollector: thread-safe static counter in TestFramework using ConcurrentDictionary with atomic swap-and-drain pattern - MSTestTelemetryDataCollector: aggregates discovery and assertion data, builds metrics, anonymizes custom type names via SHA256 - Telemetry is opt-in via MTP telemetry infrastructure (respects TESTINGPLATFORM_TELEMETRY_OPTOUT) - VSTest mode collects but discards data (no telemetry sender available) Fixes applied during review: - Fix race condition in DrainAssertionCallCounts using Interlocked.Exchange - Fix thread safety of Current property using Volatile.Read/Write - Add synchronous SendTelemetryAndReset to avoid deadlock in sync callers - Use IDictionary<string, object> delegate type for consistency with MTP - Replace bare catch with catch (Exception) - Remove duplicate _serviceProvider field (use inherited ServiceProvider) - Add missing ContainsSingle telemetry tracking - Replace string interpolation with string.Concat in hot paths - Standardize blank lines after TrackAssertionCall calls
There was a problem hiding this comment.
Pull request overview
This PR adds telemetry collection infrastructure for MSTest usage analytics, tracking assertion API usage, attribute usage, custom types (anonymized via SHA256), and configuration settings. Data is collected during test discovery and execution and sent via MTP's telemetry system on session exit. In VSTest mode, data is collected but silently discarded.
Changes:
- Adds
TelemetryCollector(static, thread-safe assertion counter) in TestFramework andMSTestTelemetryDataCollector(aggregates discovery + assertion data) in the adapter - Instruments all assertion methods across
Assert,CollectionAssert, andStringAssertwithTrackAssertionCallcalls - Integrates telemetry initialization and sending into
MSTestDiscoverer,MSTestExecutor, andMSTestBridgedTestFramework
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
TelemetryCollector.cs |
New static class with thread-safe assertion call counting using ConcurrentDictionary and atomic drain |
MSTestTelemetryDataCollector.cs |
New class aggregating settings, attribute usage, custom types, and assertion counts into telemetry metrics |
Assert.*.cs (multiple files) |
Added TrackAssertionCall at entry points of assertion methods |
CollectionAssert.cs |
Added TrackAssertionCall for collection assertion methods |
StringAssert.cs |
Added TrackAssertionCall for string assertion methods |
MSTestExecutor.cs |
Initializes telemetry collector, sends telemetry in finally block after test execution |
MSTestDiscoverer.cs |
Initializes telemetry collector, sends telemetry synchronously after discovery |
MSTestBridgedTestFramework.cs |
Creates MTP telemetry sender delegate for MTP mode |
MSTestSettings.cs |
Tracks configuration source (testconfig.json/runsettings/none) for telemetry |
TypeEnumerator.cs / AssemblyEnumerator.cs |
Passes telemetry collector through discovery pipeline |
Microsoft.Testing.Platform.csproj |
Adds InternalsVisibleTo for MSTest.TestAdapter |
TelemetryTests.cs |
Integration tests for MTP and VSTest telemetry scenarios |
You can also share your feedback on Copilot code review. Take the survey.
src/Adapter/MSTestAdapter.PlatformServices/Telemetry/MSTestTelemetryDataCollector.cs
Outdated
Show resolved
Hide resolved
Convert.ToHexString is .NET 5+ only. Use BitConverter.ToString with Replace for the .NET Framework code path, matching the existing #if NET split for SHA256.
Remove the synchronous SendTelemetryAndReset overload from MSTestTelemetryDataCollector. The blocking call now lives in MSTestDiscoverer (the only sync caller), wrapped in Task.Run to avoid SynchronizationContext deadlocks.
There was a problem hiding this comment.
Pull request overview
Adds MSTest session-level usage telemetry collection (assertion API usage, attribute usage, custom attribute subclasses, and configuration/settings source) and wires it into both MTP-hosted and VSTest-hosted execution/discovery flows, with new acceptance coverage for MTP scenarios.
Changes:
- Introduces
TelemetryCollectorin MSTest.TestFramework to track assertion call counts. - Adds
MSTestTelemetryDataCollectorin the adapter to aggregate settings/config source, discovery attributes, anonymized custom types, and drained assertion counts into telemetry metrics. - Integrates telemetry initialization/sending into discovery/execution paths (MTP bridge + VSTest adapter) and adds MTP acceptance tests.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TelemetryTests.cs | Adds acceptance coverage to validate telemetry emission/absence via diagnostic logs. |
| src/TestFramework/TestFramework/Internal/TelemetryCollector.cs | New internal assertion-call counter with drain/reset semantics. |
| src/TestFramework/TestFramework/Assertions/StringAssert.cs | Adds telemetry tracking for StringAssert APIs. |
| src/TestFramework/TestFramework/Assertions/CollectionAssert.cs | Adds telemetry tracking for CollectionAssert APIs. |
| src/TestFramework/TestFramework/Assertions/Assert.ThrowsException.cs | Adds telemetry tracking for Throws* APIs using CallerMemberName. |
| src/TestFramework/TestFramework/Assertions/Assert.StartsWith.cs | Adds telemetry tracking for StartsWith/DoesNotStartWith. |
| src/TestFramework/TestFramework/Assertions/Assert.Matches.cs | Adds telemetry tracking for MatchesRegex/DoesNotMatchRegex. |
| src/TestFramework/TestFramework/Assertions/Assert.IsTrue.cs | Adds telemetry tracking for IsTrue/IsFalse. |
| src/TestFramework/TestFramework/Assertions/Assert.IsNull.cs | Adds telemetry tracking for IsNull/IsNotNull. |
| src/TestFramework/TestFramework/Assertions/Assert.IsInstanceOfType.cs | Adds telemetry tracking for IsInstanceOfType/IsNotInstanceOfType. |
| src/TestFramework/TestFramework/Assertions/Assert.IsExactInstanceOfType.cs | Adds telemetry tracking for IsExactInstanceOfType/IsNotExactInstanceOfType. |
| src/TestFramework/TestFramework/Assertions/Assert.Inconclusive.cs | Adds telemetry tracking for Inconclusive. |
| src/TestFramework/TestFramework/Assertions/Assert.IComparable.cs | Adds telemetry tracking for IComparable-based asserts. |
| src/TestFramework/TestFramework/Assertions/Assert.Fail.cs | Adds telemetry tracking for Fail. |
| src/TestFramework/TestFramework/Assertions/Assert.EndsWith.cs | Adds telemetry tracking for EndsWith/DoesNotEndWith. |
| src/TestFramework/TestFramework/Assertions/Assert.Count.cs | Adds telemetry tracking for count/empty asserts. |
| src/TestFramework/TestFramework/Assertions/Assert.Contains.cs | Adds telemetry tracking for Contains/DoesNotContain/IsInRange APIs. |
| src/TestFramework/TestFramework/Assertions/Assert.AreSame.cs | Adds telemetry tracking for AreSame/AreNotSame. |
| src/TestFramework/TestFramework/Assertions/Assert.AreEqual.cs | Adds telemetry tracking for AreEqual/AreNotEqual overloads. |
| src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj | Grants InternalsVisibleTo MSTest.TestAdapter for internal MTP telemetry extension access. |
| src/Adapter/MSTestAdapter.PlatformServices/Telemetry/MSTestTelemetryDataCollector.cs | New adapter-side aggregator that builds metrics and sends/discards telemetry. |
| src/Adapter/MSTestAdapter.PlatformServices/MSTestSettings.cs | Records telemetry configuration source (runsettings/testconfig.json/none). |
| src/Adapter/MSTestAdapter.PlatformServices/Discovery/TypeEnumerator.cs | Hooks discovery to record method/class attribute usage into telemetry collector. |
| src/Adapter/MSTestAdapter.PlatformServices/Discovery/AssemblyEnumerator.cs | Passes the current telemetry collector into TypeEnumerator. |
| src/Adapter/MSTest.TestAdapter/VSTestAdapter/MSTestExecutor.cs | Initializes collector for runs and triggers telemetry send/reset at run end. |
| src/Adapter/MSTest.TestAdapter/VSTestAdapter/MSTestDiscoverer.cs | Initializes collector for discovery and triggers telemetry send/reset at discovery end. |
| src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestBridgedTestFramework.cs | Creates an MTP telemetry sender delegate and injects it into discoverer/executor. |
Comments suppressed due to low confidence (1)
src/Adapter/MSTestAdapter.PlatformServices/Telemetry/MSTestTelemetryDataCollector.cs:267
- Same issue in the synchronous
SendTelemetryAndReset: the early return ontelemetrySender is null/HasDatameans assertion counters are never drained/reset when telemetry is unavailable, so counts can leak across sessions and the dictionary can grow unbounded in long-lived processes. Consider ensuring counters are reset even when telemetry is discarded.
You can also share your feedback on Copilot code review. Take the survey.
| MSTestTelemetryDataCollector? collector = Current; | ||
| if (collector is not { HasData: true } || telemetrySender is null) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[AI generated] Fixed in commit cbf9140. The early-return path now calls TelemetryCollector.DrainAssertionCallCounts() to prevent memory growth even when telemetry sending is skipped. The assertion counters are always drained regardless of sender availability.
| // Initialize telemetry collection if not already set | ||
| MSTestTelemetryDataCollector.Current ??= new MSTestTelemetryDataCollector(); | ||
|
|
There was a problem hiding this comment.
[AI generated] The HasData flag is set to true by TrackDiscoveredMethod and TrackDiscoveredClass during discovery, which happens before execution for both the TestCase and sources paths. Settings metrics are always added by BuildMetrics unconditionally. The early-return path now drains assertion counters (fixed in cbf9140), so there's no memory leak risk. The current design is: if no test classes/methods were discovered (HasData=false), we don't send the telemetry event but still clean up counters.
| /// <summary> | ||
| /// Gets a snapshot of all assertion call counts and resets the counters. | ||
| /// This is thread-safe: it atomically swaps the dictionary and drains the old one. | ||
| /// </summary> | ||
| /// <returns>A dictionary mapping assertion names to call counts.</returns> | ||
| internal static Dictionary<string, long> DrainAssertionCallCounts() | ||
| { | ||
| ConcurrentDictionary<string, long> old = Interlocked.Exchange(ref s_assertionCallCounts, new ConcurrentDictionary<string, long>()); | ||
| #pragma warning disable IDE0028 // Simplify collection initialization - ConcurrentDictionary snapshot copy | ||
| return new Dictionary<string, long>(old); |
There was a problem hiding this comment.
[AI generated] Fixed in 6259d0e. Updated the XML doc to explicitly state that counts are best-effort and in-flight calls racing with the swap may be lost. This is acceptable for telemetry where approximate counts are sufficient.
| /// <param name="attributes">The cached attributes from the class.</param> | ||
| internal void TrackDiscoveredClass(Type classType, Attribute[] attributes) | ||
| { | ||
| HasData = true; |
There was a problem hiding this comment.
[AI generated] Fixed in 6259d0e. Removed the unused classType parameter from TrackDiscoveredClass and updated the call site in TypeEnumerator.
Add IsTelemetryOptedOut() to MSTestTelemetryDataCollector that checks the same environment variables as MTP's TelemetryManager: - TESTINGPLATFORM_TELEMETRY_OPTOUT - DOTNET_CLI_TELEMETRY_OPTOUT When either is '1' or 'true', skip initializing the collector entirely in VSTest mode (MSTestDiscoverer + MSTestExecutor). This avoids unnecessary data collection overhead when telemetry is disabled. In MTP mode, the opt-out is already handled by TelemetryManager which returns null from CreateTelemetrySender() when disabled.
Address the most important correctness issues in the telemetry branch: - drain assertion counters even when telemetry is not sent, avoiding stale usage leaking into later sessions - make collector initialization atomic with Interlocked.CompareExchange - track Assert.That and interpolated-string-handler assertion paths so modern call sites are no longer systematically undercounted
There was a problem hiding this comment.
Pull request overview
Adds opt-in telemetry collection to MSTest to capture aggregated usage analytics (assertion APIs, attributes, custom types, and settings/config source) and wires it through the adapter for MTP sessions, with integration tests validating end-to-end behavior.
Changes:
- Added a framework-level
TelemetryCollectorand instrumented many assertion APIs to increment aggregated counters. - Added an adapter-side
MSTestTelemetryDataCollectorthat aggregates discovery/settings + drained assertion counts and sends a session-exit telemetry event (MTP only). - Added acceptance integration tests for MTP telemetry-enabled/disabled scenarios and VSTest regression coverage; added
InternalsVisibleToto enable adapter ↔ platform integration.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TelemetryTests.cs | New acceptance tests validating telemetry emission (MTP) and ensuring no regressions in VSTest mode. |
| src/TestFramework/TestFramework/Internal/TelemetryCollector.cs | New internal aggregated counter store with swap-and-drain for assertion usage. |
| src/TestFramework/TestFramework/Assertions/StringAssert.cs | Adds telemetry tracking to StringAssert APIs. |
| src/TestFramework/TestFramework/Assertions/CollectionAssert.cs | Adds telemetry tracking to CollectionAssert APIs. |
| src/TestFramework/TestFramework/Assertions/Assert.ThrowsException.cs | Adds telemetry tracking to throws-related Assert APIs using caller member name. |
| src/TestFramework/TestFramework/Assertions/Assert.That.cs | Adds telemetry tracking to Assert.That. |
| src/TestFramework/TestFramework/Assertions/Assert.StartsWith.cs | Adds telemetry tracking to starts-with asserts. |
| src/TestFramework/TestFramework/Assertions/Assert.Matches.cs | Adds telemetry tracking to regex asserts. |
| src/TestFramework/TestFramework/Assertions/Assert.IsTrue.cs | Adds telemetry tracking to IsTrue/IsFalse (including interpolated handler compute paths). |
| src/TestFramework/TestFramework/Assertions/Assert.IsNull.cs | Adds telemetry tracking to IsNull/IsNotNull (including interpolated handler compute paths). |
| src/TestFramework/TestFramework/Assertions/Assert.IsInstanceOfType.cs | Adds telemetry tracking to instance-of asserts (including interpolated handler compute paths). |
| src/TestFramework/TestFramework/Assertions/Assert.IsExactInstanceOfType.cs | Adds telemetry tracking to exact-instance-of asserts (including interpolated handler compute paths). |
| src/TestFramework/TestFramework/Assertions/Assert.Inconclusive.cs | Adds telemetry tracking to Assert.Inconclusive. |
| src/TestFramework/TestFramework/Assertions/Assert.IComparable.cs | Adds telemetry tracking to comparable-based asserts. |
| src/TestFramework/TestFramework/Assertions/Assert.Fail.cs | Adds telemetry tracking to Assert.Fail. |
| src/TestFramework/TestFramework/Assertions/Assert.EndsWith.cs | Adds telemetry tracking to ends-with asserts. |
| src/TestFramework/TestFramework/Assertions/Assert.Count.cs | Adds telemetry tracking to count/empty asserts. |
| src/TestFramework/TestFramework/Assertions/Assert.Contains.cs | Adds telemetry tracking to contains/range asserts. |
| src/TestFramework/TestFramework/Assertions/Assert.AreSame.cs | Adds telemetry tracking to same/not-same asserts (including interpolated handler compute paths). |
| src/TestFramework/TestFramework/Assertions/Assert.AreEqual.cs | Adds telemetry tracking to equal/not-equal asserts (including interpolated handler compute paths). |
| src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj | Adds InternalsVisibleTo for MSTest.TestAdapter to support MTP telemetry integration. |
| src/Adapter/MSTestAdapter.PlatformServices/Telemetry/MSTestTelemetryDataCollector.cs | New adapter-side collector that aggregates settings/discovery + drained assertion counts and sends metrics. |
| src/Adapter/MSTestAdapter.PlatformServices/MSTestSettings.cs | Captures config source into telemetry collector during settings population. |
| src/Adapter/MSTestAdapter.PlatformServices/Discovery/TypeEnumerator.cs | Plumbs telemetry collector into discovery to track class/method attributes. |
| src/Adapter/MSTestAdapter.PlatformServices/Discovery/AssemblyEnumerator.cs | Passes current telemetry collector into type enumerator creation. |
| src/Adapter/MSTest.TestAdapter/VSTestAdapter/MSTestExecutor.cs | Initializes and flushes telemetry on test-run completion (sender injected for MTP). |
| src/Adapter/MSTest.TestAdapter/VSTestAdapter/MSTestDiscoverer.cs | Initializes and flushes telemetry on discovery completion (sender injected for MTP). |
| src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestBridgedTestFramework.cs | Provides MTP telemetry sender and injects it into discoverer/executor. |
You can also share your feedback on Copilot code review. Take the survey.
| private static ConcurrentDictionary<string, long> s_assertionCallCounts = new(); | ||
|
|
||
| /// <summary> | ||
| /// Records that an assertion method was called. | ||
| /// </summary> | ||
| /// <param name="assertionName">The full name of the assertion (e.g. "Assert.AreEqual", "CollectionAssert.Contains").</param> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| internal static void TrackAssertionCall(string assertionName) | ||
| => s_assertionCallCounts.AddOrUpdate(assertionName, 1, static (_, count) => count + 1); | ||
|
|
||
| /// <summary> | ||
| /// Gets a snapshot of all assertion call counts and resets the counters. | ||
| /// This is thread-safe: it atomically swaps the dictionary and drains the old one. | ||
| /// </summary> | ||
| /// <returns>A dictionary mapping assertion names to call counts.</returns> | ||
| internal static Dictionary<string, long> DrainAssertionCallCounts() | ||
| { | ||
| ConcurrentDictionary<string, long> old = Interlocked.Exchange(ref s_assertionCallCounts, new ConcurrentDictionary<string, long>()); | ||
| #pragma warning disable IDE0028 // Simplify collection initialization - ConcurrentDictionary snapshot copy | ||
| return new Dictionary<string, long>(old); | ||
| #pragma warning restore IDE0028 // Simplify collection initialization | ||
| } |
There was a problem hiding this comment.
[AI generated] Same as the earlier thread — fixed in 6259d0e by updating the XML doc to state counts are best-effort. The Interlocked.Exchange + new Dictionary pattern is intentionally simple for telemetry. Adding retry/volatile-read logic would add complexity disproportionate to the value for approximate usage metrics.
| string trackingName = attribute switch | ||
| { | ||
| TestClassAttribute => nameof(TestClassAttribute), | ||
| ParallelizeAttribute => nameof(ParallelizeAttribute), | ||
| DoNotParallelizeAttribute => nameof(DoNotParallelizeAttribute), | ||
| _ => null!, | ||
| }; | ||
|
|
||
| if (trackingName is not null) | ||
| { | ||
| _attributeCounts[trackingName] = _attributeCounts.TryGetValue(trackingName, out long count) | ||
| ? count + 1 | ||
| : 1; | ||
| } |
There was a problem hiding this comment.
[AI generated] Fixed in 6259d0e. Changed to string? trackingName with _ => null instead of null!.
| if (telemetryOptOut is "1" or "true") | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| string? cliTelemetryOptOut = Environment.GetEnvironmentVariable("DOTNET_CLI_TELEMETRY_OPTOUT"); | ||
|
|
||
| return cliTelemetryOptOut is "1" or "true"; | ||
| } | ||
|
|
There was a problem hiding this comment.
[AI generated] This is by design — the case-sensitive check for "true" mirrors MTP's TelemetryManager behavior exactly (see Microsoft.Testing.Platform.Telemetry.TelemetryManager). Keeping them consistent avoids scenarios where MTP thinks telemetry is disabled but MSTest doesn't (or vice versa). If MTP changes to case-insensitive in the future, this should follow suit.
| string diagPath = Path.Combine(AssetFixture.MTPProjectPath, "bin", "Release", tfm, TestResultsFolderName); | ||
| string diagPathPattern = Path.Combine(diagPath, @"log_.*.diag").Replace(@"\", @"\\"); | ||
|
|
There was a problem hiding this comment.
[AI generated] Valid concern in theory, but this follows the exact same pattern used by many other acceptance tests in this codebase (e.g., see AssertDiagnosticReportAsync in other test files). The generated asset paths are controlled test paths without regex metacharacters. Changing this would deviate from the established pattern across the test suite. Won't fix for consistency.
| (bool success, string content) = await CheckDiagnosticContentsMatchAsync(match.Value, diagContentsPattern); | ||
| Assert.IsTrue(success, $"{content}\n{diagContentsPattern}"); | ||
|
|
||
| return match.Value; | ||
| } | ||
|
|
||
| private static async Task<(bool IsMatch, string Content)> CheckDiagnosticContentsMatchAsync(string path, string pattern) | ||
| { | ||
| using var reader = new StreamReader(path); | ||
| string content = await reader.ReadToEndAsync(); | ||
|
|
There was a problem hiding this comment.
[AI generated] This pattern (single read after process exit) is used consistently across all acceptance tests in the codebase that check diagnostic files. The test host process has fully exited (with exit code assertion) before we read the file, so the async flush should have completed. Adding retry/poll would deviate from the established pattern. Won't fix for consistency with the rest of the test suite.
| // Verify attribute usage and config source are also present | ||
| string content = await File.ReadAllTextAsync(diagFilePath, TestContext.CancellationToken); | ||
| Assert.IsTrue(Regex.IsMatch(content, "mstest\\.attribute_usage"), $"Expected attribute_usage in telemetry.\n{content}"); | ||
| Assert.IsTrue(Regex.IsMatch(content, "mstest\\.config_source"), $"Expected config_source in telemetry.\n{content}"); |
There was a problem hiding this comment.
[AI generated] Fixed in 6259d0e. Added Assert.IsTrue(Regex.IsMatch(content, "mstest\\.assertion_usage"), ...) to MTP_RunTests_SendsTelemetryWithSettingsAndAttributes — this verifies assertion tracking is wired end-to-end since the test asset calls Assert.IsTrue(value > 0) in DataDrivenTest.
| // Assertion usage (drain the static counters) | ||
| Dictionary<string, long> assertionCounts = TelemetryCollector.DrainAssertionCallCounts(); | ||
| if (assertionCounts.Count > 0) | ||
| { | ||
| metrics["mstest.assertion_usage"] = JsonSerializer.Serialize(assertionCounts, MSTestTelemetryJsonContext.Default.DictionaryStringInt64); | ||
| } |
There was a problem hiding this comment.
[AI generated] This is by design. The InternalsVisibleTo from TestFramework to MSTestAdapter.PlatformServices already exists in the codebase for other internal APIs. TelemetryCollector is deliberately internal to avoid public API surface (per repo guidelines). The friend-assembly linkage is the standard approach used throughout this repo for adapter-framework communication.
…ectory - Fix MTP_DiscoverTests_SendsTelemetryEvent regex to match across multiple lines between sessionexit event and attribute_usage - Add global.json with VSTest runner to VSTest test asset to opt out of MTP runner enforcement from root global.json - Add workingDirectory to VSTest test methods so dotnet test resolves the local global.json correctly
| private readonly ITestSourceHandler _testSourceHandler; | ||
| private readonly Func<string, IDictionary<string, object>, Task>? _telemetrySender; | ||
|
|
||
| public MSTestDiscoverer() |
There was a problem hiding this comment.
- Is the parameterless constructor no longer called? Should we remove it?
- Should we now remove the "for testing purposes" comment on the other constructor?
There was a problem hiding this comment.
[AI generated] Good questions. The parameterless constructor is still called — it's the public entry point used by the VSTest framework to instantiate the discoverer via reflection. The internal /* for testing purposes */ constructor with telemetrySender is used by MSTestBridgedTestFramework (MTP path) which passes a real telemetry sender. Both constructors are needed. The "for testing purposes" comment could be misleading now since it's also used in production (MTP path) — happy to update the comment if the team prefers.
| finally | ||
| { | ||
| new UnitTestDiscoverer(_testSourceHandler).DiscoverTests(sources, logger, discoverySink, discoveryContext); | ||
| // Use Task.Run to avoid capturing any SynchronizationContext that could cause deadlocks |
There was a problem hiding this comment.
[AI generated] Could you elaborate on what doesn't make sense? This line Task.Run(() => MSTestTelemetryDataCollector.SendTelemetryAndResetAsync(_telemetrySender)).GetAwaiter().GetResult() sends telemetry at the end of discovery in the VSTest path. The Task.Run is there to avoid capturing a SynchronizationContext that could deadlock since DiscoverTests is synchronous. Happy to discuss alternatives.
| { | ||
| new UnitTestDiscoverer(_testSourceHandler).DiscoverTests(sources, logger, discoverySink, discoveryContext); | ||
| // Use Task.Run to avoid capturing any SynchronizationContext that could cause deadlocks | ||
| Task.Run(() => MSTestTelemetryDataCollector.SendTelemetryAndResetAsync(_telemetrySender)).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
That's not a good pattern.
- Do we really want to have this as a fire-and-forget?
- Synchronously blocking a threadpool thread on an async task is bad.
There was a problem hiding this comment.
[AI generated] Fair points. The DiscoverTests method is synchronous (VSTest ITestDiscoverer interface), so we can't await directly. The Task.Run(...).GetAwaiter().GetResult() pattern avoids SynchronizationContext deadlocks.
Regarding fire-and-forget: telemetry sending is wrapped in a try/catch that swallows all exceptions (SendTelemetryAndResetAsync never throws), so the blocking call is safe. The alternative would be to make it truly fire-and-forget, but then we risk the process exiting before telemetry is flushed.
This is a known trade-off — the synchronous VSTest interface forces this pattern. The MTP path (in MSTestExecutor) handles this more cleanly since the execution path is already async. Open to suggestions if there's a better approach within the VSTest constraints.
| /// Collects and aggregates telemetry data about MSTest usage within a test session. | ||
| /// Captures settings, attribute usage, custom/inherited types, and assertion API usage. | ||
| /// </summary> | ||
| internal sealed class MSTestTelemetryDataCollector |
There was a problem hiding this comment.
Is this crossing appdomain boundaries?
There was a problem hiding this comment.
[AI generated] Good question. MSTestTelemetryDataCollector uses static state (s_current via Volatile.Read/Write) so it lives in the adapter's AppDomain. On .NET Framework with AppDomains enabled, test code runs in a separate AppDomain, but the adapter (discoverer/executor) runs in the host AppDomain. Since MSTestTelemetryDataCollector is only accessed from the adapter side (discoverer, executor, settings population), it doesn't cross AppDomain boundaries.
The TelemetryCollector in TestFramework (which tracks assertion calls) is static in the test AppDomain, and its data is drained via DrainAssertionCallCounts() which is called from the adapter's BuildMetrics(). The InternalsVisibleTo allows this cross-assembly call, but both the TestFramework and the adapter's PlatformServices load in the same AppDomain context when the adapter invokes the test methods.
- Remove unused classType parameter from TrackDiscoveredClass - Fix null! to string? with null in TrackDiscoveredClass switch - Update DrainAssertionCallCounts doc to note best-effort semantics - Add assertion_usage verification in MTP run telemetry test - Add WIN_UI guards alongside WINDOWS_UWP for telemetry code - Replace System.Text.Json with manual JSON serialization - Fix Encoding.UTF8 to System.Text.Encoding.UTF8 for netstandard2.0
- Remove misleading 'for testing purposes' comment on MSTestDiscoverer internal constructor (it's also used by MSTestBridgedTestFramework) - Remove unnecessary Task.Run wrapper around SendTelemetryAndResetAsync in MSTestDiscoverer (no SyncContext deadlock risk in VSTest/MTP hosts, and SendTelemetryAndResetAsync catches all exceptions internally)
Summary
Add infrastructure to collect aggregated telemetry about MSTest usage within test sessions. This data helps understand which APIs are heavily used or unused to guide future investment.
What's collected
Architecture
TelemetryCollectorConcurrentDictionarywith atomic swap-and-drainMSTestTelemetryDataCollectorKey design decisions
TESTINGPLATFORM_TELEMETRY_OPTOUT)DrainAssertionCallCountsusesInterlocked.Exchangefor atomic swap;Currentproperty usesVolatile.Read/WriteSendTelemetryAndResetwithTask.Runto avoidSynchronizationContextcaptureTrackAssertionCallisAggressiveInlining; hot-path string building usesstring.Concatinstead of interpolationRemaining items for discussion
InterpolatedStringHandlerassertion overloads bypass telemetry tracking (they have independent code paths viaComputeAssertion()). Tracking those would require modifying handler structs.InternalsVisibleToforMSTest.TestAdapter→Microsoft.Testing.Platformtightens coupling — worth discussing if a proper API surface would be better.Test coverage