Event Facade: Make it easier to register multiple subscribers at once

[IonBazan]

This change makes it much easier to register multiple subscribers with a single method call.

[codecov[bot]]

Codecov Report

Merging #4800 (963014a) into master (54f46a7) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #4800   +/-   ##
=========================================
  Coverage     83.37%   83.38%           
- Complexity     5749     5751    +2     
=========================================
  Files           532      532           
  Lines         15413    15418    +5     
=========================================
+ Hits          12851    12856    +5     
  Misses         2562     2562           

Impacted Files	Coverage Δ
src/Event/Facade.php	82.60% <100.00%> (+0.79%)	:arrow_up:
src/Logging/JUnit/JunitXmlLogger.php	96.64% <100.00%> (+0.01%)	:arrow_up:
src/Runner/ResultCache/ResultCacheHandler.php	70.90% <100.00%> (+0.53%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 54f46a7...963014a. Read the comment docs.

[theseer]

I generally would say I agree that this change could be handy. It's not adding much complexity yet makes the calling/registration potentially indeed more readable when multiple subscribers are used.

I'd also second the notion to not use a collection or iterable but stick with ....

Yet, I do have a few small concerns / nit-picky things:

• This PR combines the change in the Facade with the refactoring of the using classes in one single commit. It would be nicer to have these separated.

• This PR changes code, introduces a new method even, but does not add any tests. I do realize that we don't have a test for the Facade yet. Maybe we should make sure we have that before changing the class.

@sebastianbergmann I fail to read the CodeCov Report: How can an increase of code and complexity without additional tests also increase the coverage??

[IonBazan]

@theseer Thanks for your feedback - I'm glad you find this change helpful 😉 .

• Let me rebase that and split into separate commits then
• Agree on that. I will try to provide some tests but I'm not sure if that's possible here - this is a "facade" so many dependencies are hardcoded for ease of use.

The reason why the coverage increased even though the complexity increased too is that the new registerSubscribers() is calling the old registerSubscriber() so when I replaced all usages of registerSubscriber() with registerSubscribers(), tests that cause JunitXmlLogger::registerSubscribers() to be called, mark both the old and new method as covered (thus Event\Facade increased coverage). Adding the fact that JunitXmlLogger::registerSubscribers() has 1 more line than before, it adds another covered line to the report. I hope that solves your concern.

Keeping that in mind, it seems that the integration tests are actually making sure Event\Facade is working properly so I'm not really sure if it makes sense to add unit tests for this class.

[theseer]

I deliberately didn't say "Unit" test. I'm perfectly fine with having something that could qualify as some sort of integration test for it. Maybe I'm paranoid, but I'm feeling a bit uneasy when modifying an existing class without having tests. Granted, the change is small and I cannot see what possibly could break but still...

:)

[sebastianbergmann]

I am sorry, this fell of my radar. I am also sorry that I did not pick your commit, Ion, but doing it myself was quicker than solving the conflicts.

We still do not have any test for the facade, as Arne already pointed out. We will deal with that when we review the tests for the event system.

[IonBazan]

No problem @sebastianbergmann, this PR serves more as a RFC rather than the actual change to be committed.