Uh oh!
There was an error while loading. Please reload this page.
[Test Improver] Add unit tests for LoggingManager.BuildAsync - #8129
Conversation
Cover all key behaviors of LoggingManager: - AddProvider with null throws ArgumentNullException - BuildAsync with no providers returns a non-null factory - Factory delegate receives the correct LogLevel and IServiceProvider - Non-IExtension provider is always included - IExtension provider enabled -> included; disabled -> excluded - IAsyncInitializableExtension + enabled -> InitializeAsync called - IAsyncInitializableExtension + disabled -> InitializeAsync not called - Non-extension IAsyncInitializableExtension -> InitializeAsync called Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds focused unit test coverage for Microsoft.Testing.Platform.Logging.LoggingManager.BuildAsync, validating provider registration, enablement filtering (IExtension.IsEnabledAsync), and asynchronous initialization (IAsyncInitializableExtension.InitializeAsync) behaviors to address #7995.
Changes:
- Added 9 unit tests covering
AddProvidernull-guard behavior andBuildAsyncbehavior across provider types. - Validated that
BuildAsyncforwardsLogLevelandIServiceProviderinto provider factories. - Covered inclusion/exclusion rules for extension-based providers and initialization rules for async-initializable providers.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs | New unit tests covering LoggingManager.BuildAsync provider filtering and initialization paths. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
Summary
Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx
Key Findings
No correctness, threading, security, resource management, or API-compatibility issues were identified.
What was verified:
- Correctness: All 8 test scenarios trace correctly against the
LoggingManager.BuildAsyncimplementation. TheIExtensionenable/disable filtering path and theTryInitializeAsyncdispatch (which internally checksis IAsyncInitializableExtension) are both exercised with appropriate combinations of mock interfaces. - Internal access:
LoggingManagerandBuildAsyncareinternal; access is correctly gated by theInternalsVisibleTodeclaration forMicrosoft.Testing.Platform.UnitTestsin the main project csproj, and Moq's dynamic proxy access is similarly declared. - Mock fidelity: The three test-local interfaces (
IExtensionLoggerProvider,IInitializableLoggerProvider,IInitializableExtensionLoggerProvider) accurately model the real interface combinations the production code branches on. - Threading/async:
async Tasktest methods with properawaitusage; noTask.Result/.Wait()anti-patterns. - No public API surface added: All additions are in
test/withinternalinterfaces.
Recommendations
None — the change is well-scoped, complete, and correct.
Generated by Expert Code Reviewer
🧠 Reviewed by Expert Code Reviewer 🧠
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
Summary
Workflow: PR Nitpick Reviewer
Date: 2026-05-11
Repository: microsoft/testfx
Key Findings
Good overall structure — the tests are well-focused (each verifies exactly one behavior), cover all meaningful branches of LoggingManager.BuildAsync, and follow the Moq patterns established in sibling test files. Three minor nitpicks found:
- Dead mock setup (line 109) —
mockProvider.Setup(p => p.CreateLogger(...))inBuildAsync_InitializableExtensionProvider_WhenEnabled_CallsInitializeAsyncis never triggered since the test body never callsfactory.CreateLogger(...). Remove to reduce noise. - Implicit return-value discard (lines 68, 83, 97) —
factory.CreateLogger("test")is called for its side-effect but the return is silently dropped.LoggerFactoryTests.csuses_ =here; aligning would clarify intent. - Lambda parameter naming (line 19) — Constructor setup uses
x; all test-method setups usep. A consistent name (e.g.,mormonitor) would read more uniformly.
Recommendations
- Remove the unused
CreateLoggersetup from theWhenEnabled_CallsInitializeAsynctest. - Prefix the three
factory.CreateLogger("test")calls with_ =. - Rename
x→m(ormonitor) in the constructor lambda.
Generated by PR Nitpick Reviewer
🔍 Meticulously inspected by PR Nitpick Reviewer
🔍 Meticulously inspected by PR Nitpick Reviewer 🔍
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
Summary
Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx
Key Findings
The new LoggingManagerTests are well-structured, properly isolated (new instance per test, no static mutable state), and cover the primary behaviors of LoggingManager.BuildAsync. The happy/sad path split for extension enable/disable and the initialization checks are solid. Three improvement opportunities were identified:
[Structure] Dead
CreateLoggermock setup (lines 109, 136) — BothBuildAsync_InitializableExtensionProvider_WhenEnabled_CallsInitializeAsyncandBuildAsync_NonExtensionInitializableProvider_CallsInitializeAsyncset upCreateLoggeron the mock but never callfactory.CreateLogger, so the setup is never exercised. This creates misleading test intent.[Coverage] Provider inclusion not verified after initialization (line 141) —
BuildAsync_NonExtensionInitializableProvider_CallsInitializeAsyncconfirmsInitializeAsync()is invoked, but does not confirm the provider actually ends up in the factory. A regression breakingloggerProviders.Add(serviceInstance)would go undetected.[Coverage] No multi-provider test (line 103) — The
foreachloop logic inBuildAsyncis only ever tested with a single provider. A scenario mixing enabled + disabled providers would catch bugs in the loop'scontinuelogic.
Recommendations
- Remove or complete the
CreateLoggersetups (either drop them or also callfactory.CreateLogger+ verify). - Add a
factory.CreateLogger+Verifystep toBuildAsync_NonExtensionInitializableProvider_CallsInitializeAsync. - Add one multi-provider test covering mixed enable/disable states.
Generated by Test Expert Reviewer
🧪 Test quality reviewed by Test Expert Reviewer 🧪
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Amaury Levé (Evangelink)
commented
May 11, 2026
Copilot address review comment |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Amaury Levé (Evangelink)
commented
May 11, 2026
Copilot resolve the merge conflicts in this pull request |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Merge conflicts resolved. The resolved file incorporates changes from both sides: |
Amaury Levé (Evangelink)
commented
May 11, 2026
Copilot fix the following issues azure-pipelines test\UnitTests\MSTestAdapter.PlatformServices.UnitTests\Services\DesktopTestSourceHostTests.cs(107,57): error CS8625: (NETCORE_ENGINEERING_TELEMETRY=Build) Cannot convert null literal to non-nullable reference type. azure-pipelines test\UnitTests\MSTestAdapter.PlatformServices.UnitTests\Services\DesktopTestSourceHostTests.cs(164,57): error CS8625: (NETCORE_ENGINEERING_TELEMETRY=Build) Cannot convert null literal to non-nullable reference type. azure-pipelines test\UnitTests\MSTestAdapter.PlatformServices.UnitTests\Services\DesktopTestSourceHostTests.cs(107,57): error CS8625: (NETCORE_ENGINEERING_TELEMETRY=Build) Cannot convert null literal to non-nullable reference type. |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Fixed in the latest commit. Both occurrences of |
Amaury Levé (Evangelink)
commented
May 12, 2026
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #8140 🤖 Test Improver here. Status update: the core LoggingManager tests merged in #8130. The unique value in this PR (explicit A clean patch with just the
|
…anager-buildAsync-tests-7351033ae996670b # Conflicts: # test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/DesktopTestSourceHostTests.cs
Uh oh!
There was an error while loading. Please reload this page.
Amaury Levé (Evangelink)
commented
May 13, 2026
Copilot resolve the merge conflicts in this pull request |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Merge conflicts resolved. Adopted main's cleaner structure (nested interfaces, updated test names) and added |
Uh oh!
There was an error while loading. Please reload this page.
Cover all key behaviors of LoggingManager:
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>