Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CodeQuality] Skip call parent::__construct() indirect parent on ConstructClassMethodToSetUpTestCaseRector #251

Merged
merged 20 commits into from
Sep 10, 2023

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Sep 10, 2023

Continue of PR:

this ensure when there is parent::__construct() call on indirect parent, it should be skipped as indirect use.

@samsonasik
Copy link
Member Author

Init early should also be skipped:

final class SkipParameterUsed extends TestCase
{
    public function __construct($param)
    {
        $this->initEarly($param);
    }

    private function initEarly($param)
    {
        echo 'init';
    }
}

It currently cause invalid result:

 final class SkipParameterUsed extends TestCase
 {
-    public function __construct($param)
+    protected function setUp()
     {
         $this->initEarly($param);

which param removed but used in the caller.

@@ -8,6 +8,7 @@
<testsuites>
<testsuite name="main">
<directory>tests</directory>
<directory>rules-tests</directory>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomasVotruba here rules-tests need to be registered to show the PHPUnit error, it was never executed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, that's a big miss from my side 🤦 😅

@samsonasik
Copy link
Member Author

15 errors because rules-tests was never executed #251 (review)

Run vendor/bin/phpunit
  vendor/bin/phpunit
  shell: /usr/bin/bash -e {0}
  env:
    COMPOSER_ROOT_VERSION: dev-main
    COMPOSER_PROCESS_TIMEOUT: 0
    COMPOSER_NO_INTERACTION: 1
    COMPOSER_NO_AUDIT: 1
PHPUnit 10.3.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.23
Configuration: /home/runner/work/rector-phpunit/rector-phpunit/phpunit.xml

..................FFF.......F.F.........FF.FF..................  63 / 195 ( 32%)
...............F............F.................................. 126 / 195 ( 64%)
F..........................................FFF................. 189 / 195 ( 96%)
......                                                          195 / 195 (100%)

Time: 00:03.898, Memory: 146.50 MB

There were 4 PHPUnit test runner warnings:

1) No tests found in class "Rector\PHPUnit\Tests\AnnotationsToAttributes\Rector\ClassMethod\DependsAnnotationWithValueToAttributeRector\Source\AnotherTest".

2) Class DifferentNamespaceTest cannot be found in /home/runner/work/rector-phpunit/rector-phpunit/rules-tests/CodeQuality/Rector/Class_/AddSeeTestAnnotationRector/Source/DifferentNamespaceTest.php

3) Class SkipSimpleClassCommentTest cannot be found in /home/runner/work/rector-phpunit/rector-phpunit/rules-tests/CodeQuality/Rector/Class_/AddSeeTestAnnotationRector/Source/SkipSimpleClassCommentTest.php

4) Class SomeExistingTest cannot be found in /home/runner/work/rector-phpunit/rector-phpunit/rules-tests/CodeQuality/Rector/Class_/AddSeeTestAnnotationRector/Source/SomeExistingTest.php

--

There were 15 failures:

1) Rector\PHPUnit\Tests\AnnotationsToAttributes\Rector\ClassMethod\TestWithAnnotationToAttributeRector\TestWithAnnotationToAttributeRectorTest::test with data set #0
Failed on fixture file "some_fixture.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 final class SomeFixture extends TestCase
 {
-    #[\PHPUnit\Framework\Attributes\TestWith(['foo'])]
-    #[\PHPUnit\Framework\Attributes\TestWith(['bar'])]
+    /**
+     * @testWith ["foo"]
+     *           ["bar"]
+     */
     public function testOne(): void
     {
     }

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/AnnotationsToAttributes/Rector/ClassMethod/TestWithAnnotationToAttributeRector/TestWithAnnotationToAttributeRectorTest.php:16

2) Rector\PHPUnit\Tests\AnnotationsToAttributes\Rector\ClassMethod\TestWithAnnotationToAttributeRector\TestWithAnnotationToAttributeRectorTest::test with data set #1
Failed on fixture file "some_lower_cased.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 final class SomeLowerCased extends TestCase
 {
-    #[\PHPUnit\Framework\Attributes\TestWith(['rum'])]
-    #[\PHPUnit\Framework\Attributes\TestWith(['fim'])]
+    /**
+     * @testwith ["rum"]
+     * @testwith    ["fim"]
+     */
     public function testOne(): void
     {
     }

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/AnnotationsToAttributes/Rector/ClassMethod/TestWithAnnotationToAttributeRector/TestWithAnnotationToAttributeRectorTest.php:16

3) Rector\PHPUnit\Tests\AnnotationsToAttributes\Rector\ClassMethod\TestWithAnnotationToAttributeRector\TestWithAnnotationToAttributeRectorTest::test with data set #2
Failed on fixture file "the_most_complex_json.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 final class TheMostComplexJson extends TestCase
 {
-    #[\PHPUnit\Framework\Attributes\TestWith([['day' => 'monday', 'conditions' => 'sunny'], ['day', 'conditions']])]
+    /**
+     * @testWith [{"day": "monday", "conditions": "sunny"}, ["day", "conditions"]]
+     */
      public function testOne(): void
      {
      }

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/AnnotationsToAttributes/Rector/ClassMethod/TestWithAnnotationToAttributeRector/TestWithAnnotationToAttributeRectorTest.php:16

4) Rector\PHPUnit\Tests\CodeQuality\Rector\ClassMethod\RemoveEmptyTestMethodRector\RemoveEmptyTestMethodRectorTest::test with data set #0
Failed on fixture file "fixture.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 class SomeTest extends \PHPUnit\Framework\TestCase
 {
+    /**
+     * testGetTranslatedModelField method
+     *
+     * @return void
+     */
+    public function testGetTranslatedModelField()
+    {
+    }
 }
 
 ?>

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/CodeQuality/Rector/ClassMethod/RemoveEmptyTestMethodRector/RemoveEmptyTestMethodRectorTest.php:16

5) Rector\PHPUnit\Tests\CodeQuality\Rector\ClassMethod\ReplaceTestAnnotationWithPrefixedFunctionRector\ReplaceTestAnnotationWithPrefixedFunctionRectorTest::test with data set #1
Failed on fixture file "skip_already_prefixed_function.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 class SkipAlreadyPrefixedFunction extends \PHPUnit\Framework\TestCase
 {
-    /**
-     * @test
-     */
-    public function testOnePlusOneShouldBeTwo()
+    public function testTestOnePlusOneShouldBeTwo()
     {
         $this->assertSame(2, 1+1);
     }

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/CodeQuality/Rector/ClassMethod/ReplaceTestAnnotationWithPrefixedFunctionRector/ReplaceTestAnnotationWithPrefixedFunctionRectorTest.php:16

6) Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\ConstructClassMethodToSetUpTestCaseRector\ConstructClassMethodToSetUpTestCaseRectorTest::test with data set #2
Failed on fixture file "skip_call_parent_construct.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 {
     private $someValue;
 
-    public function __construct()
+    protected function setUp()
     {
         $this->someValue = 1000;
-
-        parent::__construct();
     }
 }

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/CodeQuality/Rector/Class_/ConstructClassMethodToSetUpTestCaseRector/ConstructClassMethodToSetUpTestCaseRectorTest.php:16

7) Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\ConstructClassMethodToSetUpTestCaseRector\ConstructClassMethodToSetUpTestCaseRectorTest::test with data set #3
Failed on fixture file "skip_parameter_used.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 final class SkipParameterUsed extends TestCase
 {
-    public function __construct($param)
+    protected function setUp()
     {
         $this->initEarly($param);
     }

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/CodeQuality/Rector/Class_/ConstructClassMethodToSetUpTestCaseRector/ConstructClassMethodToSetUpTestCaseRectorTest.php:16

8) Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\PreferPHPUnitThisCallRector\PreferPHPUnitThisCallRectorTest::test with data set #0
Failed on fixture file "replace_none_static_skip_static_function.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 {
     public function testMe()
     {
-        $this->assertSame(1, 1);
+        self::assertSame(1, 1);
     }
 
     public static function testMe2()

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitThisCallRector/PreferPHPUnitThisCallRectorTest.php:16

9) Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\PreferPHPUnitThisCallRector\PreferPHPUnitThisCallRectorTest::test with data set #1
Failed on fixture file "short_classes.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 {
     public function testMe()
     {
-        $this->assertSame(1, 1);
+        self::assertSame(1, 1);
     }
 }

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitThisCallRector/PreferPHPUnitThisCallRectorTest.php:16

10) Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\AssertIssetToSpecificMethodRector\AssertIssetToSpecificMethodRectorTest::test with data set #3
Failed on fixture file "skip_if_magic_method_isset_exists_in_parent.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
     public function test()
     {
         $foo = new \Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\AssertIssetToSpecificMethodRector\Fixture\CustomIsset2();
-        $this->assertObjectHasAttribute('bar', $foo);
+        $this->assertTrue(isset($foo->bar));
     }
 }

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/CodeQuality/Rector/MethodCall/AssertIssetToSpecificMethodRector/AssertIssetToSpecificMethodRectorTest.php:16

11) Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\AssertTrueFalseToSpecificMethodRector\AssertTrueFalseToSpecificMethodRectorTest::test with data set #3
Failed on fixture file "fixture.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
         $this->assertContains('...', ['...'], 'argument');
 
         $this->assertNotIsReadable('...');
-        $this->assertEmpty('...');
+        $this->assertTrue(empty('...'));
         $this->assertFileNotExists('...');
         $this->assertDirectoryExists('...');
         $this->assertFinite('...');

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/CodeQuality/Rector/MethodCall/AssertTrueFalseToSpecificMethodRector/AssertTrueFalseToSpecificMethodRectorTest.php:16

12) Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\AddDoesNotPerformAssertionToNonAssertingTestRectorTest::test with data set #0
Failed on fixture file "fixture.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 class SomeClass extends \PHPUnit\Framework\TestCase
 {
-    /**
-     * @doesNotPerformAssertions
-     */
     public function test()
     {
         $nothing = 5;

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/AddDoesNotPerformAssertionToNonAssertingTestRectorTest.php:16

13) Rector\PHPUnit\Tests\PHPUnit70\Rector\Class_\RemoveDataProviderTestPrefixRector\RemoveDataProviderTestPrefixRectorTest::test with data set #0
Failed on fixture file "fixture.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
         $nothing = 5;
     }
 
-    public function provideData()
+    public function testProvideData()
     {
         return ['123'];
     }

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/PHPUnit70/Rector/Class_/RemoveDataProviderTestPrefixRector/RemoveDataProviderTestPrefixRectorTest.php:16

14) Rector\PHPUnit\Tests\PHPUnit70\Rector\Class_\RemoveDataProviderTestPrefixRector\RemoveDataProviderTestPrefixRectorTest::test with data set #1
Failed on fixture file "multiple_data_providers.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
         $nothing = 5;
     }
 
-    public function provideData()
+    public function testProvideData()
     {
         return ['123'];
     }
 
-    public function nextProvideData2()
+    public function testNextProvideData2()
     {
         return ['123'];
     }
 
-    public function nextProvideData()
+    public function testNextProvideData()
     {
         return ['123'];
     }

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/PHPUnit70/Rector/Class_/RemoveDataProviderTestPrefixRector/RemoveDataProviderTestPrefixRectorTest.php:16

15) Rector\PHPUnit\Tests\PHPUnit70\Rector\Class_\RemoveDataProviderTestPrefixRector\RemoveDataProviderTestPrefixRectorTest::test with data set #2
Failed on fixture file "with_test_annotation.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
         $nothing = 5;
     }
 
-    public function provideDataForWithATestAnnotation()
+    public function testProvideDataForWithATestAnnotation()
     {
         return ['123'];
     }

/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:245
/home/runner/work/rector-phpunit/rector-phpunit/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:169
/home/runner/work/rector-phpunit/rector-phpunit/rules-tests/PHPUnit70/Rector/Class_/RemoveDataProviderTestPrefixRector/RemoveDataProviderTestPrefixRectorTest.php:16

FAILURES!
Tests: 195, Assertions: 261, Failures: 15, Warnings: 4.

@samsonasik
Copy link
Member Author

@TomasVotruba @staabm the errors seems due to remove regex support:

which make the following code no longer works:

if (! $this->isName($node->name, 'assert*')) {
return null;
}

The error detected after rules-tests registered to phpunit.xml above

#251 (review)

@@ -71,7 +72,7 @@ public function refactor(Node $node): ?Node
return null;
}

if ($this->isName($node->name, 'test*')) {
if (fnmatch('test*', $node->name->toString(), FNM_NOESCAPE)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomasVotruba @staabm using fnmatch() directly seems the solution

Comment on lines +179 to +183
] as $methodCallNamePattern) {
if (fnmatch($methodCallNamePattern, $callname, FNM_NOESCAPE)) {
return true;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomasVotruba @staabm here the tweak fnmatch in loop :)

@samsonasik
Copy link
Member Author

Fixed 🎉

@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba I am merging it to have faster feedback to test ;)

@samsonasik samsonasik merged commit fde1cef into main Sep 10, 2023
5 checks passed
@samsonasik samsonasik deleted the skip-parent-construct branch September 10, 2023 03:12
@samsonasik samsonasik changed the title [CodeQuality] Skip call parent::__construct() on ConstructClassMethodToSetUpTestCaseRector [CodeQuality] Skip call parent::__construct() indirect parent on ConstructClassMethodToSetUpTestCaseRector Sep 10, 2023
private function resolveFirstArgument(FuncCall|Empty_ $firstArgumentValue): ?string
{
return $firstArgumentValue instanceof Empty_
? 'empty'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomasVotruba this is due to removal of EmptyNameResolver PR:

detected after rules-tests registered to phpunit.xml

#251 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants