Thoughts on removing TestListener to TestHook rule?

[ccovey]

There is a rule provided in phpunit90 rule set that converts TestListener usages to TestHook usages. This was done because phpunit deprecated TestListener and provided somewhat similar functionality under their TestHook system. This has a couple gotchas where I think maybe removing this from the rule set may be a good idea?

First is the TestListener service provides wildly different functionality to the TestHook system, For example TestListener provides the actual test object whereas the TestHook only provides a string representation. Unfortunately this means that there is a large part of functionality available that a simple method signature change can't ever address.

I think the more important issue though is that TestHook is not a system they recommend to use nor is it actually staying long term, a new event system is under development for phpunit 10. You can see some of the discussion around that here sebastianbergmann/phpunit#3390.

The main reason I suggest this change is that its a potentially breaking change rule since the interface isn't at all the same but its also not even a system that will be in use going forward. I can make a PR to do this but wanted to discuss first what your thoughts were on this?

Just for reference the rule I"m talking about.
https://github.com/rectorphp/rector-phpunit/blob/main/config/sets/phpunit90.php#L13

[TomasVotruba]

Hi @ccovey ,

thanks for the proposal. I personally don't use these listeners much, but I recall I decompose it to hook as soon as possible.
Removing the rule from set would kill the point of making upgrade sets.

If there is a specific bug, it should be backed by fixture in the tests and fixed. E.g. by skipping the edge case you talk about.
Could you add such failing fixture to the rule test itself? It would help to have this case covered and not break anything :)

[ccovey]

I'll try to look at getting you a failing test but for now I'll just try to describe the issue. below is from our actual listener, we get the object under test and reset all the properties after a test has run. Note that the Test object is passed in and we are able to manipulate it.

public function endTest(Test $test, float $time) : void
{
    $refl = new ReflectionObject($test);
    foreach ($refl->getProperties() as $prop) {
        if (!$prop->isStatic() && strpos($prop->getDeclaringClass()->getName(), 'PHPUnit_') !== 0) {
            $prop->setAccessible(true);
            $prop->setValue($test, null);
        }
    }
}

when converting to a hook we see public function executeAfterTest(string $test, float $time): void; as the new signature and only a string representation of a test is passed in not the object itself. I don't have thoughts on how to determine whether a string is good enough or not?

The reason I suggested not adding it to the upgrade set is because phpunit doesn't actually recommend you use them and also plans on deleting the TestHooks along with the TestListeners.

Sorry I can't get you a test case atm, in the middle of work day and just came across this. I'll try and get you something tonight.

Thanks!

[TomasVotruba]

Closing for lack of momentum here. Feel free to send a PR with rule proposal, we can push it from there together 👍