Skip to content

refactor: apply Interface Segregation to TUnitServiceProvider#4939

Closed
thomhurst wants to merge 1 commit into
mainfrom
refactor/service-provider-isp
Closed

refactor: apply Interface Segregation to TUnitServiceProvider#4939
thomhurst wants to merge 1 commit into
mainfrom
refactor/service-provider-isp

Conversation

@thomhurst

Copy link
Copy Markdown
Owner

Summary

Addresses #4898 - TUnitServiceProvider exposes 35+ public properties, violating the Interface Segregation Principle.

  • Extracts three focused interfaces from TUnitServiceProvider so consumers depend only on the services they actually need:
    • ITestDiscoveryServices: DiscoveryService, TestBuilderPipeline, TestFilterService, TestFinder
    • ITestExecutionServices: TestExecutor, TestSessionCoordinator, CancellationToken, FailFastCancellationSource, AfterSessionHooksFailed
    • ILoggingServices: Logger, VerbosityService, MessageBus
  • Updates TestSessionCoordinator to depend on ITestExecutionServices instead of the full TUnitServiceProvider
  • Updates IRequestHandler and TestRequestHandler to accept the specific interfaces they need
  • TUnitServiceProvider implements all three interfaces, so TUnitTestFramework can pass it directly

This is an incremental first step -- further consumers and properties can be migrated to narrower interfaces in follow-up work.

Test plan

  • dotnet build TUnit.Engine/TUnit.Engine.csproj passes with zero errors and zero warnings
  • Existing tests continue to pass (no behavioral changes, pure refactoring)

Extract three focused interfaces from TUnitServiceProvider to allow
consumers to depend only on the services they actually need:

- ITestDiscoveryServices: DiscoveryService, TestBuilderPipeline,
  TestFilterService, TestFinder
- ITestExecutionServices: TestExecutor, TestSessionCoordinator,
  CancellationToken, FailFastCancellationSource, AfterSessionHooksFailed
- ILoggingServices: Logger, VerbosityService, MessageBus

Update consumers:
- TestSessionCoordinator now depends on ITestExecutionServices
- TestRequestHandler/IRequestHandler now accept the specific interfaces
- TUnitTestFramework passes serviceProvider (which implements all three)
@claude

claude Bot commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.


This is a clean, well-executed Interface Segregation refactor. A few notes worth calling out:

What's done well:

  • All three new interfaces (ILoggingServices, ITestDiscoveryServices, ITestExecutionServices) are properly scoped as internal, so no public API surface is changed and no snapshot test updates are needed.
  • The call site in TUnitTestFramework correctly passes serviceProvider three times (once per interface). Since TUnitServiceProvider implements all three, the compiler resolves each argument to the correct interface type — no runtime overhead or object allocation introduced.
  • The circular dependency in TUnitServiceProvider's constructor (executionServices: this before TestSessionCoordinator is assigned) is safe: TestSessionCoordinator only stores the reference and doesn't dereference executionServices.TestSessionCoordinator until after full construction.
  • TestSessionCoordinator correctly accesses only the three members it needs (FailFastCancellationSource, CancellationToken, AfterSessionHooksFailed) — all of which are present on ITestExecutionServices.
  • CLAUDE.md compliance: Dual-Mode rule is not triggered (this is post-metadata-collection orchestration only), AOT compatibility is preserved, no async blocking introduced.

One design note (not a blocker): AfterSessionHooksFailed { get; set; } on ITestExecutionServices exposes mutable shared state through an interface, which is a design smell. It was already present on TUnitServiceProvider before this PR, so this isn't a regression — but it's worth addressing in a follow-up alongside the incremental migrations mentioned in the PR description.

Overall, this is a solid first step in the incremental ISP migration.

@thomhurst thomhurst closed this Feb 19, 2026
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.

1 participant