Tell the user when a test extension fails to load - #16390
Open
Jakub Jareš (nohwnd) wants to merge 3 commits into
Open
Tell the user when a test extension fails to load#16390Jakub Jareš (nohwnd) wants to merge 3 commits into
Jakub Jareš (nohwnd) wants to merge 3 commits into
Conversation
Only FileLoadException reached the user. Every other reason a file could not be loaded - BadImageFormatException, a missing dependency, a wrong target framework - went to the diag log only, so the extension was silently missing and the tests it provides silently did not run. Now any failure to load an extension file is a warning, and it carries the reason instead of only pointing at /diag. An assembly where not a single type can be loaded is reported the same way; when some types do load we stay quiet, because the extension usually still works. The warning is emitted once per file, not once per extension type we scan for, and the C++ UWP probe files stay quiet because they are not expected to be there. 🤖
Contributor
There was a problem hiding this comment.
Pull request overview
Improves VSTest’s extension discovery so that any failure to load an extension assembly (not just FileLoadException) becomes a user-visible warning (including the failure reason), preventing “green runs” where tests are silently skipped due to missing/broken extensions.
Changes:
- Emit a warning (once per extension file) when an extension assembly cannot be loaded, including the failure reason; keep the “known probe” assemblies quiet.
- Separate “assembly load” failures (warn user) from “inspection” failures (stay in diag log) and handle the “no types could be loaded” case as a warning.
- Add unit tests covering warning emission, “warn once”, and the known-probe quiet behavior; update localized resources.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.TestPlatform.Common.UnitTests/ExtensionFramework/TestPluginDiscovererTests.cs | Adds coverage for warning emission, deduping per file, and known-probe quiet behavior. |
| src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginDiscoverer.cs | Implements warning-on-load-failure behavior and once-per-file suppression, plus known-probe quiet mode. |
| src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginCache.cs | Updates resource preloading to the new localized string key. |
| src/Microsoft.TestPlatform.Common/Resources/Resources.resx | Replaces the old warning string with a new one that includes a reason placeholder. |
| src/Microsoft.TestPlatform.Common/Resources/Resources.Designer.cs | Regenerates resource accessor for the renamed string key. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.cs.xlf | Updates localized resource entry for the new warning string. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.de.xlf | Updates localized resource entry for the new warning string. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.es.xlf | Updates localized resource entry for the new warning string. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.fr.xlf | Updates localized resource entry for the new warning string. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.it.xlf | Updates localized resource entry for the new warning string. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.ja.xlf | Updates localized resource entry for the new warning string. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.ko.xlf | Updates localized resource entry for the new warning string. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.pl.xlf | Updates localized resource entry for the new warning string. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.pt-BR.xlf | Updates localized resource entry for the new warning string. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.ru.xlf | Updates localized resource entry for the new warning string. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.tr.xlf | Updates localized resource entry for the new warning string. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.zh-Hans.xlf | Updates localized resource entry for the new warning string. |
| src/Microsoft.TestPlatform.Common/Resources/xlf/Resources.zh-Hant.xlf | Updates localized resource entry for the new warning string. |
Files not reviewed (1)
- src/Microsoft.TestPlatform.Common/Resources/Resources.Designer.cs: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69459b25-03f1-41a7-979d-9d401036d5a2
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- src/Microsoft.TestPlatform.Common/Resources/Resources.Designer.cs: Generated file
Suppressed comments (2)
src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginDiscoverer.cs:32
UnloadableFilesis used to ensure the warning is emitted only once per extension file, but the dictionary uses the default (case-sensitive) string comparer. On Windows, the same path can appear with different casing (and elsewhere in this repo extension paths are deduped withStringComparer.OrdinalIgnoreCase), which would lead to duplicate loads/warnings for what is effectively the same file.
/// <summary>
/// Files that we already failed to load. The same file is scanned once per extension type we look for
/// (test adapters, loggers, data collectors, settings providers, ...), so this both avoids repeating a load
/// that is known to fail, and makes sure the user is told about the failure only once.
/// </summary>
private static readonly ConcurrentDictionary<string, object?> UnloadableFiles = new();
test/Microsoft.TestPlatform.Common.UnitTests/ExtensionFramework/TestPluginDiscovererTests.cs:148
- The new warning test only asserts that a warning exists and mentions the extension path, but it doesn’t verify that the reason is included (the primary behavior change of this PR). Adding an assertion that the message starts with the localized resource prefix and has additional content helps prevent regressions without hardcoding an exception message.
var message = messages.Single();
Assert.AreEqual(TestMessageLevel.Warning, message.Level);
Assert.Contains(extension, message.Message);
}
The warning was deduplicated on UnloadableFiles, which is static and never cleared. In design mode Visual Studio keeps vstest.console alive across many discovery and run requests, so the user was told about a missing extension on the first request and then never again. The same set also skipped the file for the rest of the process, so an extension whose missing dependency has since appeared stayed skipped until the process exits. Split the two concerns. UnloadableFiles still answers whether loading the file is worth another try, ReportedFiles answers whether the user has already seen the warning, and ClearExtensions empties both. The runner calls that before every discovery and run request, so the warning is once per run. Reporting no longer depends on being the first to hit the failed load, the skip path hands the file to the reporting too and the reporting decides. Partial type loads stay quiet, only an assembly that yields no type at all is reported, see microsoft#290. 🤖
| /// dependency has since appeared is tried again on the next request instead of staying broken for the rest | ||
| /// of the process. | ||
| /// </summary> | ||
| private static readonly ConcurrentDictionary<string, string> UnloadableFiles = new(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Only
FileLoadExceptionreached the user. Every other reason a file could not be loaded -BadImageFormatException, a missing dependency, a wrong target framework - went to the diag log only, so the extension was silently missing and the tests it provides silently did not run. That is the worst failure mode we have: a green run that ran nothing.Now any failure to load an extension file is a warning, and it carries the reason instead of only pointing at
/diag:An assembly where
GetTypesthrows and not a single type comes out is reported the same way. When some types do load we stay quiet - the extension usually still works, and the types that failed are usually not extensions at all. That is deliberate, #290 was about exactly that noise.Two things keep it quiet in normal runs:
The warning is once per run, not once per process. It used to be deduplicated on
UnloadableFiles, which is static and never cleared, so 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.UnloadableFilesnow answers only whether loading the file is worth another try, a separateReportedFilesanswers whether the user has already seen the warning, andClearExtensions()empties both. The runner calls that before every discovery and run request. Clearing the first one also means an extension whose missing dependency has since appeared is tried again on the next request, instead of staying skipped until the process exits.I also split the assembly load from the assembly inspection in
GetTestExtensionsFromFiles. Before, onecatchcovered both, so a brokenTestPluginInformationconstructor looked the same as a file that could not be loaded. Inspection failures stay in the diag log as before.FailedToLoadAdapaterFileis replaced byFailedToLoadExtensionFile, which takes the reason. The old string is gone from the resx and the xlf files.Verified against a real run: a folder with a
Broken.TestAdapter.dllthat is not a managed assembly prints the warning once throughvstest.console /lt, and the same run without it prints no new warnings.Added tests for the warning, for the once-per-file behavior, for reporting again after the extension cache is cleared, for the casing of the path, for staying quiet on a partial type load, and for the probe files staying quiet.
Supersedes #16382, which fixed the same issue. Its once-per-run reset and its casing test are carried over here.
🤖