[Test Improver] Add unit tests for LoggingManager.BuildAsync

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider with null throws ArgumentNullException
• BuildAsync with no providers returns a functional empty factory (verifies both factory and returned logger are non-null)
• Factory delegate receives correct LogLevel and IServiceProvider (parameterized with multiple log levels)
• Non-IExtension provider is always included
• Enabled IExtension provider is included
• Disabled IExtension provider is excluded
• Non-extension IAsyncInitializableExtension provider has InitializeAsync called
• Enabled IExtension + IAsyncInitializableExtension provider is included and has InitializeAsync called
• Disabled IExtension + IAsyncInitializableExtension provider does not have InitializeAsync called

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

[Amaury Levé (Evangelink)]

Summary

Workflow: PR Nitpick Reviewer 🔍
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

Good addition of unit tests for LoggingManager.BuildAsync — the coverage is thorough, covering no providers, enabled/disabled extension providers, and initializable providers. A few minor consistency issues worth addressing:

1. Important — Mock naming inconsistency: Class-level fields use _mockXxx prefix while local test variables use xxxMock suffix. Picking one convention for the whole file improves readability.
2. Minor — Silent discard of CreateLogger return value (lines 72, 89, 104): Using _ = factory.CreateLogger(...) makes the intentional discard explicit.
3. Minor — Returns(Task.CompletedTask) vs ReturnsAsync() (line 115): All other async setups use ReturnsAsync(...); a small inconsistency worth aligning.

Positive Highlights

• Excellent use of helper test interfaces at the bottom of the file to combine ILoggerProvider, IExtension, and IAsyncInitializableExtension — clean and reusable.
• Test method names are descriptive and follow the Method_Condition_ExpectedResult pattern consistently.
• Good use of Times.Once / Times.Never for precise mock verification.

---

Generated by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Copilot]

Pull request overview

Adds unit tests for Microsoft.Testing.Platform.Logging.LoggingManager.BuildAsync, which builds the platform ILoggerFactory by evaluating provider factories, filtering disabled extensions, and initializing async-initializable providers.

Changes:

• Added a new LoggingManagerTests suite covering null guards, provider inclusion/exclusion, and initialization behavior.
• Introduced lightweight internal marker interfaces in the test file to mock combinations of ILoggerProvider + IExtension + IAsyncInitializableExtension.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	Adds new unit tests targeting LoggingManager.BuildAsync behaviors around provider creation, extension enablement filtering, and async initialization.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

1. [Assertion] Near-tautological assertion in BuildAsync_WithNoProviders_ReturnsEmptyLoggerFactory (line 36): Assert.IsNotNull(factory) can never fail since BuildAsync always constructs a new LoggerFactory. The test doesn't verify the "empty" behavior its name implies.

2. [Coverage] Missing test for enabled IExtensionInitializableLoggerProvider (line 126): The disabled case is tested, but there's no test for when IsEnabledAsync() returns true on a provider that implements both IExtension and IAsyncInitializableExtension. The production code has a distinct path where such a provider must be included AND have InitializeAsync() called — currently uncovered.

What's Well Done

• Good isolation: fresh mock instances per test class instance, no static mutable state
• The factory-delegate capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and effective
• Correct use of Times.Never to verify exclusion paths
• Helper interfaces at the bottom of the file (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a good pattern for multi-interface mocking without polluting production code

Recommendations

1. Strengthen the no-providers test to actually exercise the factory (e.g., call factory.CreateLogger("test") and verify a logger is returned without throwing)
2. Add BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider to cover the enabled + initializable extension path

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Review Scope

Single file added: test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs — 144 lines of unit tests for the internal LoggingManager.BuildAsync method.

Analysis

I reviewed the test code against the production logic in LoggingManager.BuildAsync and the supporting types (LoggerFactory, TryInitializeAsync, IExtension, IAsyncInitializableExtension).

Correctness ✅ All 8 tests accurately model the production branching logic:

• The disabled-provider guard (IExtension.IsEnabledAsync()) is correctly exercised by tests 5, 6, and 8, each with properly-configured IsEnabledAsync mock setup so no await null risk.
• The initialization path (IAsyncInitializableExtension.InitializeAsync() via TryInitializeAsync) is correctly targeted by tests 7 and 8.
• The Times.Once / Times.Never verifications are sound — they would catch bugs in both directions.

Mock setup ✅ All mocks that implement IExtension have IsEnabledAsync() explicitly set up, avoiding the await null footgun from Moq's loose default for Task-returning members.

Threading ✅ _mockMonitor.Lock(...) is correctly wired to return a disposable, satisfying the using block in LoggerFactory.CreateLogger.

IDisposable — ILoggerFactory instances are not disposed after use, but this is inconsequential since all providers are mock objects with no real resources; no improvement needed here.

Internal interfaces — The three internal marker interfaces (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a clean test-local pattern consistent with the existing IDisposableLoggerProvider in LoggerFactoryTests.cs.

Key Findings

No correctness, threading, performance, API compatibility, or security issues found.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

All review comments were addressed in the previous commit: mock naming standardized (mockXxx prefix), explicit discards added, Returns(Task.CompletedTask) used consistently (Moq 4.18.4 doesn't support ReturnsAsync() with no args), no-providers test strengthened with an actual CreateLogger assertion, and BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider added for the missing coverage path.

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. The add/add conflict in LoggingManagerTests.cs was resolved by taking main's structure (shorter test names, DataRow parameterization) and applying our improvements on top (explicit _ = discards, stronger no-providers assertion).

[Copilot]

Copilot resolve the merge conflicts in this pull request

Already resolved in the previous commit — the add/add conflict in LoggingManagerTests.cs was resolved by adopting main's structure with our improvements applied on top.

[Amaury Levé (Evangelink)]

Copilot fix 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.
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.

[Copilot]

Copilot fix issues azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Windows Debug)
...

Fixed in the latest commit. Both Mock<TestSourceHost> constructor calls that were passing a bare null for the run settings argument (lines 107 and 164 in the original, now 107 and 147 after minor line shifts) have been updated to use (IRunSettings?)null, which satisfies the nullable reference type annotation and resolves both CS8625 errors.

[Amaury Levé (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 LoggingManager tests from this PR landed in main via #8124 and #8130. This PR is a duplicate and can be safely closed.

Generated by Daily Test Improver · ● 9.5M · ◷

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in commit 0aad6c4. The two conflicted files (LoggingManagerTests.cs and DesktopTestSourceHostTests.cs) both took main's version, which includes the restructured tests (interfaces at namespace level, updated test names) and null! for the run settings arguments.

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider with null throws ArgumentNullException
• BuildAsync with no providers returns a functional empty factory (verifies both factory and returned logger are non-null)
• Factory delegate receives correct LogLevel and IServiceProvider (parameterized with multiple log levels)
• Non-IExtension provider is always included
• Enabled IExtension provider is included
• Disabled IExtension provider is excluded
• Non-extension IAsyncInitializableExtension provider has InitializeAsync called
• Enabled IExtension + IAsyncInitializableExtension provider is included and has InitializeAsync called
• Disabled IExtension + IAsyncInitializableExtension provider does not have InitializeAsync called

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

[Amaury Levé (Evangelink)]

Summary

Workflow: PR Nitpick Reviewer 🔍
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

Good addition of unit tests for LoggingManager.BuildAsync — the coverage is thorough, covering no providers, enabled/disabled extension providers, and initializable providers. A few minor consistency issues worth addressing:

1. Important — Mock naming inconsistency: Class-level fields use _mockXxx prefix while local test variables use xxxMock suffix. Picking one convention for the whole file improves readability.
2. Minor — Silent discard of CreateLogger return value (lines 72, 89, 104): Using _ = factory.CreateLogger(...) makes the intentional discard explicit.
3. Minor — Returns(Task.CompletedTask) vs ReturnsAsync() (line 115): All other async setups use ReturnsAsync(...); a small inconsistency worth aligning.

Positive Highlights

• Excellent use of helper test interfaces at the bottom of the file to combine ILoggerProvider, IExtension, and IAsyncInitializableExtension — clean and reusable.
• Test method names are descriptive and follow the Method_Condition_ExpectedResult pattern consistently.
• Good use of Times.Once / Times.Never for precise mock verification.

---

Generated by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Copilot]

Pull request overview

Adds unit tests for Microsoft.Testing.Platform.Logging.LoggingManager.BuildAsync, which builds the platform ILoggerFactory by evaluating provider factories, filtering disabled extensions, and initializing async-initializable providers.

Changes:

• Added a new LoggingManagerTests suite covering null guards, provider inclusion/exclusion, and initialization behavior.
• Introduced lightweight internal marker interfaces in the test file to mock combinations of ILoggerProvider + IExtension + IAsyncInitializableExtension.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	Adds new unit tests targeting LoggingManager.BuildAsync behaviors around provider creation, extension enablement filtering, and async initialization.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

1. [Assertion] Near-tautological assertion in BuildAsync_WithNoProviders_ReturnsEmptyLoggerFactory (line 36): Assert.IsNotNull(factory) can never fail since BuildAsync always constructs a new LoggerFactory. The test doesn't verify the "empty" behavior its name implies.

2. [Coverage] Missing test for enabled IExtensionInitializableLoggerProvider (line 126): The disabled case is tested, but there's no test for when IsEnabledAsync() returns true on a provider that implements both IExtension and IAsyncInitializableExtension. The production code has a distinct path where such a provider must be included AND have InitializeAsync() called — currently uncovered.

What's Well Done

• Good isolation: fresh mock instances per test class instance, no static mutable state
• The factory-delegate capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and effective
• Correct use of Times.Never to verify exclusion paths
• Helper interfaces at the bottom of the file (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a good pattern for multi-interface mocking without polluting production code

Recommendations

1. Strengthen the no-providers test to actually exercise the factory (e.g., call factory.CreateLogger("test") and verify a logger is returned without throwing)
2. Add BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider to cover the enabled + initializable extension path

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Review Scope

Single file added: test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs — 144 lines of unit tests for the internal LoggingManager.BuildAsync method.

Analysis

I reviewed the test code against the production logic in LoggingManager.BuildAsync and the supporting types (LoggerFactory, TryInitializeAsync, IExtension, IAsyncInitializableExtension).

Correctness ✅ All 8 tests accurately model the production branching logic:

• The disabled-provider guard (IExtension.IsEnabledAsync()) is correctly exercised by tests 5, 6, and 8, each with properly-configured IsEnabledAsync mock setup so no await null risk.
• The initialization path (IAsyncInitializableExtension.InitializeAsync() via TryInitializeAsync) is correctly targeted by tests 7 and 8.
• The Times.Once / Times.Never verifications are sound — they would catch bugs in both directions.

Mock setup ✅ All mocks that implement IExtension have IsEnabledAsync() explicitly set up, avoiding the await null footgun from Moq's loose default for Task-returning members.

Threading ✅ _mockMonitor.Lock(...) is correctly wired to return a disposable, satisfying the using block in LoggerFactory.CreateLogger.

IDisposable — ILoggerFactory instances are not disposed after use, but this is inconsequential since all providers are mock objects with no real resources; no improvement needed here.

Internal interfaces — The three internal marker interfaces (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a clean test-local pattern consistent with the existing IDisposableLoggerProvider in LoggerFactoryTests.cs.

Key Findings

No correctness, threading, performance, API compatibility, or security issues found.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

All review comments were addressed in the previous commit: mock naming standardized (mockXxx prefix), explicit discards added, Returns(Task.CompletedTask) used consistently (Moq 4.18.4 doesn't support ReturnsAsync() with no args), no-providers test strengthened with an actual CreateLogger assertion, and BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider added for the missing coverage path.

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. The add/add conflict in LoggingManagerTests.cs was resolved by taking main's structure (shorter test names, DataRow parameterization) and applying our improvements on top (explicit _ = discards, stronger no-providers assertion).

[Copilot]

Copilot resolve the merge conflicts in this pull request

Already resolved in the previous commit — the add/add conflict in LoggingManagerTests.cs was resolved by adopting main's structure with our improvements applied on top.

[Amaury Levé (Evangelink)]

Copilot fix 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.
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.

[Copilot]

Copilot fix issues azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Windows Debug)
...

Fixed in the latest commit. Both Mock<TestSourceHost> constructor calls that were passing a bare null for the run settings argument (lines 107 and 164 in the original, now 107 and 147 after minor line shifts) have been updated to use (IRunSettings?)null, which satisfies the nullable reference type annotation and resolves both CS8625 errors.

[Amaury Levé (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 LoggingManager tests from this PR landed in main via #8124 and #8130. This PR is a duplicate and can be safely closed.

Generated by Daily Test Improver · ● 9.5M · ◷

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in commit 0aad6c4. The two conflicted files (LoggingManagerTests.cs and DesktopTestSourceHostTests.cs) both took main's version, which includes the restructured tests (interfaces at namespace level, updated test names) and null! for the run settings arguments.

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider with null throws ArgumentNullException
• BuildAsync with no providers returns a functional empty factory (verifies both factory and returned logger are non-null)
• Factory delegate receives correct LogLevel and IServiceProvider (parameterized with multiple log levels)
• Non-IExtension provider is always included
• Enabled IExtension provider is included
• Disabled IExtension provider is excluded
• Non-extension IAsyncInitializableExtension provider has InitializeAsync called
• Enabled IExtension + IAsyncInitializableExtension provider is included and has InitializeAsync called
• Disabled IExtension + IAsyncInitializableExtension provider does not have InitializeAsync called

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

[Amaury Levé (Evangelink)]

Summary

Workflow: PR Nitpick Reviewer 🔍
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

Good addition of unit tests for LoggingManager.BuildAsync — the coverage is thorough, covering no providers, enabled/disabled extension providers, and initializable providers. A few minor consistency issues worth addressing:

1. Important — Mock naming inconsistency: Class-level fields use _mockXxx prefix while local test variables use xxxMock suffix. Picking one convention for the whole file improves readability.
2. Minor — Silent discard of CreateLogger return value (lines 72, 89, 104): Using _ = factory.CreateLogger(...) makes the intentional discard explicit.
3. Minor — Returns(Task.CompletedTask) vs ReturnsAsync() (line 115): All other async setups use ReturnsAsync(...); a small inconsistency worth aligning.

Positive Highlights

• Excellent use of helper test interfaces at the bottom of the file to combine ILoggerProvider, IExtension, and IAsyncInitializableExtension — clean and reusable.
• Test method names are descriptive and follow the Method_Condition_ExpectedResult pattern consistently.
• Good use of Times.Once / Times.Never for precise mock verification.

---

Generated by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Copilot]

Pull request overview

Adds unit tests for Microsoft.Testing.Platform.Logging.LoggingManager.BuildAsync, which builds the platform ILoggerFactory by evaluating provider factories, filtering disabled extensions, and initializing async-initializable providers.

Changes:

• Added a new LoggingManagerTests suite covering null guards, provider inclusion/exclusion, and initialization behavior.
• Introduced lightweight internal marker interfaces in the test file to mock combinations of ILoggerProvider + IExtension + IAsyncInitializableExtension.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	Adds new unit tests targeting LoggingManager.BuildAsync behaviors around provider creation, extension enablement filtering, and async initialization.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

1. [Assertion] Near-tautological assertion in BuildAsync_WithNoProviders_ReturnsEmptyLoggerFactory (line 36): Assert.IsNotNull(factory) can never fail since BuildAsync always constructs a new LoggerFactory. The test doesn't verify the "empty" behavior its name implies.

2. [Coverage] Missing test for enabled IExtensionInitializableLoggerProvider (line 126): The disabled case is tested, but there's no test for when IsEnabledAsync() returns true on a provider that implements both IExtension and IAsyncInitializableExtension. The production code has a distinct path where such a provider must be included AND have InitializeAsync() called — currently uncovered.

What's Well Done

• Good isolation: fresh mock instances per test class instance, no static mutable state
• The factory-delegate capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and effective
• Correct use of Times.Never to verify exclusion paths
• Helper interfaces at the bottom of the file (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a good pattern for multi-interface mocking without polluting production code

Recommendations

1. Strengthen the no-providers test to actually exercise the factory (e.g., call factory.CreateLogger("test") and verify a logger is returned without throwing)
2. Add BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider to cover the enabled + initializable extension path

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Review Scope

Single file added: test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs — 144 lines of unit tests for the internal LoggingManager.BuildAsync method.

Analysis

I reviewed the test code against the production logic in LoggingManager.BuildAsync and the supporting types (LoggerFactory, TryInitializeAsync, IExtension, IAsyncInitializableExtension).

Correctness ✅ All 8 tests accurately model the production branching logic:

• The disabled-provider guard (IExtension.IsEnabledAsync()) is correctly exercised by tests 5, 6, and 8, each with properly-configured IsEnabledAsync mock setup so no await null risk.
• The initialization path (IAsyncInitializableExtension.InitializeAsync() via TryInitializeAsync) is correctly targeted by tests 7 and 8.
• The Times.Once / Times.Never verifications are sound — they would catch bugs in both directions.

Mock setup ✅ All mocks that implement IExtension have IsEnabledAsync() explicitly set up, avoiding the await null footgun from Moq's loose default for Task-returning members.

Threading ✅ _mockMonitor.Lock(...) is correctly wired to return a disposable, satisfying the using block in LoggerFactory.CreateLogger.

IDisposable — ILoggerFactory instances are not disposed after use, but this is inconsequential since all providers are mock objects with no real resources; no improvement needed here.

Internal interfaces — The three internal marker interfaces (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a clean test-local pattern consistent with the existing IDisposableLoggerProvider in LoggerFactoryTests.cs.

Key Findings

No correctness, threading, performance, API compatibility, or security issues found.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

All review comments were addressed in the previous commit: mock naming standardized (mockXxx prefix), explicit discards added, Returns(Task.CompletedTask) used consistently (Moq 4.18.4 doesn't support ReturnsAsync() with no args), no-providers test strengthened with an actual CreateLogger assertion, and BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider added for the missing coverage path.

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. The add/add conflict in LoggingManagerTests.cs was resolved by taking main's structure (shorter test names, DataRow parameterization) and applying our improvements on top (explicit _ = discards, stronger no-providers assertion).

[Copilot]

Copilot resolve the merge conflicts in this pull request

Already resolved in the previous commit — the add/add conflict in LoggingManagerTests.cs was resolved by adopting main's structure with our improvements applied on top.

[Amaury Levé (Evangelink)]

Copilot fix 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.
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.

[Copilot]

Copilot fix issues azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Windows Debug)
...

Fixed in the latest commit. Both Mock<TestSourceHost> constructor calls that were passing a bare null for the run settings argument (lines 107 and 164 in the original, now 107 and 147 after minor line shifts) have been updated to use (IRunSettings?)null, which satisfies the nullable reference type annotation and resolves both CS8625 errors.

[Amaury Levé (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 LoggingManager tests from this PR landed in main via #8124 and #8130. This PR is a duplicate and can be safely closed.

Generated by Daily Test Improver · ● 9.5M · ◷

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in commit 0aad6c4. The two conflicted files (LoggingManagerTests.cs and DesktopTestSourceHostTests.cs) both took main's version, which includes the restructured tests (interfaces at namespace level, updated test names) and null! for the run settings arguments.

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider with null throws ArgumentNullException
• BuildAsync with no providers returns a functional empty factory (verifies both factory and returned logger are non-null)
• Factory delegate receives correct LogLevel and IServiceProvider (parameterized with multiple log levels)
• Non-IExtension provider is always included
• Enabled IExtension provider is included
• Disabled IExtension provider is excluded
• Non-extension IAsyncInitializableExtension provider has InitializeAsync called
• Enabled IExtension + IAsyncInitializableExtension provider is included and has InitializeAsync called
• Disabled IExtension + IAsyncInitializableExtension provider does not have InitializeAsync called

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

[Amaury Levé (Evangelink)]

Summary

Workflow: PR Nitpick Reviewer 🔍
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

Good addition of unit tests for LoggingManager.BuildAsync — the coverage is thorough, covering no providers, enabled/disabled extension providers, and initializable providers. A few minor consistency issues worth addressing:

1. Important — Mock naming inconsistency: Class-level fields use _mockXxx prefix while local test variables use xxxMock suffix. Picking one convention for the whole file improves readability.
2. Minor — Silent discard of CreateLogger return value (lines 72, 89, 104): Using _ = factory.CreateLogger(...) makes the intentional discard explicit.
3. Minor — Returns(Task.CompletedTask) vs ReturnsAsync() (line 115): All other async setups use ReturnsAsync(...); a small inconsistency worth aligning.

Positive Highlights

• Excellent use of helper test interfaces at the bottom of the file to combine ILoggerProvider, IExtension, and IAsyncInitializableExtension — clean and reusable.
• Test method names are descriptive and follow the Method_Condition_ExpectedResult pattern consistently.
• Good use of Times.Once / Times.Never for precise mock verification.

---

Generated by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Copilot]

Pull request overview

Adds unit tests for Microsoft.Testing.Platform.Logging.LoggingManager.BuildAsync, which builds the platform ILoggerFactory by evaluating provider factories, filtering disabled extensions, and initializing async-initializable providers.

Changes:

• Added a new LoggingManagerTests suite covering null guards, provider inclusion/exclusion, and initialization behavior.
• Introduced lightweight internal marker interfaces in the test file to mock combinations of ILoggerProvider + IExtension + IAsyncInitializableExtension.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	Adds new unit tests targeting LoggingManager.BuildAsync behaviors around provider creation, extension enablement filtering, and async initialization.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

1. [Assertion] Near-tautological assertion in BuildAsync_WithNoProviders_ReturnsEmptyLoggerFactory (line 36): Assert.IsNotNull(factory) can never fail since BuildAsync always constructs a new LoggerFactory. The test doesn't verify the "empty" behavior its name implies.

2. [Coverage] Missing test for enabled IExtensionInitializableLoggerProvider (line 126): The disabled case is tested, but there's no test for when IsEnabledAsync() returns true on a provider that implements both IExtension and IAsyncInitializableExtension. The production code has a distinct path where such a provider must be included AND have InitializeAsync() called — currently uncovered.

What's Well Done

• Good isolation: fresh mock instances per test class instance, no static mutable state
• The factory-delegate capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and effective
• Correct use of Times.Never to verify exclusion paths
• Helper interfaces at the bottom of the file (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a good pattern for multi-interface mocking without polluting production code

Recommendations

1. Strengthen the no-providers test to actually exercise the factory (e.g., call factory.CreateLogger("test") and verify a logger is returned without throwing)
2. Add BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider to cover the enabled + initializable extension path

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Review Scope

Single file added: test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs — 144 lines of unit tests for the internal LoggingManager.BuildAsync method.

Analysis

I reviewed the test code against the production logic in LoggingManager.BuildAsync and the supporting types (LoggerFactory, TryInitializeAsync, IExtension, IAsyncInitializableExtension).

Correctness ✅ All 8 tests accurately model the production branching logic:

• The disabled-provider guard (IExtension.IsEnabledAsync()) is correctly exercised by tests 5, 6, and 8, each with properly-configured IsEnabledAsync mock setup so no await null risk.
• The initialization path (IAsyncInitializableExtension.InitializeAsync() via TryInitializeAsync) is correctly targeted by tests 7 and 8.
• The Times.Once / Times.Never verifications are sound — they would catch bugs in both directions.

Mock setup ✅ All mocks that implement IExtension have IsEnabledAsync() explicitly set up, avoiding the await null footgun from Moq's loose default for Task-returning members.

Threading ✅ _mockMonitor.Lock(...) is correctly wired to return a disposable, satisfying the using block in LoggerFactory.CreateLogger.

IDisposable — ILoggerFactory instances are not disposed after use, but this is inconsequential since all providers are mock objects with no real resources; no improvement needed here.

Internal interfaces — The three internal marker interfaces (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a clean test-local pattern consistent with the existing IDisposableLoggerProvider in LoggerFactoryTests.cs.

Key Findings

No correctness, threading, performance, API compatibility, or security issues found.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

All review comments were addressed in the previous commit: mock naming standardized (mockXxx prefix), explicit discards added, Returns(Task.CompletedTask) used consistently (Moq 4.18.4 doesn't support ReturnsAsync() with no args), no-providers test strengthened with an actual CreateLogger assertion, and BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider added for the missing coverage path.

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. The add/add conflict in LoggingManagerTests.cs was resolved by taking main's structure (shorter test names, DataRow parameterization) and applying our improvements on top (explicit _ = discards, stronger no-providers assertion).

[Copilot]

Copilot resolve the merge conflicts in this pull request

Already resolved in the previous commit — the add/add conflict in LoggingManagerTests.cs was resolved by adopting main's structure with our improvements applied on top.

[Amaury Levé (Evangelink)]

Copilot fix 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.
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.

[Copilot]

Copilot fix issues azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Windows Debug)
...

Fixed in the latest commit. Both Mock<TestSourceHost> constructor calls that were passing a bare null for the run settings argument (lines 107 and 164 in the original, now 107 and 147 after minor line shifts) have been updated to use (IRunSettings?)null, which satisfies the nullable reference type annotation and resolves both CS8625 errors.

[Amaury Levé (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 LoggingManager tests from this PR landed in main via #8124 and #8130. This PR is a duplicate and can be safely closed.

Generated by Daily Test Improver · ● 9.5M · ◷

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in commit 0aad6c4. The two conflicted files (LoggingManagerTests.cs and DesktopTestSourceHostTests.cs) both took main's version, which includes the restructured tests (interfaces at namespace level, updated test names) and null! for the run settings arguments.

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider with null throws ArgumentNullException
• BuildAsync with no providers returns a functional empty factory (verifies both factory and returned logger are non-null)
• Factory delegate receives correct LogLevel and IServiceProvider (parameterized with multiple log levels)
• Non-IExtension provider is always included
• Enabled IExtension provider is included
• Disabled IExtension provider is excluded
• Non-extension IAsyncInitializableExtension provider has InitializeAsync called
• Enabled IExtension + IAsyncInitializableExtension provider is included and has InitializeAsync called
• Disabled IExtension + IAsyncInitializableExtension provider does not have InitializeAsync called

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

[Amaury Levé (Evangelink)]

Summary

Workflow: PR Nitpick Reviewer 🔍
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

Good addition of unit tests for LoggingManager.BuildAsync — the coverage is thorough, covering no providers, enabled/disabled extension providers, and initializable providers. A few minor consistency issues worth addressing:

1. Important — Mock naming inconsistency: Class-level fields use _mockXxx prefix while local test variables use xxxMock suffix. Picking one convention for the whole file improves readability.
2. Minor — Silent discard of CreateLogger return value (lines 72, 89, 104): Using _ = factory.CreateLogger(...) makes the intentional discard explicit.
3. Minor — Returns(Task.CompletedTask) vs ReturnsAsync() (line 115): All other async setups use ReturnsAsync(...); a small inconsistency worth aligning.

Positive Highlights

• Excellent use of helper test interfaces at the bottom of the file to combine ILoggerProvider, IExtension, and IAsyncInitializableExtension — clean and reusable.
• Test method names are descriptive and follow the Method_Condition_ExpectedResult pattern consistently.
• Good use of Times.Once / Times.Never for precise mock verification.

---

Generated by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Copilot]

Pull request overview

Adds unit tests for Microsoft.Testing.Platform.Logging.LoggingManager.BuildAsync, which builds the platform ILoggerFactory by evaluating provider factories, filtering disabled extensions, and initializing async-initializable providers.

Changes:

• Added a new LoggingManagerTests suite covering null guards, provider inclusion/exclusion, and initialization behavior.
• Introduced lightweight internal marker interfaces in the test file to mock combinations of ILoggerProvider + IExtension + IAsyncInitializableExtension.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	Adds new unit tests targeting LoggingManager.BuildAsync behaviors around provider creation, extension enablement filtering, and async initialization.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

1. [Assertion] Near-tautological assertion in BuildAsync_WithNoProviders_ReturnsEmptyLoggerFactory (line 36): Assert.IsNotNull(factory) can never fail since BuildAsync always constructs a new LoggerFactory. The test doesn't verify the "empty" behavior its name implies.

2. [Coverage] Missing test for enabled IExtensionInitializableLoggerProvider (line 126): The disabled case is tested, but there's no test for when IsEnabledAsync() returns true on a provider that implements both IExtension and IAsyncInitializableExtension. The production code has a distinct path where such a provider must be included AND have InitializeAsync() called — currently uncovered.

What's Well Done

• Good isolation: fresh mock instances per test class instance, no static mutable state
• The factory-delegate capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and effective
• Correct use of Times.Never to verify exclusion paths
• Helper interfaces at the bottom of the file (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a good pattern for multi-interface mocking without polluting production code

Recommendations

1. Strengthen the no-providers test to actually exercise the factory (e.g., call factory.CreateLogger("test") and verify a logger is returned without throwing)
2. Add BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider to cover the enabled + initializable extension path

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Review Scope

Single file added: test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs — 144 lines of unit tests for the internal LoggingManager.BuildAsync method.

Analysis

I reviewed the test code against the production logic in LoggingManager.BuildAsync and the supporting types (LoggerFactory, TryInitializeAsync, IExtension, IAsyncInitializableExtension).

Correctness ✅ All 8 tests accurately model the production branching logic:

• The disabled-provider guard (IExtension.IsEnabledAsync()) is correctly exercised by tests 5, 6, and 8, each with properly-configured IsEnabledAsync mock setup so no await null risk.
• The initialization path (IAsyncInitializableExtension.InitializeAsync() via TryInitializeAsync) is correctly targeted by tests 7 and 8.
• The Times.Once / Times.Never verifications are sound — they would catch bugs in both directions.

Mock setup ✅ All mocks that implement IExtension have IsEnabledAsync() explicitly set up, avoiding the await null footgun from Moq's loose default for Task-returning members.

Threading ✅ _mockMonitor.Lock(...) is correctly wired to return a disposable, satisfying the using block in LoggerFactory.CreateLogger.

IDisposable — ILoggerFactory instances are not disposed after use, but this is inconsequential since all providers are mock objects with no real resources; no improvement needed here.

Internal interfaces — The three internal marker interfaces (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a clean test-local pattern consistent with the existing IDisposableLoggerProvider in LoggerFactoryTests.cs.

Key Findings

No correctness, threading, performance, API compatibility, or security issues found.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

All review comments were addressed in the previous commit: mock naming standardized (mockXxx prefix), explicit discards added, Returns(Task.CompletedTask) used consistently (Moq 4.18.4 doesn't support ReturnsAsync() with no args), no-providers test strengthened with an actual CreateLogger assertion, and BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider added for the missing coverage path.

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. The add/add conflict in LoggingManagerTests.cs was resolved by taking main's structure (shorter test names, DataRow parameterization) and applying our improvements on top (explicit _ = discards, stronger no-providers assertion).

[Copilot]

Copilot resolve the merge conflicts in this pull request

Already resolved in the previous commit — the add/add conflict in LoggingManagerTests.cs was resolved by adopting main's structure with our improvements applied on top.

[Amaury Levé (Evangelink)]

Copilot fix 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.
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.

[Copilot]

Copilot fix issues azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Windows Debug)
...

Fixed in the latest commit. Both Mock<TestSourceHost> constructor calls that were passing a bare null for the run settings argument (lines 107 and 164 in the original, now 107 and 147 after minor line shifts) have been updated to use (IRunSettings?)null, which satisfies the nullable reference type annotation and resolves both CS8625 errors.

[Amaury Levé (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 LoggingManager tests from this PR landed in main via #8124 and #8130. This PR is a duplicate and can be safely closed.

Generated by Daily Test Improver · ● 9.5M · ◷

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in commit 0aad6c4. The two conflicted files (LoggingManagerTests.cs and DesktopTestSourceHostTests.cs) both took main's version, which includes the restructured tests (interfaces at namespace level, updated test names) and null! for the run settings arguments.

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider with null throws ArgumentNullException
• BuildAsync with no providers returns a functional empty factory (verifies both factory and returned logger are non-null)
• Factory delegate receives correct LogLevel and IServiceProvider (parameterized with multiple log levels)
• Non-IExtension provider is always included
• Enabled IExtension provider is included
• Disabled IExtension provider is excluded
• Non-extension IAsyncInitializableExtension provider has InitializeAsync called
• Enabled IExtension + IAsyncInitializableExtension provider is included and has InitializeAsync called
• Disabled IExtension + IAsyncInitializableExtension provider does not have InitializeAsync called

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

[Amaury Levé (Evangelink)]

Summary

Workflow: PR Nitpick Reviewer 🔍
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

Good addition of unit tests for LoggingManager.BuildAsync — the coverage is thorough, covering no providers, enabled/disabled extension providers, and initializable providers. A few minor consistency issues worth addressing:

1. Important — Mock naming inconsistency: Class-level fields use _mockXxx prefix while local test variables use xxxMock suffix. Picking one convention for the whole file improves readability.
2. Minor — Silent discard of CreateLogger return value (lines 72, 89, 104): Using _ = factory.CreateLogger(...) makes the intentional discard explicit.
3. Minor — Returns(Task.CompletedTask) vs ReturnsAsync() (line 115): All other async setups use ReturnsAsync(...); a small inconsistency worth aligning.

Positive Highlights

• Excellent use of helper test interfaces at the bottom of the file to combine ILoggerProvider, IExtension, and IAsyncInitializableExtension — clean and reusable.
• Test method names are descriptive and follow the Method_Condition_ExpectedResult pattern consistently.
• Good use of Times.Once / Times.Never for precise mock verification.

---

Generated by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Copilot]

Pull request overview

Adds unit tests for Microsoft.Testing.Platform.Logging.LoggingManager.BuildAsync, which builds the platform ILoggerFactory by evaluating provider factories, filtering disabled extensions, and initializing async-initializable providers.

Changes:

• Added a new LoggingManagerTests suite covering null guards, provider inclusion/exclusion, and initialization behavior.
• Introduced lightweight internal marker interfaces in the test file to mock combinations of ILoggerProvider + IExtension + IAsyncInitializableExtension.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	Adds new unit tests targeting LoggingManager.BuildAsync behaviors around provider creation, extension enablement filtering, and async initialization.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

1. [Assertion] Near-tautological assertion in BuildAsync_WithNoProviders_ReturnsEmptyLoggerFactory (line 36): Assert.IsNotNull(factory) can never fail since BuildAsync always constructs a new LoggerFactory. The test doesn't verify the "empty" behavior its name implies.

2. [Coverage] Missing test for enabled IExtensionInitializableLoggerProvider (line 126): The disabled case is tested, but there's no test for when IsEnabledAsync() returns true on a provider that implements both IExtension and IAsyncInitializableExtension. The production code has a distinct path where such a provider must be included AND have InitializeAsync() called — currently uncovered.

What's Well Done

• Good isolation: fresh mock instances per test class instance, no static mutable state
• The factory-delegate capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and effective
• Correct use of Times.Never to verify exclusion paths
• Helper interfaces at the bottom of the file (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a good pattern for multi-interface mocking without polluting production code

Recommendations

1. Strengthen the no-providers test to actually exercise the factory (e.g., call factory.CreateLogger("test") and verify a logger is returned without throwing)
2. Add BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider to cover the enabled + initializable extension path

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Review Scope

Single file added: test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs — 144 lines of unit tests for the internal LoggingManager.BuildAsync method.

Analysis

I reviewed the test code against the production logic in LoggingManager.BuildAsync and the supporting types (LoggerFactory, TryInitializeAsync, IExtension, IAsyncInitializableExtension).

Correctness ✅ All 8 tests accurately model the production branching logic:

• The disabled-provider guard (IExtension.IsEnabledAsync()) is correctly exercised by tests 5, 6, and 8, each with properly-configured IsEnabledAsync mock setup so no await null risk.
• The initialization path (IAsyncInitializableExtension.InitializeAsync() via TryInitializeAsync) is correctly targeted by tests 7 and 8.
• The Times.Once / Times.Never verifications are sound — they would catch bugs in both directions.

Mock setup ✅ All mocks that implement IExtension have IsEnabledAsync() explicitly set up, avoiding the await null footgun from Moq's loose default for Task-returning members.

Threading ✅ _mockMonitor.Lock(...) is correctly wired to return a disposable, satisfying the using block in LoggerFactory.CreateLogger.

IDisposable — ILoggerFactory instances are not disposed after use, but this is inconsequential since all providers are mock objects with no real resources; no improvement needed here.

Internal interfaces — The three internal marker interfaces (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a clean test-local pattern consistent with the existing IDisposableLoggerProvider in LoggerFactoryTests.cs.

Key Findings

No correctness, threading, performance, API compatibility, or security issues found.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

All review comments were addressed in the previous commit: mock naming standardized (mockXxx prefix), explicit discards added, Returns(Task.CompletedTask) used consistently (Moq 4.18.4 doesn't support ReturnsAsync() with no args), no-providers test strengthened with an actual CreateLogger assertion, and BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider added for the missing coverage path.

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. The add/add conflict in LoggingManagerTests.cs was resolved by taking main's structure (shorter test names, DataRow parameterization) and applying our improvements on top (explicit _ = discards, stronger no-providers assertion).

[Copilot]

Copilot resolve the merge conflicts in this pull request

Already resolved in the previous commit — the add/add conflict in LoggingManagerTests.cs was resolved by adopting main's structure with our improvements applied on top.

[Amaury Levé (Evangelink)]

Copilot fix 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.
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.

[Copilot]

Copilot fix issues azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Windows Debug)
...

Fixed in the latest commit. Both Mock<TestSourceHost> constructor calls that were passing a bare null for the run settings argument (lines 107 and 164 in the original, now 107 and 147 after minor line shifts) have been updated to use (IRunSettings?)null, which satisfies the nullable reference type annotation and resolves both CS8625 errors.

[Amaury Levé (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 LoggingManager tests from this PR landed in main via #8124 and #8130. This PR is a duplicate and can be safely closed.

Generated by Daily Test Improver · ● 9.5M · ◷

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in commit 0aad6c4. The two conflicted files (LoggingManagerTests.cs and DesktopTestSourceHostTests.cs) both took main's version, which includes the restructured tests (interfaces at namespace level, updated test names) and null! for the run settings arguments.

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider with null throws ArgumentNullException
• BuildAsync with no providers returns a functional empty factory (verifies both factory and returned logger are non-null)
• Factory delegate receives correct LogLevel and IServiceProvider (parameterized with multiple log levels)
• Non-IExtension provider is always included
• Enabled IExtension provider is included
• Disabled IExtension provider is excluded
• Non-extension IAsyncInitializableExtension provider has InitializeAsync called
• Enabled IExtension + IAsyncInitializableExtension provider is included and has InitializeAsync called
• Disabled IExtension + IAsyncInitializableExtension provider does not have InitializeAsync called

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

[Amaury Levé (Evangelink)]

Summary

Workflow: PR Nitpick Reviewer 🔍
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

Good addition of unit tests for LoggingManager.BuildAsync — the coverage is thorough, covering no providers, enabled/disabled extension providers, and initializable providers. A few minor consistency issues worth addressing:

1. Important — Mock naming inconsistency: Class-level fields use _mockXxx prefix while local test variables use xxxMock suffix. Picking one convention for the whole file improves readability.
2. Minor — Silent discard of CreateLogger return value (lines 72, 89, 104): Using _ = factory.CreateLogger(...) makes the intentional discard explicit.
3. Minor — Returns(Task.CompletedTask) vs ReturnsAsync() (line 115): All other async setups use ReturnsAsync(...); a small inconsistency worth aligning.

Positive Highlights

• Excellent use of helper test interfaces at the bottom of the file to combine ILoggerProvider, IExtension, and IAsyncInitializableExtension — clean and reusable.
• Test method names are descriptive and follow the Method_Condition_ExpectedResult pattern consistently.
• Good use of Times.Once / Times.Never for precise mock verification.

---

Generated by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Copilot]

Pull request overview

Adds unit tests for Microsoft.Testing.Platform.Logging.LoggingManager.BuildAsync, which builds the platform ILoggerFactory by evaluating provider factories, filtering disabled extensions, and initializing async-initializable providers.

Changes:

• Added a new LoggingManagerTests suite covering null guards, provider inclusion/exclusion, and initialization behavior.
• Introduced lightweight internal marker interfaces in the test file to mock combinations of ILoggerProvider + IExtension + IAsyncInitializableExtension.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	Adds new unit tests targeting LoggingManager.BuildAsync behaviors around provider creation, extension enablement filtering, and async initialization.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

1. [Assertion] Near-tautological assertion in BuildAsync_WithNoProviders_ReturnsEmptyLoggerFactory (line 36): Assert.IsNotNull(factory) can never fail since BuildAsync always constructs a new LoggerFactory. The test doesn't verify the "empty" behavior its name implies.

2. [Coverage] Missing test for enabled IExtensionInitializableLoggerProvider (line 126): The disabled case is tested, but there's no test for when IsEnabledAsync() returns true on a provider that implements both IExtension and IAsyncInitializableExtension. The production code has a distinct path where such a provider must be included AND have InitializeAsync() called — currently uncovered.

What's Well Done

• Good isolation: fresh mock instances per test class instance, no static mutable state
• The factory-delegate capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and effective
• Correct use of Times.Never to verify exclusion paths
• Helper interfaces at the bottom of the file (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a good pattern for multi-interface mocking without polluting production code

Recommendations

1. Strengthen the no-providers test to actually exercise the factory (e.g., call factory.CreateLogger("test") and verify a logger is returned without throwing)
2. Add BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider to cover the enabled + initializable extension path

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Review Scope

Single file added: test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs — 144 lines of unit tests for the internal LoggingManager.BuildAsync method.

Analysis

I reviewed the test code against the production logic in LoggingManager.BuildAsync and the supporting types (LoggerFactory, TryInitializeAsync, IExtension, IAsyncInitializableExtension).

Correctness ✅ All 8 tests accurately model the production branching logic:

• The disabled-provider guard (IExtension.IsEnabledAsync()) is correctly exercised by tests 5, 6, and 8, each with properly-configured IsEnabledAsync mock setup so no await null risk.
• The initialization path (IAsyncInitializableExtension.InitializeAsync() via TryInitializeAsync) is correctly targeted by tests 7 and 8.
• The Times.Once / Times.Never verifications are sound — they would catch bugs in both directions.

Mock setup ✅ All mocks that implement IExtension have IsEnabledAsync() explicitly set up, avoiding the await null footgun from Moq's loose default for Task-returning members.

Threading ✅ _mockMonitor.Lock(...) is correctly wired to return a disposable, satisfying the using block in LoggerFactory.CreateLogger.

IDisposable — ILoggerFactory instances are not disposed after use, but this is inconsequential since all providers are mock objects with no real resources; no improvement needed here.

Internal interfaces — The three internal marker interfaces (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a clean test-local pattern consistent with the existing IDisposableLoggerProvider in LoggerFactoryTests.cs.

Key Findings

No correctness, threading, performance, API compatibility, or security issues found.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

All review comments were addressed in the previous commit: mock naming standardized (mockXxx prefix), explicit discards added, Returns(Task.CompletedTask) used consistently (Moq 4.18.4 doesn't support ReturnsAsync() with no args), no-providers test strengthened with an actual CreateLogger assertion, and BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider added for the missing coverage path.

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. The add/add conflict in LoggingManagerTests.cs was resolved by taking main's structure (shorter test names, DataRow parameterization) and applying our improvements on top (explicit _ = discards, stronger no-providers assertion).

[Copilot]

Copilot resolve the merge conflicts in this pull request

Already resolved in the previous commit — the add/add conflict in LoggingManagerTests.cs was resolved by adopting main's structure with our improvements applied on top.

[Amaury Levé (Evangelink)]

Copilot fix 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.
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.

[Copilot]

Copilot fix issues azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Windows Debug)
...

Fixed in the latest commit. Both Mock<TestSourceHost> constructor calls that were passing a bare null for the run settings argument (lines 107 and 164 in the original, now 107 and 147 after minor line shifts) have been updated to use (IRunSettings?)null, which satisfies the nullable reference type annotation and resolves both CS8625 errors.

[Amaury Levé (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 LoggingManager tests from this PR landed in main via #8124 and #8130. This PR is a duplicate and can be safely closed.

Generated by Daily Test Improver · ● 9.5M · ◷

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in commit 0aad6c4. The two conflicted files (LoggingManagerTests.cs and DesktopTestSourceHostTests.cs) both took main's version, which includes the restructured tests (interfaces at namespace level, updated test names) and null! for the run settings arguments.

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider with null throws ArgumentNullException
• BuildAsync with no providers returns a functional empty factory (verifies both factory and returned logger are non-null)
• Factory delegate receives correct LogLevel and IServiceProvider (parameterized with multiple log levels)
• Non-IExtension provider is always included
• Enabled IExtension provider is included
• Disabled IExtension provider is excluded
• Non-extension IAsyncInitializableExtension provider has InitializeAsync called
• Enabled IExtension + IAsyncInitializableExtension provider is included and has InitializeAsync called
• Disabled IExtension + IAsyncInitializableExtension provider does not have InitializeAsync called

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

[Amaury Levé (Evangelink)]

Summary

Workflow: PR Nitpick Reviewer 🔍
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

Good addition of unit tests for LoggingManager.BuildAsync — the coverage is thorough, covering no providers, enabled/disabled extension providers, and initializable providers. A few minor consistency issues worth addressing:

1. Important — Mock naming inconsistency: Class-level fields use _mockXxx prefix while local test variables use xxxMock suffix. Picking one convention for the whole file improves readability.
2. Minor — Silent discard of CreateLogger return value (lines 72, 89, 104): Using _ = factory.CreateLogger(...) makes the intentional discard explicit.
3. Minor — Returns(Task.CompletedTask) vs ReturnsAsync() (line 115): All other async setups use ReturnsAsync(...); a small inconsistency worth aligning.

Positive Highlights

• Excellent use of helper test interfaces at the bottom of the file to combine ILoggerProvider, IExtension, and IAsyncInitializableExtension — clean and reusable.
• Test method names are descriptive and follow the Method_Condition_ExpectedResult pattern consistently.
• Good use of Times.Once / Times.Never for precise mock verification.

---

Generated by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Copilot]

Pull request overview

Adds unit tests for Microsoft.Testing.Platform.Logging.LoggingManager.BuildAsync, which builds the platform ILoggerFactory by evaluating provider factories, filtering disabled extensions, and initializing async-initializable providers.

Changes:

• Added a new LoggingManagerTests suite covering null guards, provider inclusion/exclusion, and initialization behavior.
• Introduced lightweight internal marker interfaces in the test file to mock combinations of ILoggerProvider + IExtension + IAsyncInitializableExtension.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	Adds new unit tests targeting LoggingManager.BuildAsync behaviors around provider creation, extension enablement filtering, and async initialization.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

1. [Assertion] Near-tautological assertion in BuildAsync_WithNoProviders_ReturnsEmptyLoggerFactory (line 36): Assert.IsNotNull(factory) can never fail since BuildAsync always constructs a new LoggerFactory. The test doesn't verify the "empty" behavior its name implies.

2. [Coverage] Missing test for enabled IExtensionInitializableLoggerProvider (line 126): The disabled case is tested, but there's no test for when IsEnabledAsync() returns true on a provider that implements both IExtension and IAsyncInitializableExtension. The production code has a distinct path where such a provider must be included AND have InitializeAsync() called — currently uncovered.

What's Well Done

• Good isolation: fresh mock instances per test class instance, no static mutable state
• The factory-delegate capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and effective
• Correct use of Times.Never to verify exclusion paths
• Helper interfaces at the bottom of the file (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a good pattern for multi-interface mocking without polluting production code

Recommendations

1. Strengthen the no-providers test to actually exercise the factory (e.g., call factory.CreateLogger("test") and verify a logger is returned without throwing)
2. Add BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider to cover the enabled + initializable extension path

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Review Scope

Single file added: test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs — 144 lines of unit tests for the internal LoggingManager.BuildAsync method.

Analysis

I reviewed the test code against the production logic in LoggingManager.BuildAsync and the supporting types (LoggerFactory, TryInitializeAsync, IExtension, IAsyncInitializableExtension).

Correctness ✅ All 8 tests accurately model the production branching logic:

• The disabled-provider guard (IExtension.IsEnabledAsync()) is correctly exercised by tests 5, 6, and 8, each with properly-configured IsEnabledAsync mock setup so no await null risk.
• The initialization path (IAsyncInitializableExtension.InitializeAsync() via TryInitializeAsync) is correctly targeted by tests 7 and 8.
• The Times.Once / Times.Never verifications are sound — they would catch bugs in both directions.

Mock setup ✅ All mocks that implement IExtension have IsEnabledAsync() explicitly set up, avoiding the await null footgun from Moq's loose default for Task-returning members.

Threading ✅ _mockMonitor.Lock(...) is correctly wired to return a disposable, satisfying the using block in LoggerFactory.CreateLogger.

IDisposable — ILoggerFactory instances are not disposed after use, but this is inconsequential since all providers are mock objects with no real resources; no improvement needed here.

Internal interfaces — The three internal marker interfaces (IExtensionLoggerProvider, IInitializableLoggerProvider, IExtensionInitializableLoggerProvider) are a clean test-local pattern consistent with the existing IDisposableLoggerProvider in LoggerFactoryTests.cs.

Key Findings

No correctness, threading, performance, API compatibility, or security issues found.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

All review comments were addressed in the previous commit: mock naming standardized (mockXxx prefix), explicit discards added, Returns(Task.CompletedTask) used consistently (Moq 4.18.4 doesn't support ReturnsAsync() with no args), no-providers test strengthened with an actual CreateLogger assertion, and BuildAsync_WithEnabledInitializableExtensionProvider_IncludesAndInitializesProvider added for the missing coverage path.

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. The add/add conflict in LoggingManagerTests.cs was resolved by taking main's structure (shorter test names, DataRow parameterization) and applying our improvements on top (explicit _ = discards, stronger no-providers assertion).

[Copilot]

Copilot resolve the merge conflicts in this pull request

Already resolved in the previous commit — the add/add conflict in LoggingManagerTests.cs was resolved by adopting main's structure with our improvements applied on top.

[Amaury Levé (Evangelink)]

Copilot fix 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.
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.

[Copilot]

Copilot fix issues azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Windows Debug)
...

Fixed in the latest commit. Both Mock<TestSourceHost> constructor calls that were passing a bare null for the run settings argument (lines 107 and 164 in the original, now 107 and 147 after minor line shifts) have been updated to use (IRunSettings?)null, which satisfies the nullable reference type annotation and resolves both CS8625 errors.

[Amaury Levé (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 LoggingManager tests from this PR landed in main via #8124 and #8130. This PR is a duplicate and can be safely closed.

Generated by Daily Test Improver · ● 9.5M · ◷

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in commit 0aad6c4. The two conflicted files (LoggingManagerTests.cs and DesktopTestSourceHostTests.cs) both took main's version, which includes the restructured tests (interfaces at namespace level, updated test names) and null! for the run settings arguments.