Rector: [AddDoesNotPerformAssertionToNonAssertingTestRector] Infinite loop

Created on 11 Jan 2020  路  10Comments  路  Source: rectorphp/rector

| Subject | Details |
| :------------- | :----------------------------------------------------------- |
| Rector version | 0.6.x-dev@a21b805 |
| PHP version | 7.3.13 |
| Full Command | php rector.phar process -c var/rector-hang.yaml --ansi --dry-run -vvv tests/Test.php |

Current Behaviour

Given : 

# var/rector-hang.yaml
services:
    Rector\PHPUnit\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector: ~

And adding dump() calls at the beginning of each methods of AddDoesNotPerformAssertionToNonAssertingTestRector : 

^ "public function __construct()"
Rector 0.6.x-dev@a21b805
Config file: var/rector-hang.yaml

[parsing] tests/Test.php
[refactoring] tests/Test.php
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
^ "public function getNodeTypes(): array"
    [applying] Rector\PHPUnit\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector
^ "public function refactor(Node $node): ?Node"
^ "private function shouldSkipClassMethod(ClassMethod $classMethod): bool"

^ "private function containsAssertCall(ClassMethod $classMethod): bool"
^ "private function hasDirectAssertCall(ClassMethod $classMethod): bool"
^ "private function hasNestedAssertCall(ClassMethod $classMethod): bool"
^ "private function findClassMethod(Node $node): ?ClassMethod"
^ "private function findClassMethodByParsingReflection(Node $node): ?ClassMethod"
^ "private function findClassMethodInFile(string $fileName, string $methodName): ?ClassMethod"

^ "private function containsAssertCall(ClassMethod $classMethod): bool"
^ "private function hasDirectAssertCall(ClassMethod $classMethod): bool"
^ "private function hasNestedAssertCall(ClassMethod $classMethod): bool"
^ "private function findClassMethod(Node $node): ?ClassMethod"
^ "private function findClassMethodByParsingReflection(Node $node): ?ClassMethod"
^ "private function findClassMethod(Node $node): ?ClassMethod"
^ "private function findClassMethodByParsingReflection(Node $node): ?ClassMethod"
^ "private function findClassMethodInFile(string $fileName, string $methodName): ?ClassMethod"

^ "private function containsAssertCall(ClassMethod $classMethod): bool"
^ "private function hasDirectAssertCall(ClassMethod $classMethod): bool"
^ "private function hasNestedAssertCall(ClassMethod $classMethod): bool"
^ "private function findClassMethod(Node $node): ?ClassMethod"
^ "private function findClassMethodByParsingReflection(Node $node): ?ClassMethod"
^ "private function findClassMethodInFile(string $fileName, string $methodName): ?ClassMethod"

^ "private function containsAssertCall(ClassMethod $classMethod): bool"
^ "private function hasDirectAssertCall(ClassMethod $classMethod): bool"
^ "private function hasNestedAssertCall(ClassMethod $classMethod): bool"
^ "private function findClassMethod(Node $node): ?ClassMethod"

^ "private function containsAssertCall(ClassMethod $classMethod): bool"
^ "private function hasDirectAssertCall(ClassMethod $classMethod): bool"
^ "private function hasNestedAssertCall(ClassMethod $classMethod): bool"
^ "private function findClassMethod(Node $node): ?ClassMethod"
^ "private function findClassMethodByParsingReflection(Node $node): ?ClassMethod"
^ "private function findClassMethodInFile(string $fileName, string $methodName): ?ClassMethod"

^ "private function containsAssertCall(ClassMethod $classMethod): bool"
^ "private function hasDirectAssertCall(ClassMethod $classMethod): bool"
^ "private function hasNestedAssertCall(ClassMethod $classMethod): bool"
^ "private function findClassMethod(Node $node): ?ClassMethod"
^ "private function findClassMethodByParsingReflection(Node $node): ?ClassMethod"
^ "private function findClassMethodInFile(string $fileName, string $methodName): ?ClassMethod"

^ "private function containsAssertCall(ClassMethod $classMethod): bool"
^ "private function hasDirectAssertCall(ClassMethod $classMethod): bool"
^ "private function hasNestedAssertCall(ClassMethod $classMethod): bool"
^ "private function findClassMethod(Node $node): ?ClassMethod"

^ "private function containsAssertCall(ClassMethod $classMethod): bool"
^ "private function hasDirectAssertCall(ClassMethod $classMethod): bool"
^ "private function hasNestedAssertCall(ClassMethod $classMethod): bool"
^ "private function findClassMethod(Node $node): ?ClassMethod"
^ "private function containsAssertCall(ClassMethod $classMethod): bool"
^ "private function findClassMethod(Node $node): ?ClassMethod"
^ "private function findClassMethodByParsingReflection(Node $node): ?ClassMethod"
^ "private function findClassMethodInFile(string $fileName, string $methodName): ?ClassMethod"

^ "private function containsAssertCall(ClassMethod $classMethod): bool"
^ "private function hasDirectAssertCall(ClassMethod $classMethod): bool"
^ "private function hasNestedAssertCall(ClassMethod $classMethod): bool"
^ "private function findClassMethod(Node $node): ?ClassMethod"

^ "private function containsAssertCall(ClassMethod $classMethod): bool"
^ "private function hasDirectAssertCall(ClassMethod $classMethod): bool"
^ "private function hasNestedAssertCall(ClassMethod $classMethod): bool"
^ "private function findClassMethod(Node $node): ?ClassMethod"
^ "private function findClassMethodByParsingReflection(Node $node): ?ClassMethod"
^ "private function findClassMethodInFile(string $fileName, string $methodName): ?ClassMethod"

^ "private function containsAssertCall(ClassMethod $classMethod): bool"
^ "private function hasDirectAssertCall(ClassMethod $classMethod): bool"
^ "private function hasNestedAssertCall(ClassMethod $classMethod): bool"
^ "private function findClassMethod(Node $node): ?ClassMethod"
^ "private function findClassMethodByParsingReflection(Node $node): ?ClassMethod"
^ "private function findClassMethodInFile(string $fileName, string $methodName): ?ClassMethod"

etc...

Minimal PHP Code Causing Issue

I managed to reduce the file to : 

<?php

declare(strict_types=1);

namespace App;

use App\SomeClass;
use SomeVendor\Uuid\Uuid;
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;

final class Test extends KernelTestCase
{
    public function testSomething(): void
    {
        $someClass = SomeClass::create(Uuid::create(), Uuid::create(), 'test');
        $someClass->send(Uuid::create(), 'test', Uuid::create());
    }
}

Unfortunately, I can't share the code of SomeClass publicly and did not manage to reproduce in my playground. Yet, I thought it was worth creating the issue, as you might now anyway why/how an infinite loop could happen in such a code, or have tips how I could debug it ?

Seems like containsAssertCall -> hasDirectAssertCall -> hasNestedAssertCall is the trigger loop.

I tried to add the annotation before running Rector too, but it didn't change a thing (which is really weird IMHO : why would this Rector still loop if the annotation already exists ? :thinking: ) : 

final class Test extends KernelTestCase
{
    /**
     * @doesNotPerformAssertion
     */
    public function testSomething(): void
    {
        $someClass = SomeClass::create(Uuid::create(), Uuid::create(), 'test');
        $someClass->send(Uuid::create(), 'test', Uuid::create());
    }
}

Expected Behaviour

Rector dev-master@a21b805
Config file: rector.yaml

[parsing] src/SomeClass.php
[parsing] vendor/some-vendor/Uuid/Uuid.php
[parsing] tests/Test.php
[refactoring] src/SomeClass.php
[refactoring] vendor/some-vendor/Uuid/Uuid.php
[refactoring] tests/Test.php
[printing] src/SomeClass.php
[printing] vendor/some-vendor/Uuid/Uuid.php
[printing] tests/Test.php

1 file with changes
===================

1) tests/Test.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -8,6 +8,9 @@

 final class Test extends KernelTestCase
 {
+    /**
+     * @doesNotPerformAssertion
+     */
     public function testSomething(): void
     {
         $someClass = SomeClass::create(Uuid::create(), Uuid::create(), 'test');
    ----------- end diff -----------

Applied rules:

 * Rector\PHPUnit\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector

 [OK] Rector is done! 1 file would have changed (dry-run).
bug
Source
picture gnutix

All 10 comments

Seems like containsAssertCall -> hasDirectAssertCall -> hasNestedAssertCall is the trigger loop.

Maybe we could check for assert in method name and break the loop with true if found.
What do you think?

TomasVotruba picture TomasVotruba on 11 Jan 2020

Just noticed the PR, good job :+1:

TomasVotruba picture TomasVotruba on 11 Jan 2020

The PR doesn't really fix the fact that this Rector goes into an infinite loop and does not add the annotation if it's not there (I added them manually in my codebase and the fix prevent the infinite loop when it's there already).

gnutix picture gnutix on 11 Jan 2020

I can handle it, if you add failing test case

TomasVotruba picture TomasVotruba on 11 Jan 2020

Easier said than done! Already my third attempt to anonymize our code and yet keep the bug in there... (it's only happening on critical domain code, which I can't share)

I'm wondering if this could have to do with the fact that we use Prophecy ? ("mockery/mockery": "^0.9.5") As this Rector been developed with other testing framework than PHPUnit in mind ?

Should it detect this example below as an assertion ? 

<?php
$repository = $this->prophesize(SomeEntityRepositoryInterface::class);
$repository
    ->save($someEntity)
    ->shouldBeCalledTimes(1); // This here is a Prophecy assertion
gnutix picture gnutix on 11 Jan 2020

Easier said than done! Already my third attempt to anonymize our code and yet keep the bug in there... (it's only happening on critical domain code, which I can't share)

You never know until it's done :).

Personally I'd try to replicate in new fixture file, instead of rewriting your code base to pseudo code. Something like:

<?php

class SomeTest extends TestCase
{
    public testNoAssertion()
    {
        $this->go();
    }

    private function go()
    {
        $this->containsAssertCall();
    }

    private function containsAssertCall()
    {
        $this->go();
    }
}

And add complexity untill it breaks on the loop.

I'm wondering if this could have to do with the fact that we use Prophecy ? ("mockery/mockery": "^0.9.5") As this Rector been developed with other testing framework than PHPUnit in mind ?

Sure, whatever real case this rule should cover.

TomasVotruba picture TomasVotruba on 11 Jan 2020

Bad luck. I tried implementing a Prophecy oriented test but it did not trigger the infinite loop...

And add complexity untill it breaks on the loop.

The thing is, there's a big gap between "a simple test" (as above) and our real test cases (using an event bus, CQRS, etc...). I can't really add all these layers of complexity one by one (I don't even know them all that well since I've only been working on this codebase for a few weeks), even more so in an anomized fashion...

So until someone reports another simpler piece of code triggering that infinite loop, this will have to be closed.

gnutix picture gnutix on 11 Jan 2020

So until someone reports another simpler piece of code triggering that infinite loop, this will have to be closed.

In that case, I'd just skip this rule in that specific method/file (whatever is better context):
https://github.com/rectorphp/rector#exclude-paths-and-rectors

TomasVotruba picture TomasVotruba on 11 Jan 2020

Yup, I've added it to exclude_rectors (too many occurrences to go with @noRector).

gnutix picture gnutix on 11 Jan 2020
👍1

@TomasVotruba It seems that https://github.com/rectorphp/rector/commit/0ea5e8df5be3eec03ee65d74294378d9c9bb6282 fixed the infinite loop ! I was able to remove the Rector from exclude_rectors and run it on the whole codebase without issues.

gnutix picture gnutix on 12 Jan 2020
🎉1
Was this page helpful?
0 / 5 - 0 ratings

Related issues

dehrk picture dehrk  路  4Comments
localheinz picture localheinz  路  3Comments
pavarnos picture pavarnos  路  4Comments
peterrehm picture peterrehm  路  4Comments
powtac picture powtac  路  4Comments