Phpunit: Make TestHook interfaces as easy to use as TestListener

[sebastianbergmann]

This issue is where the discussion that was started in #3381 should continue.

Related to #3388 and #3389.

[rpkamp]

A few thoughts:

1. The TestHook doesn't provide any functionality similar to TestListener::startTestSuite and TestListener::endTestSuite at the moment, but both TeamCity and JUnit use those and I don't see a way round it. So we probably need a BeforeTestSuiteHook and AfterTestSuiteHook?

2. In terms of BC it's probably easiest to implement all classes that currently use TestListener again using the hooks instead, so PHPUnit itself can use the new extensions, while userland can continue to extend the TestListeners, at least until PHPUnit 9 where the TestListener and everything that implements it will be removed. That does mean there is no more coverage of the old classes in the e2e tests. Not sure if that's a bad thing though, since PHPUnit itself isn't using them anyway?

3. The printerClass setting could remain intact, but we could check in the TestRunner if it's a TestListener or Extension, and register it accordingly. When it's a TestListener we could additionally print a deprecation message (in 8.0).

[rpkamp]

I just tried a quick proof of concept to make the TeamCity logger an extension and as it stands this is not possible.

First of all, the printResult method (which would become the executeAfterLastTest method) needs a TestResult, but doesn't get one. Adding this to the interface would be a BC break.

Then the addError (which would become the executeAfterFailedTest I believe) needs access to the actual Throwable to get a stack trace, but only receives a message string.

I stopped trying after that, but I'm sure there would be more issues in the same vain.

[epdenouden]

@rpkamp Thanks for your detailed feedback! I kept notes of what I ran into when refactoring the TestDox printer for #3380 and my list of required changes for the TestHook closely mirrors yours.

I will write up a more detailed response and proposal. The bottomline is the same as Remon says: the current printers/listeners get a lot more detail about the test run. The equivalent of startTest() and endTest() are just:

interface BeforeTestHook extends TestHook
{
    public function executeBeforeTest(string $test): void;
}

interface AfterSuccessfulTestHook extends TestHook
{
    public function executeAfterSuccessfulTest(string $test, float $time): void;
}

Reimplementing the current builtin output loggers requires (much) more information to be available.

[localheinz]

Just a quick heads-up: as an example, I have taken a look at johnkary/phpunit-speedtrap and tried to switch over from implementing the deprecated TestListener interface to implementing the (more or less) corresponding hooks.

Now I noticed that previously by implementing the TestListener it was possible to access the actual TestCase and invoke methods on it - in this particular case, annotations from the test methods are inspected which might provide information about individual thresholds.

When switching over to the hooks that appears not to be possible anymore (at least not in an easy way), as now the name of the test is passed in rather than the TestCase. That is, as long as it is easily possible to derive the test class and test method from the name of the test, that shouldn't be an issue, otherwise it would be a bit difficult to implement the behaviour using the the hooks.

What do you think? Can the name of the test be a reliable source for determining test class and method?

[sebastianbergmann]

There are many reasons why I want to replace the TestListener interface with the TestHook interfaces. Here are a few off the top of my head:

• TestListener has too many methods and violates the Interface Segregation Principle
• TestListener passes around the actual TestCase objects allowing not only read ("listen") access but also write access

The small TestHook interfaces allow for the addition of new hook capabilities without breaking backward compatibility (adding a new method to TestListener is a backward compatibility break).

Also, following the priciniple of minimal visibility, TestHook implementors currently only receive minimal information. If we find that more information is necessary then we can add it. Note that this addition of information may break backward compatibility and therefore would have to wait for the next major version of PHPUnit. Please note, though, that I do not want to pass TestCase around. Over the years I had too many confusing bug reports that were due to "listeners" messing with TestCase objects. A solution could be to create a read-only proxy for TestCase that we pass instead of the real TestCase object. But even that I would like to avoid.

[joesaunderson]

@sebastianbergmann if using a hook rather than a Listener, how can you get access to the Test class from the string?

[sebastianbergmann]

You do not. And you should not need to.

[ChrGriffin]

I've discovered this thread by Googling after PHPStorm informed me that PHPUnit\Framework\TestListener was deprecated. What's the story? Are these new 'hooks' in 8.2 or no? The documentation for 8.2 still talks a great deal about using TestListeners, despite them apparently being deprecated.
https://phpunit.readthedocs.io/en/8.2/extending-phpunit.html#implement-phpunit-framework-testlistener

[sebastianbergmann]

The TestHook interfaces have been around for a while. They are 1) not documented yet and 2) not easy (enough) to use yet.

[ChrGriffin]

I'm trying to understand them - mainly because I need to hook into the 'beforeFirstTest' event, as opposed to the 'startTestSuite' event (regardless of how many suites are run, I need to perform one action at the beginning, and one at the end), and the older listeners don't seem to have that functionality. From what I can find, they need to be added as a .phar extension, as opposed to loaded in via the phpunit.xml <listeners> element? Is that the extra difficulty?

[rpkamp]

No you can install them as extensions in phpunit.xml:

<extensions>
    <extension class="Namespace\To\Your\Listener" />
</extensions>

Just make sure your class implements PHPUnit\Runner\TestHook

[ChrGriffin]

Nice, thanks! Sorry for the somewhat off-topic divergence.

[karser]

I have a listener which does some stuff if the test suite contains a particular @group.

class MyListener implements TestListener {
    public function startTestSuite(TestSuite $suite): void
    {
        // check $suite->getGroups() and do some stuff
    }
}

How can this be converted to a TestHook based extension?
I tried to convert but the BeforeTestSuiteHook is missing.
Btw, @epdenouden we already discussed this last year

[rodnaph]

Adding a use-case...

I’m adding a custom annotation, which I use a TestListener before/after tests to read via $test->getAnnotations() - it would be nice to be able to do this (ie. have annotations available) with hooks instead.

[greg0ire]

This is a big BC-break for symfony/phpunit-bridge Most features implemented in https://github.com/symfony/symfony/blob/4.4/src/Symfony/Bridge/PhpUnit/Legacy/CoverageListenerTrait.php or https://github.com/symfony/symfony/blob/4.4/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php seem hard / impossible to migrate to the new API.

I found a way to get annotations, but I can't say it feels right:

use PHPUnit\Util\Test;

Test::parseTestMethodAnnotations(...explode('::', $test))

@karser, you can probably use something similar with Test::getGroups

But beware: * @internal This class is not covered by the backward compatibility promise for PHPUnit

[sebastianbergmann]

@greg0ire @nicolas-grekas Let us discuss symfony/phpunit-bridge in person at SymfonyCon Amsterdam's Hackday on November 23.

[epdenouden]

@greg0ire @nicolas-grekas Let us discuss symfony/phpunit-bridge in person at SymfonyCon Amsterdam's Hackday on November 23.

@greg0ire @nicolas-grekas @sebastianbergmann I'd be happy to join in, I'll bring _stroopwafels_

During the EU FOSSA hackathon we got a lot of work done on new event handling system for the loggers. I'd be happy to help out implementing the new event system to recreate the functionality you need for the bridge, and extend where needed.

[nicolas-grekas]

We need to lobby https://github.com/ManoManoTech so that @greg0ire can make it to SFCon :)

[greg0ire]

It's now official, I'm going! So whatever lobbying you did behind the scenes, it worked 👍

[gmazzap]

Today I spent one hour trying to find an acceptable solution for a simple scenario.

I have an "integration" suite that needs to initialize the lib I'm integrating with and a "unit" suite that only tests my lib. Initializing the integration is slow (DB involved), so I need to avoid doing it when not necessary.

As others have said, one big problem with hooks is there's none of them that listen to suite start/end.

But that's not all.

Oftentimes, especially during development, I run tests selectively with --filter and I need to know if what I'm filtering belongs to integration suite or not.

With current hooks status, that's not possible, and I think that a way to overcome this issue would be to make TestHook aware of the configuration object.

There would be no way hooks could _modify_ the configuration (array are passed by value) or do the nasty things that listeners could do receiving TestCase, but at the same time could provide enough context for the hook to decide if/how to behave.

In fact, for my use case what I've resorted to doing is to use BeforeFirstTestHook::executeBeforeFirstTest and there look at the command-line options (looking at $argv), searching for --filter and/or --testsuite options.
Thanks to the fact my integration tests classes live in a Tests\Integration namespace I can test for stripos($filter, 'integration') === false in the eventual filter.

Note that to parse options is not that easy because one needs to take into account both --filter Foo and --filter=Foo syntax.

If the hook object would be aware of the configuration object I could have written 3 lines of code instead of 25.

Maybe a ConfigAwareHook interface with a withRunnerConfigs(array $configs) could do the trick: for any Hook implementing it, right after instantiation, the runner could call that method passing the configuration, and it would be entirely backward compatible.

[Jean85]

I've opened a separate issue, but it's probably related to this one: #4131

Basically, I would like an alternative to --printer, a way to add this hooks without resorting to edit the configuration.

[trickeyone]

The TestHook interfaces have been around for a while. They are 1) not documented yet and 2) not easy (enough) to use yet.

@sebastianbergmann If they aren't easy enough to use, then should they not be marked @deprecated, no? It's not a breaking issue, of course, but it does muddy test runs with a bunch of unneeded deprecation warnings. I understand that these annotations have been around for a while now, as this thread is already almost 2 years old. Could we remove the @deprecated annotations until the TestHook implementations are made easier to integrate?

[sebastianbergmann]

Over the course of the last year, we learned that while the approach implemented with the TestHook interfaces is better than what we had before with the TestListener interface, it is still not good enough. @theseer and @localheinz are working on a new solution that will be integrated later this year. At that point in time, the TestHook interfaces and everything related to them will be deprecated and scheduled for removal.

I understand that it must be frustrating that we deprecated the old TestListener system, introduced the TestHook system which was not as easy-to-use as the system it was intended to replace, and now we are talking about deprecating the TestHook system as well as introducing yet another new system. I am truly sorry for this but I am certain that the event-based system @theseer and @localheinz are working on will make up for this.