Add dynamically resolved MTP extensions via JSON manifests - #10406
Conversation
Captures the design space for the recurring "load MTP extensions at run time without touching the test project's build" request: classifies the asks into observe/shape/environment/intercept buckets, documents the existing org-wide static injection and out-of-process answers, records why the naive plugin form conflicts with MTP's IVT-coupled contract, NativeAOT/single-file support and netstandard2.0 target, and lists the non-negotiable constraints and prerequisites if we build a narrow form. Marked "Under discussion" - no decision is proposed yet. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
Central infrastructure teams need to change how tests run without editing every test project's build. This adds a late-bound registration path that keeps exactly one extension-registration concept: the static AddExtensions(ITestApplicationBuilder, string[]) hook the MSBuild-generated SelfRegisteredExtensions already calls. Only how the hook is reached differs. A manifest named *.testingplatformextensions.json next to the test application declares extension assemblies plus hook type names. At the end of TestApplication.CreateBuilderAsync the platform discovers the manifests, loads each assembly, and invokes its hook. Design decisions, in docs/RFCs/023-Dynamic-Extension-Loading.md: - Isolation by default. Each extension assembly gets its own AssemblyLoadContext resolving dependencies from its own .deps.json, so an extension cannot conflict with the application's dependency graph. Only the platform contract assemblies are shared by name, which is what keeps ITestApplicationBuilder a single type across the boundary; when the host does not carry a shared abstractions assembly the extension falls back to its own copy rather than failing to load. netstandard2.0 has no ALC, so it uses Assembly.LoadFrom and reports that isolation is unavailable. - Fail loudly. A broken manifest, missing assembly, missing type, wrong hook signature or throwing hook fails the run; a manifest exists because someone decided every run must be affected by it. The diagnostic log is flushed before the failure escapes. - Hooks must return void. The hook is invoked synchronously, so an async Task hook would never be awaited and its failures would be swallowed. - Hooks receive the real builder, not a wrapper, because shipped helpers such as AddOpenTelemetryProvider and AddRunSettingsService downcast ITestApplicationBuilder. The one restriction - dynamic extensions may not register a test framework, which would silently change which tests run - is enforced inside the builder for the duration of the call. - TESTINGPLATFORM_NODYNAMICEXTENSIONS is the kill switch and first triage step. Covered by unit tests for manifest parsing, discovery, de-duplication and hook invocation, plus acceptance tests that exercise the real load contexts, including one where the application and the extension depend on conflicting builds of the same assembly and each must keep its own. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
The test asserted the error message contains "1" to prove it names the second entry, but the manifest path was built from Path.GetTempPath(), which almost always contains a digit (for example the 8.3 short name AMAURY~1, or an agent's ...\Temp\1\). The assertion therefore passed regardless of the index the parser reported. Use a deterministic digit-free manifest path so the only "1" that can appear is the index, and also assert the message does not name entry 0. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
There was a problem hiding this comment.
Pull request overview
Adds manifest-driven, isolated MTP extension loading for centrally managed test infrastructure.
Changes:
- Discovers, validates, de-duplicates, and invokes extension hooks from JSON manifests.
- Adds isolated assembly loading, diagnostics, safeguards, schema, and localization resources.
- Adds unit and acceptance coverage across supported runtimes.
Show a summary per file
| File | Description |
|---|---|
docs/RFCs/023-Dynamic-Extension-Loading.md |
Documents the proposed contract and rationale. |
docs/testingplatformextensions.schema.json |
Defines the manifest schema. |
src/Platform/Microsoft.Testing.Platform/Builder/TestApplication.cs |
Runs dynamic registration during builder creation. |
src/Platform/Microsoft.Testing.Platform/Builder/TestApplicationBuilder.cs |
Guards test-framework registration during hooks. |
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionAssemblyLoader.cs |
Implements isolated assembly loading. |
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionConstants.cs |
Defines manifest and loading constants. |
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionEntry.cs |
Models validated extension entries. |
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionJsonReader.cs |
Parses manifests on .NET. |
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionJsonReader.netstandard.cs |
Parses manifests on .NET Standard. |
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionLoader.cs |
Handles discovery, filtering, and invocation. |
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionManifest.cs |
Models parsed manifests. |
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionManifestParser.cs |
Validates and resolves manifest entries. |
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/IDynamicExtensionAssemblyLoader.cs |
Abstracts assembly loading. |
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/IDynamicExtensionRegistrationGuard.cs |
Defines registration guard scope. |
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/RawExtensionManifest.cs |
Defines intermediate JSON models. |
src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs |
Adds the dynamic-extension kill switch. |
src/Platform/Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt |
Tracks new internal APIs. |
src/Platform/Microsoft.Testing.Platform/Resources/PlatformResources.resx |
Adds dynamic-loading diagnostics. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.cs.xlf |
Updates Czech localization inputs. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.de.xlf |
Updates German localization inputs. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.es.xlf |
Updates Spanish localization inputs. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.fr.xlf |
Updates French localization inputs. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.it.xlf |
Updates Italian localization inputs. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ja.xlf |
Updates Japanese localization inputs. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ko.xlf |
Updates Korean localization inputs. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pl.xlf |
Updates Polish localization inputs. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pt-BR.xlf |
Updates Portuguese localization inputs. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ru.xlf |
Updates Russian localization inputs. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.tr.xlf |
Updates Turkish localization inputs. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hans.xlf |
Updates Simplified Chinese localization inputs. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hant.xlf |
Updates Traditional Chinese localization inputs. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/DynamicExtensionIsolationTests.cs |
Verifies dependency and contract isolation. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/DynamicExtensionTests.cs |
Verifies end-to-end manifest behavior. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/DynamicExtensions/DynamicExtensionAssemblyLoaderTests.cs |
Tests real assembly-loading behavior. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/DynamicExtensions/DynamicExtensionLoaderTests.cs |
Tests discovery and hook invocation. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/DynamicExtensions/DynamicExtensionManifestParserTests.cs |
Tests manifest parsing and validation. |
Review details
- Files reviewed: 36/36 changed files
- Comments generated: 3
- Review effort level: Balanced
…erage Review feedback surfaced three doc/behaviour ambiguities: - RFC section 4 described the ordering as "at the end of CreateBuilderAsync, therefore before the static extensions", which reads as a contradiction: "end" meant the end of that method, not the end of the sequence. State the order outright and show the three-call entry point so it is unambiguous. - The DynamicExtensionTests remark claimed the asset is the strongest check of the isolation contract, but the class also runs net462, where the loader uses Assembly.LoadFrom and reports IsIsolated == false. Split the claim per target framework and point at DynamicExtensionIsolationTests for the real cross-context coverage. - RFC section 1 stated the duplicate-id conflict rule without saying it only applies to declarations that actually load. Disabled entries are skipped before their id is considered, so a switched-off declaration can neither collide with nor block an enabled one. Document that and pin it with a test: nothing is silently dropped when the author explicitly opted out, and the alternative would let enabled:false hard-fail a run, defeating its role as the per-extension escape hatch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs:48
- This shared source file is also compiled into HotReload, Retry, TrxReport, HangDump, and MSBuild, and those projects track its internal constants (for example
Microsoft.Testing.Extensions.HotReload/InternalAPI/InternalAPI.Shipped.txt:20). Add this constant to each correspondingInternalAPI.Unshipped.txt; otherwise their API analyzer builds will report the newly declared internal API.
public const string TESTINGPLATFORM_NODYNAMICEXTENSIONS = nameof(TESTINGPLATFORM_NODYNAMICEXTENSIONS);
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionLoader.cs:76
IsDynamicCodeSupportedis not an assembly-loading capability.test/IntegrationTests/MSTest.Acceptance.IntegrationTests/PublishAotNonNativeTests.cs:9-27documents that<PublishAot>truesets this switch to false even when the managed output still runs, so such test applications will now fail before a load is attempted. Gate on actual assembly-loading support, or attempt the load and translatePlatformNotSupportedException, instead.
if (!_runtimeFeature.IsDynamicCodeSupported)
{
// Silently skipping here would mean the policy the manifest encodes did not apply and nobody
// noticed, which is the failure mode this feature exists to avoid.
throw new InvalidOperationException(string.Format(
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/DynamicExtensionIsolationTests.cs:12
- Correct the grammatical error: “the application's exposes” should be “the application exposes.”
- Files reviewed: 36/36 changed files
- Comments generated: 1
- Review effort level: Balanced
The return-type check rejected a Task-returning hook because the platform
invokes hooks synchronously, so the task would never be awaited. An
'async void' hook slips through that check -- its ReturnType is void -- while
behaving identically: Invoke returns at the first await, the registration
guard scope is disposed, whatever the hook does afterwards races the test
application's own setup, and an exception past that point never reaches the
surrounding try/catch.
Reject it by looking for the compiler-emitted AsyncStateMachineAttribute,
which is the only reliable way to tell an async method from a synchronous one
through reflection, and give it its own message since the fix ("make the hook
synchronous") differs from the return-type one ("change the return type").
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionLoader.cs:110
Directory.Existsreturnsfalseboth when a directory is absent and when access/I/O errors prevent probing it. The early return therefore treats an unreadable application directory as “no manifests,” contradicting the fail-loud policy and potentially skipping centrally deployed extensions. Enumerate whenever the directory path is known and wrap all access failures (includingSecurityExceptionon .NET Framework).
if (RoslynString.IsNullOrEmpty(directory) || !_fileSystem.ExistDirectory(directory))
{
return [];
}
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionJsonReader.netstandard.cs:29
- The Jsonite reader accepts trailing non-JSON content because
Json.Deserializereturns after parsing the first root value without requiring EOF (Json.cs:83-84,JsonReader.cs:206-210). Thus{"extensions":[]} garbageis accepted on net462 but rejected byJsonDocument.Parseon .NET, violating the cross-target validation guarantee and allowing a malformed manifest to run. Require full input consumption and add a parity test for trailing content.
document = Json.Deserialize(content, Settings);
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionManifestParser.cs:106
- An explicitly empty or whitespace
idis silently treated as omitted, although the schema requiresminLength: 1. If a deployment substitution emits a blank ID, manifests pointing to different paths will no longer de-duplicate and the extension may run twice. Distinguish an omitted value (null) from a present blank value and reject the latter.
string id = RoslynString.IsNullOrWhiteSpace(raw.Id)
? $"{resolvedAssemblyPath}|{typeFullName}"
: raw.Id!;
- Files reviewed: 36/36 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Three issues raised in review, all in code added by this PR:
- The RuntimeFeature.IsDynamicCodeSupported gate was the wrong signal.
<PublishAot>true</PublishAot> turns that switch off even for builds whose
managed output still runs normally on CoreCLR -- the repo's own
PublishAotNonNativeTests documents exactly that -- so such applications
would have been refused an extension they were perfectly able to load.
Detect the real constraint instead by attempting the load and translating
PlatformNotSupportedException, which still yields the AOT-specific message
naming the manifest, the enabled property and the kill switch. IRuntimeFeature
is no longer needed by the loader.
- Discovery pre-checked Directory.Exists, which returns false both for a
missing directory and for one that cannot be read. An unreadable application
directory was therefore silently treated as "no manifests", contradicting the
fail-loud policy. Enumerate directly instead and distinguish the two:
DirectoryNotFoundException means nothing was declared, while IO, access and
security failures throw. SecurityException is now caught too, for .NET
Framework.
- The Jsonite-based netstandard2.0 reader returns as soon as it has parsed the
root value and never checks for end of input, so '{...} garbage' was accepted
on .NET Framework while System.Text.Json rejected it on .NET. That broke the
cross-target validation parity the two readers are supposed to share. Feed
Jsonite through a counting reader so the trailing content can be detected,
and assert the parity from a test that runs on both.
Also fixes an awkward possessive in the isolation test's remarks.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/Platform/Microsoft.Testing.Platform/Resources/PlatformResources.resx:1047
- This message is also used for non-Task return values (the new
NonVoidHooktest returnsint), so “a returned task would never be awaited” becomes an incorrect diagnostic such as forSystem.Int32. Use generic wording for all non-void returns or separate Task-like returns from other values, then regenerate the XLF files.
<value>The '{0}.{1}' extension hook from the assembly '{2}' declared in the extension manifest '{3}' returns '{4}' but must return void. The platform invokes the hook synchronously, so a returned task would never be awaited and its registrations and failures would be lost.</value>
docs/testingplatformextensions.schema.json:40
- The schema accepts whitespace-only required paths and type names because
minLength: 1counts spaces, whileDynamicExtensionManifestParser.RequireNonEmptyrejects them withIsNullOrWhiteSpace. Add a non-whitespace pattern to both required string definitions so editor validation matches runtime validation.
"minLength": 1,
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionAssemblyLoader.cs:128
- Contract sharing is not actually guaranteed by simple name here.
Default.LoadFromAssemblyName(assemblyName)applies version binding; if an injected extension references a newer Microsoft.Testing.Platform than the host, this can reject the already-loaded host assembly and the caller then loads the extension's private copy. The hook'sITestApplicationBuilderconsequently has a different identity, so a valid hook is reported as missing. Prefer an already-loaded default-context assembly with the matching simple name before attempting versioned resolution (or fail with an explicit compatibility error), and cover version skew in the loader tests.
return Default.LoadFromAssemblyName(assemblyName);
- Files reviewed: 36/36 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Default.LoadFromAssemblyName applies version binding, so an extension compiled against a newer Microsoft.Testing.Platform than the host would fail to bind. The failure was swallowed and the loader fell through to the extension's own private copy of the platform, giving the hook a different ITestApplicationBuilder identity -- which then surfaced as "the type does not expose a public static void AddExtensions(...)", a baffling diagnostic for what is really version skew. Sharing a contract means sharing it by name, so look the assembly up among the default context's already-loaded assemblies instead, ignoring the requested version. If the versions really are incompatible the resulting MissingMethodException is the honest failure, and the same one a statically referenced extension would give. Assemblies the host has not loaded still fall through to the extension's own copy, so an extension depending on an abstractions package the test application never referenced keeps working. Also from the same review pass: - The non-void return message claimed "a returned task would never be awaited", which reads as nonsense for a hook returning int. Reworded to cover any return value while still calling out the task case. - The JSON schema allowed whitespace-only assemblyPath and typeFullName because minLength counts spaces, while the parser rejects them via IsNullOrWhiteSpace. Added a non-whitespace pattern so editor validation matches runtime. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionAssemblyLoader.cs:148
- This only shares contract assemblies that have already been loaded in the default context. Dynamic hooks run before static hooks, so
Microsoft.Testing.Extensions.TrxReport.Abstractionscan be present in the application's dependency graph but not loaded yet; a dynamically loaded Trx extension then binds to its private copy, while MSTest later binds to the default copy, makingITrxReportCapabilityinvisible across the boundary. After checkingDefault.Assemblies, try loading the listed contract by simple name from the default context and fall back to the extension copy only when it is genuinely absent.
private static Assembly? FindLoadedContractAssembly(string? simpleName)
{
foreach (Assembly loaded in Default.Assemblies)
- Files reviewed: 36/36 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The previous change resolved shared contract assemblies by scanning the default context's already-loaded assemblies, which fixed version-skew binding but introduced a timing hole: being loaded is not the same as being available. Dynamic hooks run before the statically registered extensions, so a contract such as Microsoft.Testing.Extensions.TrxReport.Abstractions is frequently in the application's dependency graph but untouched at the moment an extension resolves it. Scanning only loaded assemblies handed the extension a private copy, and when the static extension later loaded the real one the contract was split -- an ITrxReportCapability implemented by the dynamic extension would have been invisible to TrxReport, which is precisely the failure the shared-contract list exists to prevent. Prefer an already-loaded copy first (still ignoring version, so skew cannot break binding), then ask the default context to load the contract by simple name only, and fall back to the extension's own copy just when it is genuinely absent. All three paths are covered by tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionLoader.cs:150
- On .NET Framework, reading a protected manifest can throw
SecurityException. The directory scan handles that exception, but this filter does not, so it escapes without the actionable manifest-specific error promised for unreadable manifests. IncludeSecurityExceptionin this filter.
catch (Exception ex) when (ex is IOException or UnauthorizedAccessException)
test/UnitTests/Microsoft.Testing.Platform.UnitTests/DynamicExtensions/DynamicExtensionAssemblyLoaderTests.cs:96
- Evaluating
typeof(Moq.Mock).Assemblynecessarily loads Moq beforeResolveSharedContractAssemblyis invoked. The resolver therefore finds it inDefault.Assembliesand this test never exercises theDefault.LoadFromAssemblyNamebranch its name and comments claim to cover. Use a dependency confirmed absent fromDefault.Assembliesbefore invocation so the not-yet-loaded contract path is actually tested.
- Files reviewed: 36/36 changed files
- Comments generated: 2
- Review effort level: Balanced
Security review asked for three changes to the posture of this feature. Off by default, enabled per run. Nothing is discovered, parsed or loaded without --enable-dynamic-extensions. A dynamically loaded extension runs with full trust in the test process, so write access to a directory must not by itself be enough to get code executed. A command line option rather than an environment variable is deliberate: the switch belongs with the invocation that accepts the risk, visible in the CI definition and the process command line, not in ambient machine state that is set once and forgotten. TESTINGPLATFORM_NODYNAMICEXTENSIONS is kept as a second line of defence and now wins over the option, so a machine can be locked down regardless of what a pipeline asks for. Never silent. When anything is loaded, the platform writes to standard output how many extensions were loaded and, for each, its display name, resolved assembly path, hook type and declaring manifest, followed by the trust warning. This is not gated on --diagnostic: whoever reads the log should see that foreign code ran in the test process without having opted into extra logging. Server mode is the one exception, since stdout is a protocol channel there; the diagnostic log still records everything. An explicit trust warning now appears in the RFC, the JSON schema description, the --help text and the run output: dynamically loaded extensions run with full trust inside the test process, do not use this feature with code or directories you do not trust. Version compatibility is explicitly the user's responsibility and is recorded as settled in the RFC rather than left open. Help and info expectations updated in all four acceptance files as required. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionLoader.cs:60
- The PR description promises
TESTINGPLATFORM_NODYNAMICEXTENSIONS=1as the emergency kill switch and says it has acceptance coverage, but this is the only feature gate and the environment variable does not exist anywhere in the implementation or tests. Either implement the documented override or update the PR description/testing claims so operators are not told to rely on a nonexistent escape hatch.
if (!_commandLineParseResult.IsOptionSet(PlatformCommandLineProvider.EnableDynamicExtensionsOptionKey))
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionLoader.cs:99
ReportLoadedExtensionswrites throughSystemConsole, whoseStreamWriter.WriteLinecan throw (for example on a closed stdout pipe). Because this call runs infinally, such an output failure replaces an active assembly/hook exception, losing the manifest-aware error this failure policy promises. Preserve the original exception when reporting also fails (while still allowing reporting failures to surface on the success path).
finally
{
ReportLoadedExtensions(loaded);
}
src/Platform/Microsoft.Testing.Platform/Builder/TestApplication.cs:145
- If flushing or disposing the file logger throws, this
catchexits with that cleanup exception instead of rethrowing the original manifest/assembly/hook failure.FileLogger.DisposeAsyncperforms fallible stream flush/disposal, so the diagnostic-preservation path can erase the actionable error it is meant to preserve. Catch cleanup failures without replacing the original exception (or aggregate them while keeping the original primary).
if (loggingState.FileLoggerProvider is { } fileLoggerProvider)
{
await DisposeHelper.DisposeAsync(fileLoggerProvider).ConfigureAwait(false);
}
- Files reviewed: 41/41 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Two paths could discard an actionable exception in favour of an incidental one: * ReportLoadedExtensions ran in a finally, so if a hook threw and stdout was also broken (a closed pipe), the console IOException replaced the InvalidOperationException naming the manifest and the extension. * The diagnostic-log flush in CreateBuilderAsync could throw from DisposeAsync and replace the very failure it exists to preserve. Both are now best-effort while unwinding. On the success path a reporting failure still surfaces, since nothing else is wrong and swallowing it would leave extensions loaded with no notice at all. Covered by two tests. The masking one needs a partial load to be meaningful -- reporting writes nothing when the first extension fails -- and mutation testing against the old finally confirms it catches the regression. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
30f7586 to
fc8f21c
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionJsonReader.netstandard.cs:22
- The two target-specific readers currently enforce different nesting limits.
JsonDocument.Parseuses a maximum depth of 64 by default, while Jsonite'sMaxDepthdefaults to0(unlimited), so a deeply nested manifest is rejected on .NET but accepted on net462 and can recurse without a bound there. Set the Jsonite limit to 64 as well so validation remains target-framework-neutral.
private static readonly JsonSettings Settings = new()
{
AllowComments = true,
AllowTrailingCommas = true,
};
src/Platform/Microsoft.Testing.Platform/CommandLine/PlatformCommandLineProvider.cs:40
- The PR description still promises
TESTINGPLATFORM_NODYNAMICEXTENSIONS=1as the global triage kill switch and says acceptance tests cover it, but this implementation only introduces the opt-in command-line option and there is no environment-variable check anywhere in the repository. Either implement the advertised override and coverage, or update the PR description to state that removing--enable-dynamic-extensionsis the escape hatch.
public const string EnableDynamicExtensionsOptionKey = "enable-dynamic-extensions";
- Files reviewed: 41/41 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Code quality review flagged the two empty catch blocks added by the previous commit. The concern is fair: swallowing without a trace leaves nothing for the next person debugging it. In the loader, the console is the thing that failed, so the diagnostic log is a sink still worth trying; the attempt is itself guarded so nothing can displace the load failure being rethrown. In CreateBuilderAsync there is genuinely nowhere to report to -- the file logger is what just failed, and standard output is a protocol channel under --server and a single JSON document under --list-tests json, so writing there would corrupt a machine-readable stream. Both suggested remedies were unusable for that reason: IConsole exposes no stderr, and System.Diagnostics.Trace is not wired up anywhere in this platform. The block stays empty and now says why. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionJsonReader.cs:63
- Duplicate
extensionsproperties are accepted with last-one-wins semantics, so{ "extensions": [policyA], "extensions": [policyB] }silently dropspolicyA. That conflicts with this feature's fail-loudly policy: a malformed manifest can omit a deliberately deployed extension without any error or diagnostic. Please reject duplicate recognized properties (both the rootextensionskey and recognized entry keys) consistently in both JSON readers and add parity tests.
// JsonDocument surfaces every occurrence of a duplicated key, whereas the Jsonite-based reader
// used on netstandard2.0 keeps only the last. Reset here so both readers are last-wins and a
// pathological manifest cannot behave differently per target framework.
manifest.HasExtensionsProperty = true;
manifest.IsExtensionsPropertyAnArray = false;
manifest.Entries.Clear();
- Files reviewed: 41/41 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
A manifest could declare 'extensions' (or a recognized entry property) twice and the reader kept one, discarding the other with no error. That directly contradicts the failure policy: an infra team's deliberately deployed block would vanish from a run that otherwise looked fine. The previous code went out of its way to make both readers last-wins, for cross-target parity. That optimized for the wrong thing -- parity of a bad behavior. System.Text.Json exposes every occurrence of a repeated key, so the .NET reader now records it and the parser rejects the manifest. The netstandard2.0 reader cannot follow: Jsonite fills a dictionary by indexer assignment, so the earlier value is gone before the reader runs, and detecting it would mean forking a vendored parser the server-mode JSON-RPC stack also uses. Duplicate keys are therefore rejected on .NET and last-wins on .NET Framework, which is documented on both readers and in the RFC. Staying quiet everywhere for the sake of matching would have been the worse trade. Unknown properties stay exempt: repeating one cannot change what loads, so failing on it would only punish forward-compatible manifests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionJsonReader.cs:1
- This new C# file is missing the UTF-8 BOM required by
.editorconfig:66-67. Resave it as UTF-8 with BOM so it follows the repository's mandatory encoding convention.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionLoader.cs:1
- This new C# file is missing the UTF-8 BOM required by
.editorconfig:66-67. Resave it as UTF-8 with BOM so it follows the repository's mandatory encoding convention.
// Copyright (c) Microsoft Corporation. All rights reserved.
test/UnitTests/Microsoft.Testing.Platform.UnitTests/DynamicExtensions/DynamicExtensionLoaderTests.cs:1
- This new C# file is missing the UTF-8 BOM required by
.editorconfig:66-67. Resave it as UTF-8 with BOM so it follows the repository's mandatory encoding convention.
- Files reviewed: 41/41 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This comment has been minimized.
This comment has been minimized.
.editorconfig requires charset = utf-8-bom for *.cs. Three files created by this PR lost their BOM because I rewrote them with Set-Content / WriteAllText during earlier fixes, both of which default to BOM-less UTF-8. Encoding-only change: one line differs per file, CRLF line endings and content are untouched, and both unit test suites still pass. Not touching JsonCommandLineOptionsTests.cs, which is also BOM-less but was already so before this branch, so fixing it here would be an unrelated change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 229e9cb6-3e63-4144-9ea4-babc7243be32
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection results could not be parsed. Review the workflow run logs for details. 🧪 Test quality grade — PR #10406No new or modified test methods were identified in the changed regions of this PR (the pre-step's changed-test-file list was empty or unavailable). Nothing to grade. Re-run with
|
There was a problem hiding this comment.
Review details
Suppressed comments (3)
docs/RFCs/023-Dynamic-Extension-Loading.md:406
- The claim that every error names the opt-in flag is inaccurate: most manifest, assembly, type, and hook error resources do not include
--enable-dynamic-extensions. Keep the escape-hatch guidance without promising it appears in every message.
The counterweight to a strict policy is that turning the feature off is always one flag away: every
error names `--enable-dynamic-extensions` as the way to skip discovery entirely.
src/Platform/Microsoft.Testing.Platform/DynamicExtensions/DynamicExtensionLoader.cs:407
GetMethodcan throwAmbiguousMatchExceptionwhen the hook type contains both the required non-generic hook and a generic overload with the same parameter types. Static registration compiles and selects the non-generic hook, but dynamic registration reports that no hook exists, violating the shared hook contract. Enumerate the public static candidates and select the non-generic method with the exact two parameter types, then apply the existing return/async checks; add a generic-overload regression test.
hook = hookType.GetMethod(
DynamicExtensionConstants.HookMethodName,
BindingFlags.Public | BindingFlags.Static | BindingFlags.FlattenHierarchy,
binder: null,
types: [typeof(ITestApplicationBuilder), typeof(string[])],
docs/RFCs/023-Dynamic-Extension-Loading.md:14
- This summary states that every declared assembly is loaded into an isolated context, but the supported .NET Framework path uses non-isolated
Assembly.LoadFrom. Qualify the summary here so readers do not infer an isolation guarantee that the implementation explicitly cannot provide on net462.
This issue also appears on line 405 of the same file.
assemblies; at start-up the platform discovers those manifests, loads each declared assembly into an
isolated load context, and invokes a static hook whose signature is **identical to the existing
MSBuild `TestingPlatformBuilderHook`**:
- Files reviewed: 41/41 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. 🧵 Parallel-safety audit — PR #10406Parallelization — assemblies touched by this PR:
All three assemblies opt in via a pre-existing Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 0. Reviewed every changed test file and its HEAD-side line ranges:
No changed Nothing to flag for parallel-safety. Advisory only — heuristic, non-blocking. Re-run with
|
What this is
Adds an opt-in way to load Microsoft.Testing.Platform extensions declared by JSON manifests sitting next to the test application, so a central infrastructure team can change how tests run without editing the build of every test project.
The design deliberately keeps one extension-registration concept. A manifest names an assembly and a type; that type exposes the same static
AddExtensions(ITestApplicationBuilder, string[])hook the MSBuildTestingPlatformBuilderHookpath already uses. Only how the hook is reached differs: the compiler (static registration) or a manifest (dynamic registration).Why
Internal infrastructure teams own the CI pipeline, not the hundreds of test projects that flow through it. They want to attach a reporter, a diagnostic collector or a policy hook to runs they do not own the build for. The existing answer, publishing an internal package and injecting one
PackageReferencefrom a centralDirectory.Build.targets, already covers a large share of these requests and remains the recommendation. Dynamic registration exists for teams who cannot influence the build graph at all.We resisted this for a long time. VSTest had a comparable mechanism and it caused years of pain: extension dependencies collided with the test app's own, extension-point contracts were unversioned, and failures surfaced as unexplained test-host crashes. The RFC's Motivation section lists each of those failure modes and how this design addresses it.
Design in brief
*.testingplatformextensions.jsonin the test application's own directory, non-recursiveassemblyPath,typeFullNameid(de-duplication),enabled,displayNamepublic static void AddExtensions(ITestApplicationBuilder, string[]), identical to the MSBuild hook--enable-dynamic-extensionsAssemblyLoadContextper extension assembly on .NET;Assembly.LoadFrom(no isolation, reported) on netstandard2.0assemblyPathandtypeFullNameare required. The three optional properties each earn their place:idde-duplicates the same extension declared by two manifests (without it, a data consumer double-reports),enabledsupports "ship the manifest everywhere, turn it on for some" and doubles as the per-extension escape hatch, anddisplayNamegives the extension a handle that is not a fully-qualified type name in the loaded-extensions report, the diagnostic log, and assembly-level errors.How it answers the historical failure modes
AssemblyLoadContext, resolving dependencies from its own.deps.json. Only an explicit list of contract assemblies is shared with the host.ITestApplicationBuilderis the same type on both sides.*.dll.--enable-dynamic-extensionsfrom the invocation disables the whole mechanism, andenabled: falsedisables one extension without deleting its manifest.docs/RFCs/023-Dynamic-Extension-Loading.mdhas the full rationale, the alternatives that were rejected (directory probing, a separateIDynamicExtensioninterface, an out-of-process extension host), and the open questions.docs/testingplatformextensions.schema.jsongives editors completion for the manifest.Security posture
Reviewed with the .NET security team against the baseline security assumptions.
The only security-relevant property is where manifests are read from. They are read from the directory containing the test application executable, which is a fully trusted application folder whose contents can already influence execution flow, and never from the current working directory, which the baseline explicitly excludes. That rule is pinned by a unit test.
Everything else follows from the baseline rather than from anything this design does. Extensions run with the full privileges of the test process, exactly as a statically referenced extension or any consumed NuGet package does; in-process composition is not a security boundary and .NET offers no intra-process sandbox. Isolation is a compatibility mechanism, not containment. The opt-in is a predictability decision, not a security control: an actor who can write to the application directory is already fully trusted, so gating on a flag does not restrict them. The RFC says all of this explicitly so that nobody, now or later, mistakes these behaviours for enforcement.
Testing
--infovisibility and the opt-in gate.DynamicExtensionIsolationTestsis the important one: the application and the extension depend on two conflicting builds of the sameContoso.Sharedassembly, and each must observe its own. Without isolation one would silently win. The extension simultaneously registers a realICommandLineOptionsProviderthat the host consumes, which only works because the platform assembly identity is shared, even though the extension folder carries its own copy of it.Notes for reviewers
RegisterTestFrameworkguard lives insideTestApplicationBuilderrather than in a wrapper builder. Handing hooks a wrapper would break shipped helpers:AddOpenTelemetryProviderhard-casts toTestApplicationBuilder, andAddRunSettingsService/AddMSTestuseischecks that would silently no-op.PlatformNotSupportedException, deliberately not by pre-checkingRuntimeFeature.IsDynamicCodeSupported—<PublishAot>true</PublishAot>turns that switch off even for builds whose managed output runs fine and can load extensions.PublishTrimmedwith dynamic extensions. The trimmer removes anything the application does not reference, BCL included, so an extension can load and then fail at run time. This is documented as a limitation rather than worked around, since the extension-facing surface is unbounded.