[Test Improver] Add unit tests for LoggingManager.BuildAsync

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider(null) throws ArgumentNullException
• BuildAsync with no providers returns a non-null factory
• Non-extension provider is always included
• IExtension provider with IsEnabledAsync()=true is included
• IExtension provider with IsEnabledAsync()=false is excluded (not added to factory)
• Non-extension IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Enabled IExtension + IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Disabled IExtension + IAsyncInitializableExtension provider does NOT have InitializeAsync() called
• Factory is invoked with the correct LogLevel and IServiceProvider (parameterized across multiple log levels)

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

[Copilot]

Pull request overview

Adds a focused unit-test suite for LoggingManager.BuildAsync (Microsoft.Testing.Platform logging pipeline) to validate provider registration, extension enablement filtering, async initialization behavior, and factory delegate inputs.

Changes:

• Added unit tests validating AddProvider(null) throws and that providers are included/excluded based on IExtension.IsEnabledAsync().
• Added unit tests validating IAsyncInitializableExtension.InitializeAsync() is invoked only for included providers.
• Added a unit test validating the provider factory receives the expected LogLevel and IServiceProvider.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	New unit tests covering LoggingManager.BuildAsync provider filtering, initialization, and factory argument passing.

Copilot's findings

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

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

The new LoggingManagerTests are well-structured and cover the core scenarios cleanly. Three minor improvements stand out:

1. Missing test (Important) — The enabled IExtension + IAsyncInitializableExtension combination isn't tested. The disabled path is covered, but the enabled path (which should call InitializeAsync()) has no corresponding test.

2. Incomplete assertion (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync verifies InitializeAsync() was invoked but doesn't verify the provider was included in the resulting factory. The other "include" tests check both; this one should too.

3. Ambiguous test name (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync doesn't communicate that the provider intentionally omits IExtension. This distinction is meaningful given the two code paths under test.

Positive Highlights

• Consistent test structure and naming convention across all methods.
• Good use of interface composition to isolate specific behavioural combinations (extension-only, initializable-only, both).
• Internal test interfaces correctly follow the semicolon shorthand style (consistent with IDisposableLoggerProvider in LoggerFactoryTests.cs).
• _mockMonitor setup in the constructor mirrors the pattern in LoggerFactoryTests.

---

Generated by PR Nitpick Reviewer 🔍

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Analysis

This PR adds a single new test file (LoggingManagerTests.cs) with 7 test methods covering LoggingManager.BuildAsync. I reviewed the tests against the production implementation in LoggingManager.cs and LoggerFactory.cs.

Verified Correctness

• Null guard test: Correctly verifies that AddProvider(null!) throws ArgumentNullException — matches the production ?? throw new ArgumentNullException(...) guard.
• Provider inclusion/exclusion: Tests for enabled/disabled IExtension providers correctly map to the if (serviceInstance is IExtension extension && !await extension.IsEnabledAsync()) branch in production.
• Initialization ordering: BuildAsync_WithInitializableProvider_CallsInitializeAsync correctly verifies TryInitializeAsync calls InitializeAsync when the provider implements IAsyncInitializableExtension. The separate BuildAsync_WithDisabledExtensionProvider_DoesNotCallInitializeAsync correctly confirms the continue statement prevents initialization of disabled extensions.
• Factory parameter forwarding: The capture-variable approach in BuildAsync_PassesLogLevelAndServiceProviderToFactory correctly exercises the factory(logLevel, serviceProvider) call site.
• IMonitor mock setup: The _mockMonitor.Setup(m => m.Lock(...)).Returns(...) correctly satisfies LoggerFactory.CreateLogger's using (_monitor.Lock(_sync)) pattern.

Threading & Concurrency

Each MSTest test method gets a fresh LoggingManagerTests instance, so the readonly Mock<> fields are not shared between tests. No threading concerns.

API Compliance

All assertions use MSTest APIs (Assert.ThrowsExactly, Assert.AreEqual, Assert.AreSame) and Moq Verify() — compliant with the project's BannedSymbols.txt which bans AwesomeAssertions.

Key Findings

No issues found in any review category (correctness, threading, performance, API compat, cross-TFM, resources, security, defensive coding).

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

1. [Coverage] Missing test: enabled extension + initializable provider — The production code has a path where a provider implements both IExtension (enabled) and IAsyncInitializableExtension, causing InitializeAsync() to be called after the IsEnabledAsync()=true check. This path is not exercised. BuildAsync_WithInitializableProvider_CallsInitializeAsync (line 76) does not cover it because IInitializableLoggerProvider doesn't implement IExtension, so the is IExtension check is false and it takes a completely different branch. IExtensionAndInitializableLoggerProvider (line 131) exists but is only used in the disabled case.

What's Good

• Isolation: Each test constructs a fresh LoggingManager; instance-level mocks are recreated per-test by MSTest's per-method instantiation — no shared mutable state.
• Assertions: Moq Verify with Times.Once / Times.Never gives precise failure messages.
• Factory capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and correctly verifies the parameters forwarded to the factory lambda.
• Test interfaces declared at the bottom of the file are a neat way to compose the exact capability mix each test needs without boilerplate classes.

Recommendations

Add one test for the enabled+initializable extension case as described in the inline comment — the IExtensionAndInitializableLoggerProvider interface is already available at the bottom of the file.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Copilot address review comment

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main into the branch. The conflict in LoggingManagerTests.cs was resolved by adopting the main branch's version (which was the authoritative merged version from #8124).

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider(null) throws ArgumentNullException
• BuildAsync with no providers returns a non-null factory
• Non-extension provider is always included
• IExtension provider with IsEnabledAsync()=true is included
• IExtension provider with IsEnabledAsync()=false is excluded (not added to factory)
• Non-extension IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Enabled IExtension + IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Disabled IExtension + IAsyncInitializableExtension provider does NOT have InitializeAsync() called
• Factory is invoked with the correct LogLevel and IServiceProvider (parameterized across multiple log levels)

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

[Copilot]

Pull request overview

Adds a focused unit-test suite for LoggingManager.BuildAsync (Microsoft.Testing.Platform logging pipeline) to validate provider registration, extension enablement filtering, async initialization behavior, and factory delegate inputs.

Changes:

• Added unit tests validating AddProvider(null) throws and that providers are included/excluded based on IExtension.IsEnabledAsync().
• Added unit tests validating IAsyncInitializableExtension.InitializeAsync() is invoked only for included providers.
• Added a unit test validating the provider factory receives the expected LogLevel and IServiceProvider.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	New unit tests covering LoggingManager.BuildAsync provider filtering, initialization, and factory argument passing.

Copilot's findings

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

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

The new LoggingManagerTests are well-structured and cover the core scenarios cleanly. Three minor improvements stand out:

1. Missing test (Important) — The enabled IExtension + IAsyncInitializableExtension combination isn't tested. The disabled path is covered, but the enabled path (which should call InitializeAsync()) has no corresponding test.

2. Incomplete assertion (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync verifies InitializeAsync() was invoked but doesn't verify the provider was included in the resulting factory. The other "include" tests check both; this one should too.

3. Ambiguous test name (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync doesn't communicate that the provider intentionally omits IExtension. This distinction is meaningful given the two code paths under test.

Positive Highlights

• Consistent test structure and naming convention across all methods.
• Good use of interface composition to isolate specific behavioural combinations (extension-only, initializable-only, both).
• Internal test interfaces correctly follow the semicolon shorthand style (consistent with IDisposableLoggerProvider in LoggerFactoryTests.cs).
• _mockMonitor setup in the constructor mirrors the pattern in LoggerFactoryTests.

---

Generated by PR Nitpick Reviewer 🔍

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Analysis

This PR adds a single new test file (LoggingManagerTests.cs) with 7 test methods covering LoggingManager.BuildAsync. I reviewed the tests against the production implementation in LoggingManager.cs and LoggerFactory.cs.

Verified Correctness

• Null guard test: Correctly verifies that AddProvider(null!) throws ArgumentNullException — matches the production ?? throw new ArgumentNullException(...) guard.
• Provider inclusion/exclusion: Tests for enabled/disabled IExtension providers correctly map to the if (serviceInstance is IExtension extension && !await extension.IsEnabledAsync()) branch in production.
• Initialization ordering: BuildAsync_WithInitializableProvider_CallsInitializeAsync correctly verifies TryInitializeAsync calls InitializeAsync when the provider implements IAsyncInitializableExtension. The separate BuildAsync_WithDisabledExtensionProvider_DoesNotCallInitializeAsync correctly confirms the continue statement prevents initialization of disabled extensions.
• Factory parameter forwarding: The capture-variable approach in BuildAsync_PassesLogLevelAndServiceProviderToFactory correctly exercises the factory(logLevel, serviceProvider) call site.
• IMonitor mock setup: The _mockMonitor.Setup(m => m.Lock(...)).Returns(...) correctly satisfies LoggerFactory.CreateLogger's using (_monitor.Lock(_sync)) pattern.

Threading & Concurrency

Each MSTest test method gets a fresh LoggingManagerTests instance, so the readonly Mock<> fields are not shared between tests. No threading concerns.

API Compliance

All assertions use MSTest APIs (Assert.ThrowsExactly, Assert.AreEqual, Assert.AreSame) and Moq Verify() — compliant with the project's BannedSymbols.txt which bans AwesomeAssertions.

Key Findings

No issues found in any review category (correctness, threading, performance, API compat, cross-TFM, resources, security, defensive coding).

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

1. [Coverage] Missing test: enabled extension + initializable provider — The production code has a path where a provider implements both IExtension (enabled) and IAsyncInitializableExtension, causing InitializeAsync() to be called after the IsEnabledAsync()=true check. This path is not exercised. BuildAsync_WithInitializableProvider_CallsInitializeAsync (line 76) does not cover it because IInitializableLoggerProvider doesn't implement IExtension, so the is IExtension check is false and it takes a completely different branch. IExtensionAndInitializableLoggerProvider (line 131) exists but is only used in the disabled case.

What's Good

• Isolation: Each test constructs a fresh LoggingManager; instance-level mocks are recreated per-test by MSTest's per-method instantiation — no shared mutable state.
• Assertions: Moq Verify with Times.Once / Times.Never gives precise failure messages.
• Factory capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and correctly verifies the parameters forwarded to the factory lambda.
• Test interfaces declared at the bottom of the file are a neat way to compose the exact capability mix each test needs without boilerplate classes.

Recommendations

Add one test for the enabled+initializable extension case as described in the inline comment — the IExtensionAndInitializableLoggerProvider interface is already available at the bottom of the file.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Copilot address review comment

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main into the branch. The conflict in LoggingManagerTests.cs was resolved by adopting the main branch's version (which was the authoritative merged version from #8124).

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider(null) throws ArgumentNullException
• BuildAsync with no providers returns a non-null factory
• Non-extension provider is always included
• IExtension provider with IsEnabledAsync()=true is included
• IExtension provider with IsEnabledAsync()=false is excluded (not added to factory)
• Non-extension IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Enabled IExtension + IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Disabled IExtension + IAsyncInitializableExtension provider does NOT have InitializeAsync() called
• Factory is invoked with the correct LogLevel and IServiceProvider (parameterized across multiple log levels)

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

[Copilot]

Pull request overview

Adds a focused unit-test suite for LoggingManager.BuildAsync (Microsoft.Testing.Platform logging pipeline) to validate provider registration, extension enablement filtering, async initialization behavior, and factory delegate inputs.

Changes:

• Added unit tests validating AddProvider(null) throws and that providers are included/excluded based on IExtension.IsEnabledAsync().
• Added unit tests validating IAsyncInitializableExtension.InitializeAsync() is invoked only for included providers.
• Added a unit test validating the provider factory receives the expected LogLevel and IServiceProvider.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	New unit tests covering LoggingManager.BuildAsync provider filtering, initialization, and factory argument passing.

Copilot's findings

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

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

The new LoggingManagerTests are well-structured and cover the core scenarios cleanly. Three minor improvements stand out:

1. Missing test (Important) — The enabled IExtension + IAsyncInitializableExtension combination isn't tested. The disabled path is covered, but the enabled path (which should call InitializeAsync()) has no corresponding test.

2. Incomplete assertion (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync verifies InitializeAsync() was invoked but doesn't verify the provider was included in the resulting factory. The other "include" tests check both; this one should too.

3. Ambiguous test name (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync doesn't communicate that the provider intentionally omits IExtension. This distinction is meaningful given the two code paths under test.

Positive Highlights

• Consistent test structure and naming convention across all methods.
• Good use of interface composition to isolate specific behavioural combinations (extension-only, initializable-only, both).
• Internal test interfaces correctly follow the semicolon shorthand style (consistent with IDisposableLoggerProvider in LoggerFactoryTests.cs).
• _mockMonitor setup in the constructor mirrors the pattern in LoggerFactoryTests.

---

Generated by PR Nitpick Reviewer 🔍

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Analysis

This PR adds a single new test file (LoggingManagerTests.cs) with 7 test methods covering LoggingManager.BuildAsync. I reviewed the tests against the production implementation in LoggingManager.cs and LoggerFactory.cs.

Verified Correctness

• Null guard test: Correctly verifies that AddProvider(null!) throws ArgumentNullException — matches the production ?? throw new ArgumentNullException(...) guard.
• Provider inclusion/exclusion: Tests for enabled/disabled IExtension providers correctly map to the if (serviceInstance is IExtension extension && !await extension.IsEnabledAsync()) branch in production.
• Initialization ordering: BuildAsync_WithInitializableProvider_CallsInitializeAsync correctly verifies TryInitializeAsync calls InitializeAsync when the provider implements IAsyncInitializableExtension. The separate BuildAsync_WithDisabledExtensionProvider_DoesNotCallInitializeAsync correctly confirms the continue statement prevents initialization of disabled extensions.
• Factory parameter forwarding: The capture-variable approach in BuildAsync_PassesLogLevelAndServiceProviderToFactory correctly exercises the factory(logLevel, serviceProvider) call site.
• IMonitor mock setup: The _mockMonitor.Setup(m => m.Lock(...)).Returns(...) correctly satisfies LoggerFactory.CreateLogger's using (_monitor.Lock(_sync)) pattern.

Threading & Concurrency

Each MSTest test method gets a fresh LoggingManagerTests instance, so the readonly Mock<> fields are not shared between tests. No threading concerns.

API Compliance

All assertions use MSTest APIs (Assert.ThrowsExactly, Assert.AreEqual, Assert.AreSame) and Moq Verify() — compliant with the project's BannedSymbols.txt which bans AwesomeAssertions.

Key Findings

No issues found in any review category (correctness, threading, performance, API compat, cross-TFM, resources, security, defensive coding).

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

1. [Coverage] Missing test: enabled extension + initializable provider — The production code has a path where a provider implements both IExtension (enabled) and IAsyncInitializableExtension, causing InitializeAsync() to be called after the IsEnabledAsync()=true check. This path is not exercised. BuildAsync_WithInitializableProvider_CallsInitializeAsync (line 76) does not cover it because IInitializableLoggerProvider doesn't implement IExtension, so the is IExtension check is false and it takes a completely different branch. IExtensionAndInitializableLoggerProvider (line 131) exists but is only used in the disabled case.

What's Good

• Isolation: Each test constructs a fresh LoggingManager; instance-level mocks are recreated per-test by MSTest's per-method instantiation — no shared mutable state.
• Assertions: Moq Verify with Times.Once / Times.Never gives precise failure messages.
• Factory capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and correctly verifies the parameters forwarded to the factory lambda.
• Test interfaces declared at the bottom of the file are a neat way to compose the exact capability mix each test needs without boilerplate classes.

Recommendations

Add one test for the enabled+initializable extension case as described in the inline comment — the IExtensionAndInitializableLoggerProvider interface is already available at the bottom of the file.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Copilot address review comment

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main into the branch. The conflict in LoggingManagerTests.cs was resolved by adopting the main branch's version (which was the authoritative merged version from #8124).

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider(null) throws ArgumentNullException
• BuildAsync with no providers returns a non-null factory
• Non-extension provider is always included
• IExtension provider with IsEnabledAsync()=true is included
• IExtension provider with IsEnabledAsync()=false is excluded (not added to factory)
• Non-extension IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Enabled IExtension + IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Disabled IExtension + IAsyncInitializableExtension provider does NOT have InitializeAsync() called
• Factory is invoked with the correct LogLevel and IServiceProvider (parameterized across multiple log levels)

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

[Copilot]

Pull request overview

Adds a focused unit-test suite for LoggingManager.BuildAsync (Microsoft.Testing.Platform logging pipeline) to validate provider registration, extension enablement filtering, async initialization behavior, and factory delegate inputs.

Changes:

• Added unit tests validating AddProvider(null) throws and that providers are included/excluded based on IExtension.IsEnabledAsync().
• Added unit tests validating IAsyncInitializableExtension.InitializeAsync() is invoked only for included providers.
• Added a unit test validating the provider factory receives the expected LogLevel and IServiceProvider.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	New unit tests covering LoggingManager.BuildAsync provider filtering, initialization, and factory argument passing.

Copilot's findings

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

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

The new LoggingManagerTests are well-structured and cover the core scenarios cleanly. Three minor improvements stand out:

1. Missing test (Important) — The enabled IExtension + IAsyncInitializableExtension combination isn't tested. The disabled path is covered, but the enabled path (which should call InitializeAsync()) has no corresponding test.

2. Incomplete assertion (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync verifies InitializeAsync() was invoked but doesn't verify the provider was included in the resulting factory. The other "include" tests check both; this one should too.

3. Ambiguous test name (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync doesn't communicate that the provider intentionally omits IExtension. This distinction is meaningful given the two code paths under test.

Positive Highlights

• Consistent test structure and naming convention across all methods.
• Good use of interface composition to isolate specific behavioural combinations (extension-only, initializable-only, both).
• Internal test interfaces correctly follow the semicolon shorthand style (consistent with IDisposableLoggerProvider in LoggerFactoryTests.cs).
• _mockMonitor setup in the constructor mirrors the pattern in LoggerFactoryTests.

---

Generated by PR Nitpick Reviewer 🔍

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Analysis

This PR adds a single new test file (LoggingManagerTests.cs) with 7 test methods covering LoggingManager.BuildAsync. I reviewed the tests against the production implementation in LoggingManager.cs and LoggerFactory.cs.

Verified Correctness

• Null guard test: Correctly verifies that AddProvider(null!) throws ArgumentNullException — matches the production ?? throw new ArgumentNullException(...) guard.
• Provider inclusion/exclusion: Tests for enabled/disabled IExtension providers correctly map to the if (serviceInstance is IExtension extension && !await extension.IsEnabledAsync()) branch in production.
• Initialization ordering: BuildAsync_WithInitializableProvider_CallsInitializeAsync correctly verifies TryInitializeAsync calls InitializeAsync when the provider implements IAsyncInitializableExtension. The separate BuildAsync_WithDisabledExtensionProvider_DoesNotCallInitializeAsync correctly confirms the continue statement prevents initialization of disabled extensions.
• Factory parameter forwarding: The capture-variable approach in BuildAsync_PassesLogLevelAndServiceProviderToFactory correctly exercises the factory(logLevel, serviceProvider) call site.
• IMonitor mock setup: The _mockMonitor.Setup(m => m.Lock(...)).Returns(...) correctly satisfies LoggerFactory.CreateLogger's using (_monitor.Lock(_sync)) pattern.

Threading & Concurrency

Each MSTest test method gets a fresh LoggingManagerTests instance, so the readonly Mock<> fields are not shared between tests. No threading concerns.

API Compliance

All assertions use MSTest APIs (Assert.ThrowsExactly, Assert.AreEqual, Assert.AreSame) and Moq Verify() — compliant with the project's BannedSymbols.txt which bans AwesomeAssertions.

Key Findings

No issues found in any review category (correctness, threading, performance, API compat, cross-TFM, resources, security, defensive coding).

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

1. [Coverage] Missing test: enabled extension + initializable provider — The production code has a path where a provider implements both IExtension (enabled) and IAsyncInitializableExtension, causing InitializeAsync() to be called after the IsEnabledAsync()=true check. This path is not exercised. BuildAsync_WithInitializableProvider_CallsInitializeAsync (line 76) does not cover it because IInitializableLoggerProvider doesn't implement IExtension, so the is IExtension check is false and it takes a completely different branch. IExtensionAndInitializableLoggerProvider (line 131) exists but is only used in the disabled case.

What's Good

• Isolation: Each test constructs a fresh LoggingManager; instance-level mocks are recreated per-test by MSTest's per-method instantiation — no shared mutable state.
• Assertions: Moq Verify with Times.Once / Times.Never gives precise failure messages.
• Factory capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and correctly verifies the parameters forwarded to the factory lambda.
• Test interfaces declared at the bottom of the file are a neat way to compose the exact capability mix each test needs without boilerplate classes.

Recommendations

Add one test for the enabled+initializable extension case as described in the inline comment — the IExtensionAndInitializableLoggerProvider interface is already available at the bottom of the file.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Copilot address review comment

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main into the branch. The conflict in LoggingManagerTests.cs was resolved by adopting the main branch's version (which was the authoritative merged version from #8124).

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider(null) throws ArgumentNullException
• BuildAsync with no providers returns a non-null factory
• Non-extension provider is always included
• IExtension provider with IsEnabledAsync()=true is included
• IExtension provider with IsEnabledAsync()=false is excluded (not added to factory)
• Non-extension IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Enabled IExtension + IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Disabled IExtension + IAsyncInitializableExtension provider does NOT have InitializeAsync() called
• Factory is invoked with the correct LogLevel and IServiceProvider (parameterized across multiple log levels)

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

[Copilot]

Pull request overview

Adds a focused unit-test suite for LoggingManager.BuildAsync (Microsoft.Testing.Platform logging pipeline) to validate provider registration, extension enablement filtering, async initialization behavior, and factory delegate inputs.

Changes:

• Added unit tests validating AddProvider(null) throws and that providers are included/excluded based on IExtension.IsEnabledAsync().
• Added unit tests validating IAsyncInitializableExtension.InitializeAsync() is invoked only for included providers.
• Added a unit test validating the provider factory receives the expected LogLevel and IServiceProvider.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	New unit tests covering LoggingManager.BuildAsync provider filtering, initialization, and factory argument passing.

Copilot's findings

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

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

The new LoggingManagerTests are well-structured and cover the core scenarios cleanly. Three minor improvements stand out:

1. Missing test (Important) — The enabled IExtension + IAsyncInitializableExtension combination isn't tested. The disabled path is covered, but the enabled path (which should call InitializeAsync()) has no corresponding test.

2. Incomplete assertion (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync verifies InitializeAsync() was invoked but doesn't verify the provider was included in the resulting factory. The other "include" tests check both; this one should too.

3. Ambiguous test name (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync doesn't communicate that the provider intentionally omits IExtension. This distinction is meaningful given the two code paths under test.

Positive Highlights

• Consistent test structure and naming convention across all methods.
• Good use of interface composition to isolate specific behavioural combinations (extension-only, initializable-only, both).
• Internal test interfaces correctly follow the semicolon shorthand style (consistent with IDisposableLoggerProvider in LoggerFactoryTests.cs).
• _mockMonitor setup in the constructor mirrors the pattern in LoggerFactoryTests.

---

Generated by PR Nitpick Reviewer 🔍

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Analysis

This PR adds a single new test file (LoggingManagerTests.cs) with 7 test methods covering LoggingManager.BuildAsync. I reviewed the tests against the production implementation in LoggingManager.cs and LoggerFactory.cs.

Verified Correctness

• Null guard test: Correctly verifies that AddProvider(null!) throws ArgumentNullException — matches the production ?? throw new ArgumentNullException(...) guard.
• Provider inclusion/exclusion: Tests for enabled/disabled IExtension providers correctly map to the if (serviceInstance is IExtension extension && !await extension.IsEnabledAsync()) branch in production.
• Initialization ordering: BuildAsync_WithInitializableProvider_CallsInitializeAsync correctly verifies TryInitializeAsync calls InitializeAsync when the provider implements IAsyncInitializableExtension. The separate BuildAsync_WithDisabledExtensionProvider_DoesNotCallInitializeAsync correctly confirms the continue statement prevents initialization of disabled extensions.
• Factory parameter forwarding: The capture-variable approach in BuildAsync_PassesLogLevelAndServiceProviderToFactory correctly exercises the factory(logLevel, serviceProvider) call site.
• IMonitor mock setup: The _mockMonitor.Setup(m => m.Lock(...)).Returns(...) correctly satisfies LoggerFactory.CreateLogger's using (_monitor.Lock(_sync)) pattern.

Threading & Concurrency

Each MSTest test method gets a fresh LoggingManagerTests instance, so the readonly Mock<> fields are not shared between tests. No threading concerns.

API Compliance

All assertions use MSTest APIs (Assert.ThrowsExactly, Assert.AreEqual, Assert.AreSame) and Moq Verify() — compliant with the project's BannedSymbols.txt which bans AwesomeAssertions.

Key Findings

No issues found in any review category (correctness, threading, performance, API compat, cross-TFM, resources, security, defensive coding).

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

1. [Coverage] Missing test: enabled extension + initializable provider — The production code has a path where a provider implements both IExtension (enabled) and IAsyncInitializableExtension, causing InitializeAsync() to be called after the IsEnabledAsync()=true check. This path is not exercised. BuildAsync_WithInitializableProvider_CallsInitializeAsync (line 76) does not cover it because IInitializableLoggerProvider doesn't implement IExtension, so the is IExtension check is false and it takes a completely different branch. IExtensionAndInitializableLoggerProvider (line 131) exists but is only used in the disabled case.

What's Good

• Isolation: Each test constructs a fresh LoggingManager; instance-level mocks are recreated per-test by MSTest's per-method instantiation — no shared mutable state.
• Assertions: Moq Verify with Times.Once / Times.Never gives precise failure messages.
• Factory capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and correctly verifies the parameters forwarded to the factory lambda.
• Test interfaces declared at the bottom of the file are a neat way to compose the exact capability mix each test needs without boilerplate classes.

Recommendations

Add one test for the enabled+initializable extension case as described in the inline comment — the IExtensionAndInitializableLoggerProvider interface is already available at the bottom of the file.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Copilot address review comment

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main into the branch. The conflict in LoggingManagerTests.cs was resolved by adopting the main branch's version (which was the authoritative merged version from #8124).

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider(null) throws ArgumentNullException
• BuildAsync with no providers returns a non-null factory
• Non-extension provider is always included
• IExtension provider with IsEnabledAsync()=true is included
• IExtension provider with IsEnabledAsync()=false is excluded (not added to factory)
• Non-extension IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Enabled IExtension + IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Disabled IExtension + IAsyncInitializableExtension provider does NOT have InitializeAsync() called
• Factory is invoked with the correct LogLevel and IServiceProvider (parameterized across multiple log levels)

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

[Copilot]

Pull request overview

Adds a focused unit-test suite for LoggingManager.BuildAsync (Microsoft.Testing.Platform logging pipeline) to validate provider registration, extension enablement filtering, async initialization behavior, and factory delegate inputs.

Changes:

• Added unit tests validating AddProvider(null) throws and that providers are included/excluded based on IExtension.IsEnabledAsync().
• Added unit tests validating IAsyncInitializableExtension.InitializeAsync() is invoked only for included providers.
• Added a unit test validating the provider factory receives the expected LogLevel and IServiceProvider.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	New unit tests covering LoggingManager.BuildAsync provider filtering, initialization, and factory argument passing.

Copilot's findings

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

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

The new LoggingManagerTests are well-structured and cover the core scenarios cleanly. Three minor improvements stand out:

1. Missing test (Important) — The enabled IExtension + IAsyncInitializableExtension combination isn't tested. The disabled path is covered, but the enabled path (which should call InitializeAsync()) has no corresponding test.

2. Incomplete assertion (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync verifies InitializeAsync() was invoked but doesn't verify the provider was included in the resulting factory. The other "include" tests check both; this one should too.

3. Ambiguous test name (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync doesn't communicate that the provider intentionally omits IExtension. This distinction is meaningful given the two code paths under test.

Positive Highlights

• Consistent test structure and naming convention across all methods.
• Good use of interface composition to isolate specific behavioural combinations (extension-only, initializable-only, both).
• Internal test interfaces correctly follow the semicolon shorthand style (consistent with IDisposableLoggerProvider in LoggerFactoryTests.cs).
• _mockMonitor setup in the constructor mirrors the pattern in LoggerFactoryTests.

---

Generated by PR Nitpick Reviewer 🔍

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Analysis

This PR adds a single new test file (LoggingManagerTests.cs) with 7 test methods covering LoggingManager.BuildAsync. I reviewed the tests against the production implementation in LoggingManager.cs and LoggerFactory.cs.

Verified Correctness

• Null guard test: Correctly verifies that AddProvider(null!) throws ArgumentNullException — matches the production ?? throw new ArgumentNullException(...) guard.
• Provider inclusion/exclusion: Tests for enabled/disabled IExtension providers correctly map to the if (serviceInstance is IExtension extension && !await extension.IsEnabledAsync()) branch in production.
• Initialization ordering: BuildAsync_WithInitializableProvider_CallsInitializeAsync correctly verifies TryInitializeAsync calls InitializeAsync when the provider implements IAsyncInitializableExtension. The separate BuildAsync_WithDisabledExtensionProvider_DoesNotCallInitializeAsync correctly confirms the continue statement prevents initialization of disabled extensions.
• Factory parameter forwarding: The capture-variable approach in BuildAsync_PassesLogLevelAndServiceProviderToFactory correctly exercises the factory(logLevel, serviceProvider) call site.
• IMonitor mock setup: The _mockMonitor.Setup(m => m.Lock(...)).Returns(...) correctly satisfies LoggerFactory.CreateLogger's using (_monitor.Lock(_sync)) pattern.

Threading & Concurrency

Each MSTest test method gets a fresh LoggingManagerTests instance, so the readonly Mock<> fields are not shared between tests. No threading concerns.

API Compliance

All assertions use MSTest APIs (Assert.ThrowsExactly, Assert.AreEqual, Assert.AreSame) and Moq Verify() — compliant with the project's BannedSymbols.txt which bans AwesomeAssertions.

Key Findings

No issues found in any review category (correctness, threading, performance, API compat, cross-TFM, resources, security, defensive coding).

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

1. [Coverage] Missing test: enabled extension + initializable provider — The production code has a path where a provider implements both IExtension (enabled) and IAsyncInitializableExtension, causing InitializeAsync() to be called after the IsEnabledAsync()=true check. This path is not exercised. BuildAsync_WithInitializableProvider_CallsInitializeAsync (line 76) does not cover it because IInitializableLoggerProvider doesn't implement IExtension, so the is IExtension check is false and it takes a completely different branch. IExtensionAndInitializableLoggerProvider (line 131) exists but is only used in the disabled case.

What's Good

• Isolation: Each test constructs a fresh LoggingManager; instance-level mocks are recreated per-test by MSTest's per-method instantiation — no shared mutable state.
• Assertions: Moq Verify with Times.Once / Times.Never gives precise failure messages.
• Factory capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and correctly verifies the parameters forwarded to the factory lambda.
• Test interfaces declared at the bottom of the file are a neat way to compose the exact capability mix each test needs without boilerplate classes.

Recommendations

Add one test for the enabled+initializable extension case as described in the inline comment — the IExtensionAndInitializableLoggerProvider interface is already available at the bottom of the file.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Copilot address review comment

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main into the branch. The conflict in LoggingManagerTests.cs was resolved by adopting the main branch's version (which was the authoritative merged version from #8124).

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider(null) throws ArgumentNullException
• BuildAsync with no providers returns a non-null factory
• Non-extension provider is always included
• IExtension provider with IsEnabledAsync()=true is included
• IExtension provider with IsEnabledAsync()=false is excluded (not added to factory)
• Non-extension IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Enabled IExtension + IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Disabled IExtension + IAsyncInitializableExtension provider does NOT have InitializeAsync() called
• Factory is invoked with the correct LogLevel and IServiceProvider (parameterized across multiple log levels)

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

[Copilot]

Pull request overview

Adds a focused unit-test suite for LoggingManager.BuildAsync (Microsoft.Testing.Platform logging pipeline) to validate provider registration, extension enablement filtering, async initialization behavior, and factory delegate inputs.

Changes:

• Added unit tests validating AddProvider(null) throws and that providers are included/excluded based on IExtension.IsEnabledAsync().
• Added unit tests validating IAsyncInitializableExtension.InitializeAsync() is invoked only for included providers.
• Added a unit test validating the provider factory receives the expected LogLevel and IServiceProvider.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	New unit tests covering LoggingManager.BuildAsync provider filtering, initialization, and factory argument passing.

Copilot's findings

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

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

The new LoggingManagerTests are well-structured and cover the core scenarios cleanly. Three minor improvements stand out:

1. Missing test (Important) — The enabled IExtension + IAsyncInitializableExtension combination isn't tested. The disabled path is covered, but the enabled path (which should call InitializeAsync()) has no corresponding test.

2. Incomplete assertion (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync verifies InitializeAsync() was invoked but doesn't verify the provider was included in the resulting factory. The other "include" tests check both; this one should too.

3. Ambiguous test name (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync doesn't communicate that the provider intentionally omits IExtension. This distinction is meaningful given the two code paths under test.

Positive Highlights

• Consistent test structure and naming convention across all methods.
• Good use of interface composition to isolate specific behavioural combinations (extension-only, initializable-only, both).
• Internal test interfaces correctly follow the semicolon shorthand style (consistent with IDisposableLoggerProvider in LoggerFactoryTests.cs).
• _mockMonitor setup in the constructor mirrors the pattern in LoggerFactoryTests.

---

Generated by PR Nitpick Reviewer 🔍

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Analysis

This PR adds a single new test file (LoggingManagerTests.cs) with 7 test methods covering LoggingManager.BuildAsync. I reviewed the tests against the production implementation in LoggingManager.cs and LoggerFactory.cs.

Verified Correctness

• Null guard test: Correctly verifies that AddProvider(null!) throws ArgumentNullException — matches the production ?? throw new ArgumentNullException(...) guard.
• Provider inclusion/exclusion: Tests for enabled/disabled IExtension providers correctly map to the if (serviceInstance is IExtension extension && !await extension.IsEnabledAsync()) branch in production.
• Initialization ordering: BuildAsync_WithInitializableProvider_CallsInitializeAsync correctly verifies TryInitializeAsync calls InitializeAsync when the provider implements IAsyncInitializableExtension. The separate BuildAsync_WithDisabledExtensionProvider_DoesNotCallInitializeAsync correctly confirms the continue statement prevents initialization of disabled extensions.
• Factory parameter forwarding: The capture-variable approach in BuildAsync_PassesLogLevelAndServiceProviderToFactory correctly exercises the factory(logLevel, serviceProvider) call site.
• IMonitor mock setup: The _mockMonitor.Setup(m => m.Lock(...)).Returns(...) correctly satisfies LoggerFactory.CreateLogger's using (_monitor.Lock(_sync)) pattern.

Threading & Concurrency

Each MSTest test method gets a fresh LoggingManagerTests instance, so the readonly Mock<> fields are not shared between tests. No threading concerns.

API Compliance

All assertions use MSTest APIs (Assert.ThrowsExactly, Assert.AreEqual, Assert.AreSame) and Moq Verify() — compliant with the project's BannedSymbols.txt which bans AwesomeAssertions.

Key Findings

No issues found in any review category (correctness, threading, performance, API compat, cross-TFM, resources, security, defensive coding).

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

1. [Coverage] Missing test: enabled extension + initializable provider — The production code has a path where a provider implements both IExtension (enabled) and IAsyncInitializableExtension, causing InitializeAsync() to be called after the IsEnabledAsync()=true check. This path is not exercised. BuildAsync_WithInitializableProvider_CallsInitializeAsync (line 76) does not cover it because IInitializableLoggerProvider doesn't implement IExtension, so the is IExtension check is false and it takes a completely different branch. IExtensionAndInitializableLoggerProvider (line 131) exists but is only used in the disabled case.

What's Good

• Isolation: Each test constructs a fresh LoggingManager; instance-level mocks are recreated per-test by MSTest's per-method instantiation — no shared mutable state.
• Assertions: Moq Verify with Times.Once / Times.Never gives precise failure messages.
• Factory capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and correctly verifies the parameters forwarded to the factory lambda.
• Test interfaces declared at the bottom of the file are a neat way to compose the exact capability mix each test needs without boilerplate classes.

Recommendations

Add one test for the enabled+initializable extension case as described in the inline comment — the IExtensionAndInitializableLoggerProvider interface is already available at the bottom of the file.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Copilot address review comment

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main into the branch. The conflict in LoggingManagerTests.cs was resolved by adopting the main branch's version (which was the authoritative merged version from #8124).

[Amaury Levé (Evangelink)]

Tests cover:

• AddProvider(null) throws ArgumentNullException
• BuildAsync with no providers returns a non-null factory
• Non-extension provider is always included
• IExtension provider with IsEnabledAsync()=true is included
• IExtension provider with IsEnabledAsync()=false is excluded (not added to factory)
• Non-extension IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Enabled IExtension + IAsyncInitializableExtension provider has InitializeAsync() called and is included in the factory
• Disabled IExtension + IAsyncInitializableExtension provider does NOT have InitializeAsync() called
• Factory is invoked with the correct LogLevel and IServiceProvider (parameterized across multiple log levels)

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

[Copilot]

Pull request overview

Adds a focused unit-test suite for LoggingManager.BuildAsync (Microsoft.Testing.Platform logging pipeline) to validate provider registration, extension enablement filtering, async initialization behavior, and factory delegate inputs.

Changes:

• Added unit tests validating AddProvider(null) throws and that providers are included/excluded based on IExtension.IsEnabledAsync().
• Added unit tests validating IAsyncInitializableExtension.InitializeAsync() is invoked only for included providers.
• Added a unit test validating the provider factory receives the expected LogLevel and IServiceProvider.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	New unit tests covering LoggingManager.BuildAsync provider filtering, initialization, and factory argument passing.

Copilot's findings

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

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

The new LoggingManagerTests are well-structured and cover the core scenarios cleanly. Three minor improvements stand out:

1. Missing test (Important) — The enabled IExtension + IAsyncInitializableExtension combination isn't tested. The disabled path is covered, but the enabled path (which should call InitializeAsync()) has no corresponding test.

2. Incomplete assertion (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync verifies InitializeAsync() was invoked but doesn't verify the provider was included in the resulting factory. The other "include" tests check both; this one should too.

3. Ambiguous test name (Minor) — BuildAsync_WithInitializableProvider_CallsInitializeAsync doesn't communicate that the provider intentionally omits IExtension. This distinction is meaningful given the two code paths under test.

Positive Highlights

• Consistent test structure and naming convention across all methods.
• Good use of interface composition to isolate specific behavioural combinations (extension-only, initializable-only, both).
• Internal test interfaces correctly follow the semicolon shorthand style (consistent with IDisposableLoggerProvider in LoggerFactoryTests.cs).
• _mockMonitor setup in the constructor mirrors the pattern in LoggerFactoryTests.

---

Generated by PR Nitpick Reviewer 🔍

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Analysis

This PR adds a single new test file (LoggingManagerTests.cs) with 7 test methods covering LoggingManager.BuildAsync. I reviewed the tests against the production implementation in LoggingManager.cs and LoggerFactory.cs.

Verified Correctness

• Null guard test: Correctly verifies that AddProvider(null!) throws ArgumentNullException — matches the production ?? throw new ArgumentNullException(...) guard.
• Provider inclusion/exclusion: Tests for enabled/disabled IExtension providers correctly map to the if (serviceInstance is IExtension extension && !await extension.IsEnabledAsync()) branch in production.
• Initialization ordering: BuildAsync_WithInitializableProvider_CallsInitializeAsync correctly verifies TryInitializeAsync calls InitializeAsync when the provider implements IAsyncInitializableExtension. The separate BuildAsync_WithDisabledExtensionProvider_DoesNotCallInitializeAsync correctly confirms the continue statement prevents initialization of disabled extensions.
• Factory parameter forwarding: The capture-variable approach in BuildAsync_PassesLogLevelAndServiceProviderToFactory correctly exercises the factory(logLevel, serviceProvider) call site.
• IMonitor mock setup: The _mockMonitor.Setup(m => m.Lock(...)).Returns(...) correctly satisfies LoggerFactory.CreateLogger's using (_monitor.Lock(_sync)) pattern.

Threading & Concurrency

Each MSTest test method gets a fresh LoggingManagerTests instance, so the readonly Mock<> fields are not shared between tests. No threading concerns.

API Compliance

All assertions use MSTest APIs (Assert.ThrowsExactly, Assert.AreEqual, Assert.AreSame) and Moq Verify() — compliant with the project's BannedSymbols.txt which bans AwesomeAssertions.

Key Findings

No issues found in any review category (correctness, threading, performance, API compat, cross-TFM, resources, security, defensive coding).

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

1. [Coverage] Missing test: enabled extension + initializable provider — The production code has a path where a provider implements both IExtension (enabled) and IAsyncInitializableExtension, causing InitializeAsync() to be called after the IsEnabledAsync()=true check. This path is not exercised. BuildAsync_WithInitializableProvider_CallsInitializeAsync (line 76) does not cover it because IInitializableLoggerProvider doesn't implement IExtension, so the is IExtension check is false and it takes a completely different branch. IExtensionAndInitializableLoggerProvider (line 131) exists but is only used in the disabled case.

What's Good

• Isolation: Each test constructs a fresh LoggingManager; instance-level mocks are recreated per-test by MSTest's per-method instantiation — no shared mutable state.
• Assertions: Moq Verify with Times.Once / Times.Never gives precise failure messages.
• Factory capture pattern in BuildAsync_PassesLogLevelAndServiceProviderToFactory is clean and correctly verifies the parameters forwarded to the factory lambda.
• Test interfaces declared at the bottom of the file are a neat way to compose the exact capability mix each test needs without boilerplate classes.

Recommendations

Add one test for the enabled+initializable extension case as described in the inline comment — the IExtensionAndInitializableLoggerProvider interface is already available at the bottom of the file.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Copilot address review comment

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main into the branch. The conflict in LoggingManagerTests.cs was resolved by adopting the main branch's version (which was the authoritative merged version from #8124).