Register Facade Subscribers where state is already global

[Slamdunk]

This PR decouples the classes that want to register some \PHPUnit\Event\Subscriber from the specific \PHPUnit\Event\Facade instance in place, the global one.

The global instance is now altered (by Facade::registerSubscribers, Facade::registerSubscriber and Facade::registerTracer) only in:

1. \PHPUnit\TextUI\Application
2. \PHPUnit\Runner\Extension\Facade
3. \PHPUnit\TestRunner\TestResult\Facade
4. \PHPUnit\TextUI\Output\Facade

Which already are single instance by architecture.

All the involved non-Facade classes can now be unit-tested, if neeeded:

1. \PHPUnit\Logging\JUnit\JunitXmlLogger
2. \PHPUnit\Logging\TeamCity\TeamCityLogger
3. \PHPUnit\Logging\TestDox\TestResultCollector
4. \PHPUnit\Runner\ResultCache\ResultCacheHandler
5. \PHPUnit\TestRunner\TestResult\Collector
6. \PHPUnit\TextUI\Output\Default\ProgressPrinter\ProgressPrinter

It is my intention to propose another PR after this one to refactor all Facades to pure Singleton too.

[codecov]

Codecov Report

Merging #5204 (7bb52b7) into 10.0 (176f4ae) will increase coverage by 0.00%.
The diff coverage is 92.68%.

@@            Coverage Diff            @@
##               10.0    #5204   +/-   ##
=========================================
  Coverage     84.29%   84.30%           
- Complexity     5863     5865    +2     
=========================================
  Files           635      635           
  Lines         18578    18588   +10     
=========================================
+ Hits          15661    15671   +10     
  Misses         2917     2917           

Impacted Files	Coverage Δ
src/Runner/Extension/Facade.php	33.33% <33.33%> (ø)
src/TextUI/Application.php	80.35% <87.50%> (+0.14%)	⬆️
src/Event/Facade.php	89.55% <92.68%> (+0.32%)	⬆️
src/Logging/JUnit/JunitXmlLogger.php	96.02% <100.00%> (ø)
src/Logging/TeamCity/TeamCityLogger.php	93.01% <100.00%> (ø)
...Logging/TestDox/TestMethod/TestResultCollector.php	72.15% <100.00%> (ø)
src/Runner/ResultCache/ResultCacheHandler.php	64.28% <100.00%> (ø)
src/Runner/TestResult/Collector.php	89.94% <100.00%> (ø)
src/Runner/TestResult/Facade.php	90.00% <100.00%> (ø)
...Output/Default/ProgressPrinter/ProgressPrinter.php	98.43% <100.00%> (ø)
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[theseer]

I generally like the idea of decoupling the actual (global) Facade from the individual classes using them - while this PR of course only addresses the registration of Subscribers or Tracers and not the emitting of events.

I have two problems with the implementation this PR proposes:

• I don't like the implicit "protocol" of having to know that one has to manually call the registration
• I seriously dislike the array return type as it's generic and not providing any type safety

Maybe we can fix these problems somehow?

[Slamdunk]

Maybe we can fix these problems somehow?

Of course we can.
Let me underline one thing first: I have many improvements and refactorings in mind to improve the code quality without alterning external nor internal APIs, but I've learned to propose one bit at time to ease the review process; I'm still learning to do it correctly though.

• I don't like the implicit "protocol" of having to know that one has to manually call the registration

Neither do I. A better approach is to require a \PHPUnit\Event\Facade instance in the constructors, but that requires a pure Singleton implementation for \PHPUnit\Event\Facade first.
I can propose that change here too is it's ok for you.

• I seriously dislike the array return type as it's generic and not providing any type safety

After the aforementioned change, the public function provideSubscribers(): array API is not necessary anymore.

[Slamdunk]

I've pushed the aforementioned refactor:

1. \PHPUnit\Event\Facade is now a pure Singleton (6f4942f) but I kept ::emitter() as is to ease its usage
2. Applied Inversion-of-Control for the classes that need to register Subscriber (6e8fae8)

[localheinz]

@Slamdunk

Maybe I am missing where you are headed with this pull request. Can you elaborate why this change would make sense?

[Slamdunk]

Sure: I'm the maintainer of ParaTest, a tool to support parallel testing in PHPUnit. In order to avoid copy-pasting basically the entire PHPUnit project, I'm re-using a lot of PHPUnit's @internal classes, and that makes ParaTest itself untestable.

The most evident friction point is the Singleton classes, or better, an unusal implementation of the Singleton pattern.
We have already successfully refactored the Singletons related to the Code Coverage in #5179 and now I can handle Code Coverage for ParaTest itself and ParaTest users without conflicting with PHPUnit's one.

I would like to be able to use all the Loggers as well, but in the current 10.0 branch they are tightly coupled with the global Facade instance.

My goal is here is to keep the current Singletons, as they are not the problem themselves, but to make them decoupled from non-global instances, and also reusable outside the global instance by keeping the constructor public.

The benefit for ParaTest is that I can reuse everything PHPUnit already has without mangling its internal state.
The benefit for PHPUnit is that all the classes become unit-testable, if needed.

[theseer]

This is certainly heading in the right direction.

I'm still having a few smaller issues though. I'm not sure if all got introduced by your changes or have been there to begin with and now only became more obvious. ;)

I'll be heading to Montreal for Confoo tomorrow and maybe I'll have some time on the flight to review things a bit more in depth.

[Slamdunk]

Hi @theseer, have you had the chance to review this?

[sebastianbergmann]

@Slamdunk Arne is still on vacation this week, he'll be back next week. I will review your changes together with @theseer and @localheinz no later than March 17-19.

[sebastianbergmann]

@Slamdunk Sorry to bother you with this, but with recent changes that I made, this PR now has conflicts that must be resolved. Can you update it? Thanks!

[Slamdunk]

Can you update it? Thanks!

Done 😉

[sebastianbergmann]

Merged manually, thanks!

And thanks to @theseer and @localheinz for the joint code review.

[Slamdunk]

Hi @sebastianbergmann , when merging PR from Github, it keeps tracking of the original PR even in the commit and the GUI:

I've seen that when you manually merge some PR, that information gets lost:

I find that link very useful to track down the PR and the precious conversation it has.
It would be great to find a way to keep the workflow you prefer AND that information.

[sebastianbergmann]

I will try to keep that in mind.