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

[Evangelink]

• Reproduce and inspect the CS8625 failures in DesktopTestSourceHostTests
• Apply minimal nullable-safe test fixes
• Run targeted unit tests for MSTestAdapter.PlatformServices.UnitTests (attempted twice; blocked by transient package feed download failures)
• Run final validation checks and reply to the PR comment

[Copilot]

Pull request overview

Adds unit test coverage in Microsoft.Testing.Platform.UnitTests for two previously untested internal helpers: LoggingManager.BuildAsync (provider filtering/initialization and factory construction) and ExtensionValidationHelper.ValidateUniqueExtension (UID uniqueness enforcement during extension registration). This supports the platform’s reliability by locking down branching behavior around extension enablement and duplicate registration.

Changes:

• Added LoggingManagerTests covering provider factory argument passing, inclusion/exclusion of extension providers based on IsEnabledAsync, and conditional InitializeAsync execution.
• Added ExtensionValidationHelperTests covering null-guard behavior, duplicate UID detection (including message content), case-sensitivity, and selector-based overload behavior.
• Extended TestExtension with UidOverride to simplify configuring unique/duplicate UIDs in tests.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/LoggingManagerTests.cs	New unit tests for LoggingManager.BuildAsync provider filtering/initialization and factory wiring.
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Helpers/TestExtension.cs	Adds UidOverride to allow configurable extension UIDs in unit tests while preserving the default UID.
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Helpers/ExtensionValidationHelperTests.cs	New unit tests for ValidateUniqueExtension overloads, including null guards and duplicate UID behavior.

Copilot's findings

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

[Evangelink]

Summary

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

---

Good addition of unit tests for LoggingManager.BuildAsync and ExtensionValidationHelper.ValidateUniqueExtension. The test structure is clear and the naming convention follows the established MethodName_Condition_ExpectedResult pattern consistently. A few minor issues are flagged below.

Key Findings

1. Important — Dead mock setup (×2) (LoggingManagerTests.cs lines 125–126, 145–146): Two tests (BuildAsync_PassesCorrectLogLevelToFactory, BuildAsync_PassesServiceProviderToFactory) declare mockLogger and wire up CreateLogger, but neither test ever invokes factory.CreateLogger. The setup is noise that could mislead future readers.

2. Minor — Asymmetric coverage (ExtensionValidationHelperTests.cs ~line 88): WithSelector tests cover case-sensitivity but SimpleOverload tests do not. Since both overloads share the same comparison logic, adding a parallel test would ensure parity.

3. Minor — Unconventional section dividers (ExtensionValidationHelperTests.cs line 13): The // ---- ... ---- style banners are not used elsewhere in the test suite; a plain comment or no divider would be more consistent.

Recommendations

• Remove the unused mockLogger variable and CreateLogger mock setup from the two capture-focused tests.
• Consider adding ValidateUniqueExtension_SimpleOverload_UidIsCaseSensitive_DifferentCaseDoesNotThrow.
• Replace the // ---- ... ---- dividers with a simpler comment style.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Evangelink]

Summary

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

Key Findings

No issues found. Analysis covered:

• Correctness: Tests accurately model LoggingManager.BuildAsync behavior — extension check via IExtension.IsEnabledAsync() is correctly bypassed for non-IExtension providers, and the IAsyncInitializableExtension + IExtension separation is properly exercised via the three composite interfaces. ExtensionValidationHelper tests correctly validate ordinal (case-sensitive) UID comparison and both overloads.

• Threading: LoggerFactory.CreateLogger uses IMonitor.Lock for thread safety. The mock setup in the test constructor (_mockMonitor.Setup(m => m.Lock(...)).Returns(...)) correctly accounts for this.

• API Compat: TestExtension.UidOverride (defaulting to "Uid") is a backward-compatible addition — all existing callers that relied on Uid returning "Uid" are unaffected.

• Resources: ILoggerFactory returned by BuildAsync is not disposed in tests, but this is an accepted pattern for test code with no real handles held.

• Cross-TFM: Test-only code. No TFM-specific APIs introduced.

• Security / Defensive: No concerns in test helper code.

Positive Observations

• Good coverage of edge cases: null inputs, empty collections, enabled/disabled filtering, and the case-sensitivity contract.
• Composite interface definitions (ILoggerProviderAndExtension, ILoggerProviderExtensionAndInitializable, etc.) are well-chosen to precisely test orthogonal code paths in BuildAsync.
• Mock setups are explicit about IsEnabledAsync() return values rather than relying on Moq defaults, making test intent clear.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

Summary

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

Key Findings

The tests are well-structured, isolated, and use appropriate assertion patterns (Assert.ThrowsExactly, Assert.AreEqual, Moq .Verify). No flakiness or isolation issues found. The findings are all coverage gaps:

1. Missing: enabled extension+initializable provider path (LoggingManagerTests) — The test suite covers the disabled-extension+initializable case and the non-extension+initializable case, but not the enabled-extension+initializable case. This means a regression in TryInitializeAsync() being called for enabled extension providers would go undetected.

2. Unclear scope of BuildAsync_WithInitializableProvider_CallsInitializeAsync — The provider under test (ILoggerProviderAndInitializable) does not implement IExtension and bypasses the extension-enablement check. The test name doesn't convey this; renaming to BuildAsync_WithNonExtensionInitializableProvider_CallsInitializeAsync would clarify the intent.

3. Multiple-duplicate scenario untested (ExtensionValidationHelperTests) — The production code collects all duplicates and formats their type names into the error message; all tests only set up a single pre-existing duplicate, leaving the multi-duplicate formatting path uncovered.

4. Case-sensitivity not tested for SimpleOverload — Minor consistency gap since it delegates to the generic overload, but symmetry in the test suite is useful documentation.

Recommendations

• Add a test for BuildAsync_WithEnabledExtensionInitializableProvider_CallsInitializeAsync using ILoggerProviderExtensionAndInitializable with IsEnabledAsync() → true.
• Consider renaming BuildAsync_WithInitializableProvider_CallsInitializeAsync to reflect it tests the non-extension variant.
• Add a multi-duplicate test case to ExtensionValidationHelperTests.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Evangelink]

@copilot address review comments

[Copilot]

@copilot address review comments

Done in commit 064f249. I addressed the actionable review feedback: removed dead mock setup, added the missing enabled extension+initializable coverage, added simple-overload case-sensitivity coverage, added a multi-duplicate validation test, and adjusted section divider comment style.

[Evangelink]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved in commit 99e7312fb by merging origin/main into this branch and fixing the LoggingManagerTests.cs merge conflict.

[Evangelink]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved in commit 5c9c12b by merging the latest origin/main and fixing the ExtensionValidationHelperTests.cs merge conflict.

[Evangelink]

@copilot fix the following 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.@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 the following build errors

...

Fixed in commit 9690f8c by updating DesktopTestSourceHostTests to pass (IRunSettings?)null in the two Mock<TestSourceHost> constructor calls, resolving the CS8625 build errors at lines 107 and 164.

[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 · ◷