Skip to content

[Rector] add PHPUnitSetList::PHPUNIT_SPECIFIC_METHOD #5320

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

Merged
merged 4 commits into from
Nov 15, 2021

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 14, 2021

Description

See https://github.com/rectorphp/rector-phpunit#rector-rules-for-phpunit

Checklist:

  • Securely signed commits
  • [] Component(s) with PHPDoc blocks, only if necessary or adds value
  • [] Unit testing, with >80% coverage
  • [] User guide updated
  • [] Conforms to style guide

@samsonasik
Copy link
Member

samsonasik commented Nov 14, 2021

@kenjis could you run:

vendor/bin/rector

and commit the change?

@kenjis kenjis force-pushed the add-rectore-phpunit-rule branch from 1ebe1e9 to daeaeb3 Compare November 15, 2021 00:08
@kenjis
Copy link
Member Author

kenjis commented Nov 15, 2021

@samsonasik I committed.

I also added PHPUnitSetList::PHPUNIT_80,
PHPUnitSetList::PHPUNIT_90,
PHPUnitSetList::PHPUNIT_91.

@samsonasik
Copy link
Member

@kenjis I think we can add setlist one by one, as it cause error :

Error: ] Could not process "system/Cookie/CloneableCookieInterface.php" file,   
         due to:                                                                
         "Argument 1 passed to Rector\Core\Rector\AbstractRector::isObjectType()
         must implement interface PhpParser\Node, null given, called in         
         vendor/rector/rector/rules/TypeDeclaration/Rector/ClassMethod/AddParamT
         ypeDeclarationRector.php:83". On line: 315        

first, ensure PHPUnitSetList::PHPUNIT_SPECIFIC_METHOD working first, and we can add more set in separate PR.

If we have specific rule that failed, eg: AddParamTypeDeclarationRector above, we can skip first under Option::SKIP on specific file.

@kenjis kenjis force-pushed the add-rectore-phpunit-rule branch from 45922a4 to daeaeb3 Compare November 15, 2021 00:55
@kenjis
Copy link
Member Author

kenjis commented Nov 15, 2021

@samsonasik Okay, I will do one by one. Thanks!


$attributes = $this->getPrivateProperty($entity, 'attributes');
$this->assertFalse(isset($attributes['foo']));
$this->assertTrue(isset($attributes['default']));
$this->assertArrayNotHasKey('foo', $attributes);
Copy link
Member Author

Choose a reason for hiding this comment

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

This assertion fails. Because $attributes is

array (4) [
    'foo' => null
    'bar' => null
    'default' => string (6) "sumfin"
    'created_at' => null
]

What do we do?

Copy link
Member

@samsonasik samsonasik Nov 15, 2021

Choose a reason for hiding this comment

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

You can register Rector\PHPUnit\Rector\MethodCall\AssertIssetToSpecificMethodRector into Option::SKIP

use Rector\PHPUnit\Rector\MethodCall\AssertIssetToSpecificMethodRector;

    $parameters->set(Option::SKIP, [
             // ....

            AssertIssetToSpecificMethodRector::class => [
                     __DIR__ . '/tests/system/Entity/EntityTest.php',
           ],
    ]);

and then rollback change for tests/system/Entity/EntityTest.php

@@ -908,10 +908,10 @@ public function testIssetKeyMap()
$entity = $this->getEntity();

$entity->created_at = '12345678';
$this->assertTrue(isset($entity->createdAt));
$this->assertObjectHasAttribute('createdAt', $entity);
Copy link
Member Author

Choose a reason for hiding this comment

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

This assertion fails.

@@ -248,7 +248,7 @@ public function testIssetReturnsTrueOnSuccess()

$_SESSION['foo'] = 'bar';

$this->assertTrue(isset($session->foo));
$this->assertObjectHasAttribute('foo', $session);
Copy link
Member Author

Choose a reason for hiding this comment

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

This assertion fails.

Copy link
Member

@samsonasik samsonasik Nov 15, 2021

Choose a reason for hiding this comment

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

You can register Rector\PHPUnit\Rector\MethodCall\AssertIssetToSpecificMethodRector into Option::SKIP

use Rector\PHPUnit\Rector\MethodCall\AssertIssetToSpecificMethodRector;

    $parameters->set(Option::SKIP, [
             // ....

            AssertIssetToSpecificMethodRector::class => [
                     __DIR__ . '/tests/system/Entity/EntityTest.php',
                    __DIR__ . '/tests/system/Session/SessionTest.php',
           ],
    ]);

and then rollback change for tests/system/Entity/EntityTest.php and tests/system/Session/SessionTest.php

Copy link
Member

@samsonasik samsonasik Nov 15, 2021

Choose a reason for hiding this comment

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

If AssertIssetToSpecificMethodRector cause invalid result in many places, we can just skip it al together:

use Rector\PHPUnit\Rector\MethodCall\AssertIssetToSpecificMethodRector;

    $parameters->set(Option::SKIP, [
             // ....

            AssertIssetToSpecificMethodRector::class,
    ]);

Copy link
Member Author

Choose a reason for hiding this comment

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

I would rather change the test code to remove the skip AssertIssetToSpecificMethodRector.

Thank you for telling me about rector!

@kenjis kenjis force-pushed the add-rectore-phpunit-rule branch from daeaeb3 to c4f0fbb Compare November 15, 2021 02:14
@kenjis kenjis merged commit 5508ef4 into codeigniter4:develop Nov 15, 2021
@kenjis kenjis deleted the add-rectore-phpunit-rule branch November 15, 2021 02:46
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.

2 participants