[Test Improver] Add unit tests for LoggingManager.BuildAsync

[Evangelink]

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 (parameterized across Trace, Warning, and Critical levels)
• Non-IExtension provider is always included
• IExtension provider enabled → included; disabled → excluded
• IAsyncInitializableExtension + enabled → InitializeAsync called and provider included in factory
• IAsyncInitializableExtension + disabled → InitializeAsync not called
• Non-extension IAsyncInitializableExtension → InitializeAsync called and provider included in factory
• Multiple providers registered simultaneously → only enabled ones are included (covers the foreach/continue loop)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

[Copilot]

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 AddProvider null-guard behavior and BuildAsync behavior across provider types.
• Validated that BuildAsync forwards LogLevel and IServiceProvider into 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

[Evangelink]

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.BuildAsync implementation. The IExtension enable/disable filtering path and the TryInitializeAsync dispatch (which internally checks is IAsyncInitializableExtension) are both exercised with appropriate combinations of mock interfaces.
• Internal access: LoggingManager and BuildAsync are internal; access is correctly gated by the InternalsVisibleTo declaration for Microsoft.Testing.Platform.UnitTests in 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 Task test methods with proper await usage; no Task.Result/.Wait() anti-patterns.
• No public API surface added: All additions are in test/ with internal interfaces.

Recommendations

None — the change is well-scoped, complete, and correct.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

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:

1. Dead mock setup (line 109) — mockProvider.Setup(p => p.CreateLogger(...)) in BuildAsync_InitializableExtensionProvider_WhenEnabled_CallsInitializeAsync is never triggered since the test body never calls factory.CreateLogger(...). Remove to reduce noise.
2. Implicit return-value discard (lines 68, 83, 97) — factory.CreateLogger("test") is called for its side-effect but the return is silently dropped. LoggerFactoryTests.cs uses _ = here; aligning would clarify intent.
3. Lambda parameter naming (line 19) — Constructor setup uses x; all test-method setups use p. A consistent name (e.g., m or monitor) would read more uniformly.

Recommendations

• Remove the unused CreateLogger setup from the WhenEnabled_CallsInitializeAsync test.
• Prefix the three factory.CreateLogger("test") calls with _ =.
• Rename x → m (or monitor) in the constructor lambda.

---

Generated by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Evangelink]

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:

1. [Structure] Dead CreateLogger mock setup (lines 109, 136) — Both BuildAsync_InitializableExtensionProvider_WhenEnabled_CallsInitializeAsync and BuildAsync_NonExtensionInitializableProvider_CallsInitializeAsync set up CreateLogger on the mock but never call factory.CreateLogger, so the setup is never exercised. This creates misleading test intent.

2. [Coverage] Provider inclusion not verified after initialization (line 141) — BuildAsync_NonExtensionInitializableProvider_CallsInitializeAsync confirms InitializeAsync() is invoked, but does not confirm the provider actually ends up in the factory. A regression breaking loggerProviders.Add(serviceInstance) would go undetected.

3. [Coverage] No multi-provider test (line 103) — The foreach loop logic in BuildAsync is only ever tested with a single provider. A scenario mixing enabled + disabled providers would catch bugs in the loop's continue logic.

Recommendations

• Remove or complete the CreateLogger setups (either drop them or also call factory.CreateLogger + verify).
• Add a factory.CreateLogger + Verify step to BuildAsync_NonExtensionInitializableProvider_CallsInitializeAsync.
• Add one multi-provider test covering mixed enable/disable states.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Evangelink]

[Structure] The CreateLogger setup on this line is dead code — factory.CreateLogger(...) is never called in this test (the return value of BuildAsync on line 112 is discarded), so the mock setup is never exercised.

Impact: Misleads readers into thinking this test also verifies provider inclusion, when it only verifies InitializeAsync() was called.

Suggestion: Either drop the CreateLogger setup (the test's only assertion is on InitializeAsync), or add factory.CreateLogger("test") + mockProvider.Verify(p => p.CreateLogger("test"), Times.Once) if the intent is to also confirm that an enabled provider ends up in the factory. The same pattern also affects the CreateLogger setup on line 136 in BuildAsync_NonExtensionInitializableProvider_CallsInitializeAsync.

[Copilot]

Resolved by adding ILoggerFactory factory = …, _ = factory.CreateLogger("test"), and a Verify for CreateLogger — the setup is now live and the test asserts both initialization and inclusion. Same fix applied to BuildAsync_NonExtensionInitializableProvider_CallsInitializeAsync.

[Evangelink]

@copilot address review comment

[Evangelink]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. The resolved file incorporates changes from both sides: [DataRow] parameterization and Assert.IsNotNull guard from main, _ = discard prefix and BuildAsync_MultipleProviders_OnlyIncludesEnabledOnes test from our branch. All 20 tests pass.

[Evangelink]

@copilot fix the following issues

@azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Windows Debug)
test\UnitTests\MSTestAdapter.PlatformServices.UnitTests\Services\DesktopTestSourceHostTests.cs#L107

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.
Check failure on line 164 in test\UnitTests\MSTestAdapter.PlatformServices.UnitTests\Services\DesktopTestSourceHostTests.cs

@azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Windows Debug)
test\UnitTests\MSTestAdapter.PlatformServices.UnitTests\Services\DesktopTestSourceHostTests.cs#L164

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.
Check failure on line 107 in test\UnitTests\MSTestAdapter.PlatformServices.UnitTests\Services\DesktopTestSourceHostTests.cs

@azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Windows Debug)
test\UnitTests\MSTestAdapter.PlatformServices.UnitTests\Services\DesktopTestSourceHostTests.cs#L107

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.

[Copilot]

@copilot fix the following issues

@azure-pipelines
...

Fixed in the latest commit. Both occurrences of new(location, null) on lines 107 and 164 are now new(location, (IRunSettings?)null) to explicitly cast the null literal and satisfy the CS8625 nullability constraint in the target-typed new context.

[Evangelink]

---

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 _ = discards + BuildAsync_MultipleProviders_OnlyIncludesEnabledOnes test) is valuable, but the CI failure stems from a DesktopTestSourceHostTests.cs change added by conflict resolution (unrelated to the test improvements themselves).

A clean patch with just the LoggingManagerTests.cs quality improvements — no DesktopTestSourceHostTests.cs changes — is available as aw-test-assist-logging-manager-quality-improvements-v1.patch in the workflow run artifacts. Closing this PR and applying the clean patch would be the simplest path forward.

Generated by Daily Test Improver · ● 9.5M · ◷