Allow multiple runs

[Slamdunk]

Q	A
PHPUnit version	10.0.x-dev
PHP version	8.1.5
Installation Method	Composer

Summary

Hi, I'm the maintainer of ParaTest, a tool to run PHPUnit tests in parallel.

One way ParaTest can run is to spawn $(nproc) processes and run multiple tests within a single process.
Multiple tests are called each with a separate \PHPUnit\TextUI\Application::run call withing the same process.
This is a design choice that allow both:

1. Single bootstrap operation for all the tests within the same process
2. Dynamic (so non-preemptive) split of tests across $(nproc) processes

To maximise the overall speed.

This design collides with the new Facade singleton in c853041, of which seal is never released after the run ends.

So I'd like to ask:

1. either to reset the Singleton after the run
2. or (better) remove the Singleton design and leverage a proper dependency injection

[theseer]

While I agree the design with the Facade and how it is currently implemented leaves room for improvements, I so far fail to see how either of your proposed changes would address the issue - at least as how I currently understand it:

1. either to reset the Singleton after the run

As I understand your explanation above, you have multiple concurrent runs in parallel within the same application process - how ever that is achieved. How would resetting the state of the Facade help you after run? Each run would have to wait for the previous to complete - killing the very idea of paratest? What am I missing here?

2. or (better) remove the Singleton design and leverage a proper dependency injection

Nitpicking alert: Technically, there is no Singleton. The Facade keeps track of the instance it created, which is not the same as a Singleton but of course the point of the Facade. Using DI would not change that.

I'm afraid as of now I'm also not understanding (yet?) why that would be a major problem. The Application::run method and the application flow in general simply calls methods of the Facade, which dispatches it - as facades do ;) - to the instance of the dispatcher / emitter that it knows. Once created, the instance stays the same.

So where exactly is the problem when reaching the level of test execution? You should not add new listeners or tracers once testing started. For that reason, the sealing has been introduced. It only affects the registration of new listeners and tracers.

Maybe I'm missing something very obvious but so far I do not see a problem?

[Slamdunk]

As I understand your explanation above, you have multiple concurrent runs in parallel within the same application process - how ever that is achieved. How would resetting the state of the Facade help you after run? Each run would have to wait for the previous to complete - killing the very idea of paratest? What am I missing here?

Multiple consecutive application runs run in the same process. It is the equivalent of writing such script:

(new PHPUnit\TextUI\Application())->run(['phpunit', 'test/MyTest1.php'], false);
(new PHPUnit\TextUI\Application())->run(['phpunit', 'test/MyTest2.php'], false);
(new PHPUnit\TextUI\Application())->run(['phpunit', 'test/MyTest3.php'], false);

Nitpicking alert: Technically, there is no Singleton. The Facade keeps track of the instance it created, which is not the same as a Singleton but of course the point of the Facade. Using DI would not change that.

Sure it is not a Singleton strictly speaking, but it is a stateful static class, so it behaves like a Singleton

So where exactly is the problem when reaching the level of test execution? You should not add new listeners or tracers once testing started. For that reason, the sealing has been introduced. It only affects the registration of new listeners and tracers.

Maybe I'm missing something very obvious but so far I do not see a problem?

If you run the above example, the first test runs smoothly, but the second exits with the following error:

An error occurred inside PHPUnit.

Message:  (no message)
Location: ./phpunit/src/Event/Facade.php:32

#0 ./src/Runner/ResultCache/ResultCacheHandler.php(143): PHPUnit\Event\Facade::registerSubscriber()
#1 ./src/Runner/ResultCache/ResultCacheHandler.php(46): PHPUnit\Runner\ResultCache\ResultCacheHandler->registerSubscribers()
#2 ./src/TextUI/TestRunner.php(111): PHPUnit\Runner\ResultCache\ResultCacheHandler->__construct()
#3 ./src/TextUI/Application.php(110): PHPUnit\TextUI\TestRunner->run()
#4 ./src/TextUI/Application.php(78): PHPUnit\TextUI\Application->run()
#5 ./phpunit(91): PHPUnit\TextUI\Application::main()
#6 {main}

The optimal solution would be to remove every static keyword, forcing Facade object state to be bound to the Application that creates it, and not to the whole PHP process.

[sebastianbergmann]

Sorry to keep you waiting, Filippo.

In general, I am not a fan (quite the opposite, actually) of singletons, global state, etc. and agree with you that using dependency injection to make emitting events possible wherever needed. However, for this first iteration of the event system, which also has to deal with legacy test execution, I would prefer to stick with the current approach and revisit the situation when we have a new test runner (this is the next big improvement I want to work on after PHPUnit 10 has been shipped).

In the meanwhile, I have no problem at all with implementing your "reset the Singleton after the run" suggestion. Please let me know if you would like to send a pull request for that. Otherwise I will implement it ASAP.

[theseer]

Sorry, maybe I'm just blind or stupid - or it's still to early but I still fail to see the problem with the facade. I'm perfectly happy to admit the implementation is far from perfect and should probably get revisited.

But as @Slamdunk describes with the consecutive calls of Application::run, the problem is caused by calling that very method multiple times despite being the single application entry point. Instead of providing a "reset" feature to a Facade that PHPUnit doesn't need for itself and with that possibly causing inconsistencies, allowing others to call the reset mid-run and what not, we should provide a better way to 3rd parties to trigger test execution without rerunning things that do not require rerunning.

This all has nothing to do with the Facade. As I (currently?) see it we'd be fixing the wrong problem here..

[Slamdunk]

In the meanwhile, I have no problem at all with implementing your "reset the Singleton after the run" suggestion. Please let me know if you would like to send a pull request for that. Otherwise I will implement it ASAP.

It seems you've already in mind the best solution, so I'll leave it to you.

The only thing that matters to me is the following test to pass:

--TEST--
phpunit ../../_files/BankAccountTest.php
--FILE--
<?php declare(strict_types=1);
$_SERVER['argv'][] = '--do-not-cache-result';
$_SERVER['argv'][] = '--no-configuration';
$_SERVER['argv'][] = __DIR__ . '/../../_files/BankAccountTest.php';

require_once __DIR__ . '/../../bootstrap.php';
PHPUnit\TextUI\Application::main(false);
PHPUnit\TextUI\Application::main(false);
--EXPECTF--
PHPUnit %s by Sebastian Bergmann and contributors.

Runtime: %s

...                                                                 3 / 3 (100%)

Time: %s, Memory: %s

OK (3 tests, 3 assertions)
PHPUnit %s by Sebastian Bergmann and contributors.

Runtime: %s

...                                                                 3 / 3 (100%)

Time: %s, Memory: %s

OK (3 tests, 3 assertions)

@theseer I don't think 3rd parties should reset the state: it's PHPUnit's duty to leave a clean state after run

[theseer]

@theseer I don't think 3rd parties should reset the state: it's PHPUnit's duty to leave a clean state after run

Valid point. I was actually somehow expecting it to be a public method, but of course there could be varying degrees of public here if we hide that call somehow within the facade.

That leaves the question, what do we consider a "run"? And why would we want to re-setup everything of the 2nd+ calls? With the risk of those being somehow different?

[Slamdunk]

That leaves the question, what do we consider a "run"?

PHPUnit\TextUI\Application::main or PHPUnit\TextUI\Application::run?

And why would we want to re-setup everything of the 2nd+ calls? With the risk of those being somehow different?

I don't understand these two questions. At the end of run, PHPUnit should reset itself to be able to run again with the same result. What's the issue you are trying to pinpoint?

[theseer]

That leaves the question, what do we consider a "run"?

PHPUnit\TextUI\Application::main or PHPUnit\TextUI\Application::run?

See, that's where I'm not sure yet. And that would include a bunch of things done plus accompanying events that are useless to be redone and emitted again: Startup, Config handling, Facade Setup, Extension bootstrapping, .... as well as cleanup.

As far as I understand it, we're trying to allow 3rd parties to trigger test execution. We do not need the aforementioned steps to be run again for every consecutive call ("run").

And why would we want to re-setup everything of the 2nd+ calls? With the risk of those being somehow different?

I don't understand these two questions. At the end of run, PHPUnit should reset itself to be able to run again with the same result. What's the issue you are trying to pinpoint?

If we do as suggested, there is exactly no control from within PHPUnit to ensure it's exactly the same result. But maybe I'm still missing the point of all this..

[Slamdunk]

But maybe I'm still missing the point of all this.

Without what I'm asking here, dynamic (so non-preemptive) parallel test runs with a fixed number of long running parallel processes is impossible to achieve.

And we need this in ParaTest to provide the fastest run possible for the entire test suite.

And to be clear, the 20M downloads we have, already achieve this in PHPUnit 9, but V10 won't allow to in the current form

[theseer]

I'm not arguing against what paratest is offering :) quite the opposite.
I'm not sure that we (as in PHPUnit) do provide you and others with the best means to achieve it.

Maybe we should have a short video call to discuss this in person to maybe resolve my confusion and find a not only a working solution but an optimal one :)

[Slamdunk]

Maybe we should have a short video call to discuss this in person

The call just ended; we agreed to have a step back and discuss the problem instead of the solution.
What ParaTest needs is:

1. Spawn a process
2. Run the bootstrap operations once at the beginning
3. Wait to know which is going to be the 1st test to be run
4. Run the first test and get its output (as log/CC)
5. Wait to know which is going to be the 2nd test to be run
6. Run the second test and get its output (as log/CC)

This is currently not feasible in PHPUnit 10, but the proposed solution at #4996 doesn't suite the goal neither: it doesn't make any sense to set all the PHPUnit stuffs for every test, and forcing users to adopt a workaround to only have their bootstrap executed once is just bad.

An idea would be to have sort of an API to easily call the bootstrapping operation and the test-run operation in two different steps, but further investigation is going to be done in the next sprint at the end of September.

[theseer]

For the record: Implementing/Addressing Issue #5059 is a step in addressing this.

[Slamdunk]

Hi @sebastianbergmann and @theseer, I see that PHPUnit 10 is scheduled to be released in less than e month, on 2023-02-03.

Tell me if I can be of any help to address this Issue on time ⏰

[sebastianbergmann]

We have cleaned up Application::run(). The actual running of tests is implemented in TestRunner::run() which needs a Configuration and a TestSuite. How to get these two objects can be seen in Application::run().

Let us know whether this allows you to use PHPUnit's test runner from ParaTest.

[theseer]

Just to stress some maybe obvious points:

• You are dealing with PHPUnit internals here that are subject to change in later, upcoming versions of PHPUnit. I'm sure that doesn't exactly surprise you ;)
• At this point, we have not decided whether or not to provide a public API for PHPUnit internals up to the level a tool such as ParaTest would require. Even if, it certainly won't make it into PHPUnit 10.0.
• We moved a lot of logic from the Application into separate components and methods, that should make it eas(y|ier) to extract the individual steps needed to have a working environment and hand you control of the test execution via TestRunner::run().

I believe we can close this issue as the problem should no longer exist. If you disagree - or have any other questions, please comment.

[Slamdunk]

A far as I can tell, in the same process I still cannot call \PHPUnit\TextUI\TestRunner::run multiple times, neither in two separate instances, because the \PHPUnit\Event\Facade can only be sealed once, can I?

The issue doesn't seem to be solved right now 😕

[sebastianbergmann]

Arne and I will look into this on Tuesday, stay tuned.

[theseer]

We believe to be finished with refactoring / extracting code from the TestRunner.

Please revisit and let us know if there are any problems left.

[sebastianbergmann]

Let me elaborate a bit:

The Application class is the entry point for PHPUnit's own CLI test runner. It is not meant to be (re)used by developers who want to wrap PHPUnit to build something such as ParaTest.

For the actual running of tests, Application uses TestRunner::run().

TestRunner::run() requires a Configuration, a ResultCache, and a TestSuite.

A Configuration can be built using Builder::build(). You need to pass $_SERVER['argv'] to this method. The method then parses CLI arguments/options and loads an XML configuration file, if one can be loaded.

A TestSuite can be built from a Configuration using TestSuiteBuilder::build().

While it is marked @internal, TestRunner is meant to be (re)used by developers who want to wrap PHPUnit's test runner.

As Arne already mentioned, we believe that this addresses your problem and makes multiple runs possible.