Skip to content

Report test extension load failures to the user - #16382

Open
Jakub Jareš (nohwnd) wants to merge 2 commits into
microsoft:mainfrom
nohwnd:pilot/tell-the-user-when-a-test-extension-fails
Open

Report test extension load failures to the user#16382
Jakub Jareš (nohwnd) wants to merge 2 commits into
microsoft:mainfrom
nohwnd:pilot/tell-the-user-when-a-test-extension-fails

Conversation

@nohwnd

Copy link
Copy Markdown
Member

TestPluginDiscoverer only told the user about a failing extension file when Assembly.Load threw FileLoadException. Two other paths were silent and wrote to EqtTrace only:

  • the general Exception handler in GetTestExtensionsFromFiles, which is the one that catches the FileNotFoundException thrown when an extension or one of its dependencies cannot be found;
  • the ReflectionTypeLoadException handler in GetTestExtensionsFromAssembly, which carries on with a half-loaded type list.

So an adapter that half-loaded gave the user fewer tests than expected, or a hang, with no way to find out why short of re-running with /diag. Both now report through TestSessionMessageLogger using the existing, already localised FailedToLoadAdapaterFile message, so no .resx or .xlf change is needed.

Scanning stays best effort. Nothing throws and nothing aborts, a partially loaded assembly is still scanned for every extension type, each file is reported once per run, and the two C++ UWP adapters that are probed speculatively when no extension was found are not reported because they are absent everywhere except UWP.

With an adapter whose dependency was deleted, the user now sees:

Failed to load extensions from file 'C:\Temp\missingdep\Fabrikam.TestAdapter.dll'. Please use /diag for more information.

and the run still discovers every test.

This is the diagnostic half of #15577. The packaging cause behind the report on that issue, the missing net462 companion assemblies, was fixed separately in #15739 and shipped in 18.9.0, so this does not close #15577 on its own.

Verified: build.cmd -c Release compiles clean, Microsoft.TestPlatform.Common.UnitTests passes on net481 and net11.0 (401 tests), and a locally built vstest.console reproduces both failure modes end to end.

Related to #15577

🤖

TestPluginDiscoverer only told the user about a failing extension file when
Assembly.Load threw FileLoadException. The general Exception handler, which is
the one that catches the FileNotFoundException thrown when an extension or one
of its dependencies is missing, and the ReflectionTypeLoadException handler for
an assembly that loads but whose types do not, both wrote to EqtTrace only. A
user whose adapter half-loaded got fewer tests than expected, or a hang, and
could only find out why by re-running with /diag.

Both now report through TestSessionMessageLogger with the existing, already
localised FailedToLoadAdapaterFile message. Scanning stays best effort: nothing
throws, nothing aborts, and a partially loaded assembly is still scanned for
every extension type. Each file is reported once per run, and the two C++ UWP
adapters that are probed speculatively when no extension was found are not
reported, since they are absent everywhere except UWP.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings August 18, 2026 14:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves user-facing diagnostics when test extension (adapter/logger/etc.) discovery fails, ensuring failures that previously only surfaced in /diag logs are reported as warnings while keeping extension scanning best-effort.

Changes:

  • Report extension load failures via TestSessionMessageLogger for general load failures and ReflectionTypeLoadException partial-type-load scenarios.
  • Deduplicate user-visible warnings so the same failing extension file is not repeatedly reported across scans.
  • Add/extend unit tests to validate warning emission, deduplication, and continued scanning behavior on partial type-load failures.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginDiscoverer.cs Adds user-visible warning reporting for additional extension load failure paths and introduces per-file warning deduplication.
test/Microsoft.TestPlatform.Common.UnitTests/ExtensionFramework/TestPluginDiscovererTests.cs Adds tests that capture TestSessionMessageLogger warnings for missing/partially-loaded extensions and verifies dedupe behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginDiscoverer.cs Outdated
The set that stops a failing extension being reported once per scan was static
and never emptied, so in a design mode process that lives for hours it reported
once per process, not once per run, and only grew. Clear it from
TestPluginCache.ClearExtensions, which the runner already calls before every
discovery and run request, and compare paths ignoring case so two spellings of
one file on Windows do not warn twice.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 19, 2026 08:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@nohwnd

Copy link
Copy Markdown
Member Author

This does the same thing as #16390 and both are green and mergeable, so whichever goes in first makes the other conflict. I read both diffs and moved the one piece that only exists here over to #16390, so this one can be closed without losing anything.

Where #16390 is better:

  • It splits the assembly load from the assembly inspection in GetTestExtensionsFromFiles. On main one catch covers both, so a broken TestPluginInformation constructor looks the same as a file that could not be loaded. This PR leaves that alone.
  • It replaces FailedToLoadAdapaterFile with FailedToLoadExtensionFile, which carries the reason, so the user sees which dependency is missing instead of being pointed at /diag. The resx, all 13 xlf files and the deliberate pre-touch in TestPluginCache.SetupAssemblyResolver are updated with it.
  • It reports a ReflectionTypeLoadException only when no type loaded at all. This PR reports it even when some types loaded, and that would warn on ordinary green runs. Vstest.console logging LoaderExceptions in success scenario as well #290 is exactly that: the MSTest adapter throws ReflectionTypeLoadException during a completely successful run because it references an older ObjectModel.

Where this PR was right and #16390 was not:

The warning in #16390 was deduplicated on UnloadableFiles, which is static and never cleared. ClearExtensions() clears the extension paths and invalidates the cache but does not touch it, so TryAdd returned true once per process. In design mode, where an editor keeps the runner alive across many discovery and run requests, the user heard about a broken extension on the first request and then never again. This PR solved that on purpose with a separate ReportedFiles that ClearExtensions() resets.

I settled that with a test rather than by reading the code. Same test source, run on both branches, Release, net11.0 and net481: scan a file that cannot be loaded, assert the warning, call TestPluginCache.Instance.ClearExtensions(), scan again, count the warnings.

  • On nohwnd-warn-when-test-extension-fails-to-load: 1 warning after the first scan, still 1 after the clear. Once per process.
  • On pilot/tell-the-user-when-a-test-extension-fails: 2 warnings. Once per run.

So the reset here was fixing something real, and it is now in #16390. It keeps that PR's structure and separates the two concerns instead of coupling them: UnloadableFiles answers whether loading the file is worth another try, ReportedFiles answers whether the user has already seen the warning, and ClearExtensions() empties both. Clearing the first one fixes a second thing along the way, an extension whose missing dependency has since appeared is tried again on the next request instead of staying skipped until the process exits.

Tests carried over from here into #16390:

  • Warn again after the extension cache is cleared. It fails without the reset, 1 warning instead of 2.
  • Warn only once when the casing of the path differs.
  • The PartiallyLoadedAssembly fake, now used for two tests: no warning when only some types fail to load, which is the Vstest.console logging LoaderExceptions in success scenario as well #290 guard, and a warning naming the missing dependency when no type loads at all.

Dropped: the two tests here that assert a warning on a partial type load. Under #16390 that case is deliberately quiet, so the assertion is inverted rather than ported.

Microsoft.TestPlatform.Common.UnitTests passes on both target frameworks with all of that in place. Nothing else in this PR is missing from #16390, so I think this one can be closed.

🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

STJ migration: DesignMode/wrapper communication hangs during test execution

2 participants