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.

[malarzm]

Is there any place one can take a look at the new event system or is it not drafted/published yet? Tried to search PRs but couldn't find anything.

[rpkamp]

I think it's this one: theseer#1

[dbu]

as TestListener is marked as deprecated, i am trying to migrate to a TestHook setup at FOSHttpCache.

we used to have a listener that did only something in startTestSuite. the new segregated interfaces are nice, we don't have to implement 10 empty methods anymore 👍 . but we use a group on the tests to know if we need to start a server or not

        if (!in_array('webserver', $suite->getGroups())) {
            return;
        }

        $this->bootWebServer();

how can i convert this to the new hook system?

[trickeyone]

as TestListener is marked as deprecated, i am trying to migrate to a TestHook setup at FOSHttpCache.

we used to have a listener that did only something in startTestSuite. the new segregated interfaces are nice, we don't have to implement 10 empty methods anymore 👍 . but we use a group on the tests to know if we need to start a server or not

        if (!in_array('webserver', $suite->getGroups())) {
            return;
        }

        $this->bootWebServer();

how can i convert this to the new hook system?

@dbu Per @sebastianbergmann comment above, it looks like the hook system will be removed. The deprecation warnings can be ignored.

[sebastianbergmann]

Correct, PHPUnit 10 will no longer support the Hook interface(s).

[dbu]

oh, ok. would it make sense to remove the deprecation on the listeners then? and deprecate the hooks again?

[theseer]

Actually, as PHPUnit 10 will come with a new Event based subsystem, both of the old mechanisms are gone.

[dbu]

is the event subsystem available in phpunit 9 already? if not, deprecating is not helping much as users don't have an alternative.

if it is, could we upgrade the listener deprecation in phpunit 8 and 9 to point to that new event system? i started adapting the hook system because thats what is referenced in the deprecation.

to avoid misunderstandings: thanks for providing this high-quality tool! i don't want to just complain, but would be happy to do a PR to change the deprecations to point to the correct upgrade path, if that helps.

[theseer]

The Event Subsystem is one of the major new things of PHPUnit 10 and thus not available in PHPUnit 9 or older. I unfortunately do not see any feasible way to "backport" that to PHPUnit 9 or even older, so it will be PHPUnit 10 and up only.

Luckily, deprecating something doesn't mean it will stop working. Granted, if you want to have something that works for PHPUnit 10 and later it will be different from PHPUnit 9 and before. But assuming your code already works for PHPUnit 8 and 9, I don't see how that would be a problem now if you want to build a new Version for PHPUnit 10 and up?

That being said: "Fixing" deprecation warnings to point to the correct replacement though seems like generally a good idea :)

[dbu]

i would be happy to do a pull request to change the deprecations to point to the new system. is there any place i can link to? on readthedocs, there seems no doc version for the upcoming version...

[theseer]

I don't think we have a documentation as of yet, given the stuff is under construction and there is no "official" API to register your own things yet.

The best place to refer to probably is #4676.

[sebastianbergmann]

The Event Subsystem is one of the major new things of PHPUnit 10 and thus not available in PHPUnit 9 or older. I unfortunately do not see any feasible way to "backport" that to PHPUnit 9 or even older, so it will be PHPUnit 10 and up only.

The event system was a catalyst for a lot of changes: Finding the right places to emit the appropriate events has revealed countless previously hidden inconsistencies and problems. For the TestRunner\Configured event, for instance, we needed a canonical as well as immutable representation of the configuration. The code that loads the XML configuration is better now. The code that processes the CLI arguments is better now. The code that combines these two configuration sources into "the configuration" is better now. In fact: "the configuration" only now exists in a meaningful form. Once we had "the configuration", we realised that we could now implement large parts of the CLI test runner using less and simpler code.

The event system requires all of these changes to work, but backporting these changes to PHPUnit 9, for instance, is just not feasible.

[top-master]

See withContext(...) feature request

In our project's testRoutes_shouldDenyCustomersAccessToAdminPages() test, we have a for-loop that iterates each and every "admin" prefixed route of our PHP App.

Now sometimes an error is thrown, or the assertion fails, and we would like to know for which route exactly, for that, said for-loop stores the context in lastRoute instance-variable of test-case, and TestListener prefixes the logged message with said context, like:

public function addError(Test $test, \Throwable $t, float $time): void
{
    $this->onThrow($test, $t);
}
public function addFailure(Test $test, AssertionFailedError $error, float $time): void
{
    $this->onThrow($test, $error);
}

private function onThrow(Test $test, \Throwable $error)
{
    if ($test instanceof MyTest) {
        if ($test->lastRoute !== null) {
            self::prefixMessage($error, 'While testing route = ' . $test->lastRoute . ":\n");
        }
    }
}

private static function prefixMessage($obj, $prefix)
{
    try {
        $reflection = new \ReflectionClass($obj);
        $property = $reflection->getProperty('message');
        $property->setAccessible(true);
        $property->setValue($obj, $prefix . $property->getValue($obj));
    } catch (\ReflectionException $e) {
    }
}

---

@sebastianbergmann How can we achive above with the new TestHook API?
Which looks like:

interface AfterTestErrorHook extends TestHook
{
    public function executeAfterTestError(string $test, string $message, float $time): void;
}

interface AfterTestFailureHook extends TestHook
{
    public function executeAfterTestFailure(string $test, string $message, float $time): void;
}

I mean, we could Make the lastRoute a static-variable (instead of instance-variable).
But how can we alter the $message?

Solutions

As you deprecated our side of handling this,
please consider implementing one of below solutions into PHPUnit API.

# 1 You could like some other frameworks, provide withContext(...) method, and the next failure (no matter if error or assert) should log the context before actual message, then clear context.

Also, test-runner should clear context after each successful test-method (without logging of course).

# 2 Or, you could change your existing API's $message to &$message, I mean, to a reference, then use reflection like above (to update message).

[rpkamp]

In our project's testRoutes_shouldDenyCustomersAccessToAdminPages() test, we have a for-loop that iterates each and every "admin" prefixed route of our PHP App.

Maybe I'm being terse, but why not use a dataprovider? It'll get you loop and output which of the loops went wrong.

[top-master]

@rpkamp

First of all, that would make the message look something like:

1) Tests\MyTest::testRoutes_shouldDenyCustomersAccessToAdminPages with data set #3 ('/admin/users/edit')
Failed asserting that ...

Instead of:

1) Tests\MyTest::testRoutes_shouldDenyCustomersAccessToAdminPages
While testing route = /admin/users/edit:
Failed asserting that ...

And someone later running the tests would get confused (as we don't write tests for ourselves).

Secondly, storing each and every route in array is not always possible, for example:

• If each route (or data-set) requires some additional data.
• The data-provider would need to pass (i.e. return) those, too.
• Which all go into our logs, making the already confusing data-provider-log even more unreadable.

Last and least, rewritting logic to create array with specific structure, instead of simply iterating would take time.

[Jean85]

The first concern is addressable using array keys. Array keys in data providers are used an names of the entries, so:

return [
    'route /admin/users/edit' => [...],
];

would generate an error like:

1) Tests\MyTest::testRoutes_shouldDenyCustomersAccessToAdminPages with 'route /admin/users/edit' ('/admin/users/edit')
Failed asserting that ...