[Test Improver] test: add unit tests for LoggingManager.BuildAsync and ExtensionValidationHelper.ValidateUniqueExtension

[Evangelink]

Adds unit tests for LoggingManager.BuildAsync covering:

• No providers case (returns non-null factory, verified by also calling CreateLogger)
• Non-extension provider inclusion
• Extension provider enabled/disabled filtering (with IsEnabledAsync call verification)
• IAsyncInitializableExtension initialization for both enabled and disabled extension provider paths
• Non-extension initializable provider path
• Factory receives correct log level and service provider (parametrized across multiple log levels)

Adds unit tests for ExtensionValidationHelper.ValidateUniqueExtension covering:

• Null argument validation for both overloads
• Empty collection (no-throw)
• No-duplicate case (no-throw)
• Single and multiple duplicates (throw), including error message content (uid and both distinct concrete type names)
• Case-sensitivity behavior (UIDs differing only by case do not conflict)
• Custom selector overload with wrapper types

Also fixes a build error in DesktopTestSourceHostTests.cs introduced by the Moq upgrade to 4.20.72, which added nullable reference type annotations requiring null literals passed to Mock<TestSourceHost> constructor args to be cast to (IRunSettings?)null.

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

[Copilot]

Pull request overview

Adds targeted unit-test coverage in Microsoft.Testing.Platform.UnitTests for two internal helpers in Microsoft.Testing.Platform: log factory construction (LoggingManager.BuildAsync) and extension UID uniqueness enforcement (ExtensionValidationHelper.ValidateUniqueExtension).

Changes:

• Add LoggingManager.BuildAsync tests covering provider inclusion/exclusion (extension enabled/disabled), async initialization, multiple providers, and argument validation for AddProvider.
• Add ExtensionValidationHelper.ValidateUniqueExtension tests covering null guards for both overloads, duplicate detection (single/multiple), custom selector behavior, and key error-message content.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	New tests validating LoggingManager.BuildAsync provider filtering/initialization and factory callback inputs.
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Helpers/ExtensionValidationHelperTests.cs	New tests validating duplicate-UID detection and null-guard behavior for both ValidateUniqueExtension overloads.

Copilot's findings

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

[Evangelink]

Summary

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

Key Findings

This is a clean, well-structured PR adding unit tests for LoggingManager.BuildAsync and ExtensionValidationHelper.ValidateUniqueExtension. The test naming follows the MethodName_Condition_ExpectedResult convention consistently, coverage is thorough, and the CreateExtension factory helper is a nice touch. A few small improvements are worth considering:

1. Important — ValidateUniqueExtension_OneDuplicate_ErrorMessageContainsBothTypeNames asserts on existingTypeName and newTypeName, but since both mocks are Mock<IExtension>, Moq produces the same proxy type for both — the two assertions check the same string. The test doesn't actually validate distinct type names.

2. Important — BuildAsync_ExtensionProviderDisabled_IsExcluded declares mockLogger and sets up CreateLogger on the disabled provider, but this setup is never invoked. Compare with BuildAsync_ExtensionDisabled_IsNotInitialized which correctly omits the unused mock.

3. Minor — mockEnabled/mockDisabled naming in BuildAsync_MixedEnabledAndDisabledExtensions_OnlyEnabledIncluded breaks the mockProvider* naming convention used throughout the rest of the file.

4. Minor — Two null-guard tests in ExtensionValidationHelperTests.cs use Mock<IExtension> newExtension = new() instead of the CreateExtension(...) helper used everywhere else.

5. Minor — The three helper internal interface declarations are placed after the test class closing brace; moving them before the class (or to a separate file) makes them easier to discover.

Recommendations

• Fix the ErrorMessageContainsBothTypeNames test to use two concrete IExtension implementations with different types, or rename the test to reflect what it actually verifies.
• Remove the superfluous mockLogger + CreateLogger setup from the disabled-provider test.

---

Generated by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Evangelink]

Summary

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

Key Findings

1. [Assertion — Important] ValidateUniqueExtension_OneDuplicate_ErrorMessageContainsBothTypeNames: Both existing.Object.GetType() and newExtension.Object.GetType() resolve to the same Moq-generated proxy type, so existingTypeName == newTypeName. The two Assert.Contains calls are asserting the same string, providing false confidence that both distinct type names appear in the error message.

2. [Coverage — Important] Missing test for the enabled+initializable provider path: BuildAsync_ExtensionDisabled_IsNotInitialized covers the disabled case for IEnabledInitializableLoggerProvider, but there is no test verifying that an enabled IEnabledInitializableLoggerProvider gets initialized. This leaves a gap in the production code path IsEnabledAsync() == true → TryInitializeAsync().

3. [Assertion — Moderate] BuildAsync_ExtensionDisabled_IsNotInitialized: Verifying InitializeAsync() is Times.Never is necessary but not sufficient. A verification that IsEnabledAsync() was actually called would close the loop and prevent bugs where the enabled check is bypassed entirely.

4. [Assertion — Minor] BuildAsync_NoProviders_ReturnsEmptyLoggerFactory: The test name claims behavioral verification ("empty factory") but only asserts IsNotNull. Either rename or strengthen the assertion.

Recommendations

• Use concrete stub types (not mocks) in ErrorMessageContainsBothTypeNames to get genuinely distinct type names
• Add a BuildAsync_ExtensionEnabled_IsInitialized test using IEnabledInitializableLoggerProvider with IsEnabledAsync() returning true
• Add Verify(p => p.IsEnabledAsync(), Times.Once) alongside the existing Times.Never assertion
• Rename BuildAsync_NoProviders_ReturnsEmptyLoggerFactory to reflect what is actually tested

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Evangelink]

Summary

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

Key Findings

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

Verified correct:

• Assert.Contains(string, string) overload resolution — Assert.Contains("my-uid", ex.Message) correctly resolves to the string-specific overload Contains(string substring, string value), not the collection overload. Checking ex.Message contains the substring is accurate.

• IEnabledLoggerProvider / IInitializableLoggerProvider / IEnabledInitializableLoggerProvider interface design — The three test-local interfaces precisely model the combinations of ILoggerProvider, IExtension, and IAsyncInitializableExtension that LoggingManager.BuildAsync branches on. The enabled/disabled/initializable logic is exercised correctly.

• LoggingManager.BuildAsync enabled-check logic — When IsEnabledAsync() returns false, the continue correctly prevents both TryInitializeAsync and loggerProviders.Add; the tests for BuildAsync_ExtensionDisabled_IsNotInitialized and BuildAsync_ExtensionProviderDisabled_IsExcluded verify both effects correctly.

• Non-extension initializable provider — IInitializableLoggerProvider : ILoggerProvider, IAsyncInitializableExtension (without IExtension) correctly bypasses the enabled check and proceeds to TryInitializeAsync, as the test BuildAsync_InitializableProvider_IsInitialized verifies.

• ExtensionValidationHelper selector overload — The WithCustomSelector test correctly exercises the generic overload with a tuple collection and a selector lambda.

• MSTest assertion APIs — All assertions use MSTest's Assert class in accordance with the project's BannedSymbols.txt (which bans AwesomeAssertions in this test project).

• Internal access — LoggingManager and IMonitor are internal, but Microsoft.Testing.Platform.UnitTests has an InternalsVisibleTo entry in the main project, so compilation will succeed.

Recommendations

None — the tests are functionally accurate and cover the key behavioral paths.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

@copilot address review comment

[Evangelink]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. Main had independently landed similar test files via #8124 and #8128 (add/add conflicts). The merged result takes main's version as the base and incorporates our improvements: interfaces placed before the test class, and IsEnabledAsync Times.Once verification in the disabled-provider test.

[Evangelink]

@copilot please fix the followinb build errors:

@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 please fix the followinb build errors:

@azure-pipelines
...

Fixed in the latest commit. Moq 4.20.72 added nullable reference type annotations, so passing a bare null literal to Mock<TestSourceHost>(location, null) now triggers CS8625. Changed both occurrences (lines 107 and 164) to (IRunSettings?)null to supply a typed null that satisfies the NRT constraint.

[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 work in this PR has been superseded. The LoggingManager tests landed in main via #8124 and #8130, and the ExtensionValidationHelper tests landed via #8128. This PR is a duplicate and can be safely closed.

Generated by Daily Test Improver · ● 9.5M · ◷