Skip to content

[Test Improver] Add unit tests for LoggingManager.BuildAsync#8104

Open
Evangelink wants to merge 5 commits into
mainfrom
test-assist/logging-manager-tests-26f492a017011427
Open

[Test Improver] Add unit tests for LoggingManager.BuildAsync#8104
Evangelink wants to merge 5 commits into
mainfrom
test-assist/logging-manager-tests-26f492a017011427

Conversation

@Evangelink
Copy link
Copy Markdown
Member

@Evangelink Evangelink commented May 11, 2026

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

Tests cover:
- AddProvider with null throws ArgumentNullException
- BuildAsync with no providers returns empty factory
- Factory delegate receives correct LogLevel and IServiceProvider
- Non-IExtension provider is always included
- Enabled IExtension provider is included
- Disabled IExtension provider is excluded
- IAsyncInitializableExtension provider has InitializeAsync called
- Disabled IAsyncInitializableExtension provider does not have InitializeAsync called

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 11, 2026 12:49
Copy link
Copy Markdown
Member Author

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🔍

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🧪

Copy link
Copy Markdown
Member Author

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

IDisposableILoggerFactory 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 🧠

@Evangelink
Copy link
Copy Markdown
Member Author

@copilot address review comments

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented May 11, 2026

@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.

@Evangelink
Copy link
Copy Markdown
Member Author

@copilot resolve the merge conflicts in this pull request

@Evangelink
Copy link
Copy Markdown
Member Author

@copilot resolve the merge conflicts in this pull request

Copilot AI and others added 2 commits May 11, 2026 16:32
…Tests.cs

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
…Tests.cs

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 11, 2026 16:32
@Evangelink Evangelink review requested due to automatic review settings May 11, 2026 16:32
Copy link
Copy Markdown
Contributor

Copilot AI commented May 11, 2026

@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).

Copy link
Copy Markdown
Contributor

Copilot AI commented May 11, 2026

@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.

@Evangelink
Copy link
Copy Markdown
Member Author

@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.

…tead of null

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 11, 2026 20:57
@Evangelink Evangelink review requested due to automatic review settings May 11, 2026 20:57
Copy link
Copy Markdown
Contributor

Copilot AI commented May 11, 2026

@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.

@Evangelink
Copy link
Copy Markdown
Member Author


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 ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants