rectorphp / rector-phpunit

Rector upgrade rules for PHPUnit
http://getrector.com
MIT License
61 stars 46 forks source link

[PHPUnit 10] DataProviderAnnotationToAttributeRector does not "find" tests class not inheriting directly from PHPUnit\Framework\TestCase #296

Closed allan-simon closed 7 months ago

allan-simon commented 7 months ago

i.e it works fine on class like this

use PHPUnit\Framework\TestCase;

class DateTest extends TestCase
{
}

but it does not work on

  use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;

  class BoostTest extends WebTestCase
  {

}

the issue seems to be that TestsNodeAnalyzer->isInTestClass does not consider them as test class

I'm trying to dig more in the issue

Note: I'm not sure if of any importance but my rector is installed in a different directory than my actual projet (i.e I have a common "rector" tool for all my projects , with its own composer.json )

samsonasik commented 7 months ago

I suggest to install rector in the root of every project, otherwise, you may got autoload juggling

allan-simon commented 7 months ago

thanks for your quick answer, the reason I don't want to do that is that I don't want my project to be blocked on some dependencies because I need library foobar at version X and a tool I use need it at version < X

note that other tools also recommend this way of doing https://cs.symfony.com/#documentation

(I will try to find back the rational for their recommendation but if i remember correctly it was more or less the argument above)

TomasVotruba commented 7 months ago

Rector is downgraded and scoped, so there is not such a blocker. All you need is PHP 7.2+

https://packagist.org/packages/rector/rector

Screenshot from 2023-12-02 15-39-25

TomasVotruba commented 7 months ago

The php-cs-fixer is doing it wrong. Instead they should scope and downgrade too.

Like ECS does - https://github.com/symplify/easy-coding-standard/

allan-simon commented 7 months ago

I also need a version phpstan, so I can't use a version below that yours (I also use phpstan) therefore my point stands

I however understand that it's a use case you don't want to support and I'm fine with it.

TomasVotruba commented 7 months ago

@allan-simon That's ok. PHPStan is also scoped and downgraded :) Upgrade to the latest PHPStan and you're good to go.

Screenshot from 2023-12-02 15-45-30

https://packagist.org/packages/phpstan/phpstan

allan-simon commented 7 months ago

yes but I may not want to upgrade "right now" my custom extensions :) which is my point

") Upgrade to the latest PHPStan and you're good to go." is not always that easy

samsonasik commented 7 months ago

How you install rector? rector-phpunitis included in rector/rector, so you need to install rector/rector only:

composer require rector/rector --dev

that, rector-phpunit is included inside /vendor/rector/rector-phpunit

https://github.com/rectorphp/rector/tree/main/vendor/rector/rector-phpunit

allan-simon commented 7 months ago

yes I installed only rector/rector , in a tools directory with its own composer.json

and when I run it precise the sources path of the project I want to run rector on

allan-simon commented 7 months ago

on " Upgrade to the latest PHPStan and you're good to go." is not always that easy

I give you an exemple I rely on phpstan extensions not made by me , that requires phpstan ^1.0 , it works well and I don't need to upgrade it, and the guy maintaining it has clearly say it does not plan on maintaining it further, but it's okay as it works well.

Tomorrow phpstan upgrade to 2.0 with a lot of awesome feature and you decide to switch to it, as it is a major version, it comes with backward compat breaking changes so now you support only phpstan 2.0 , and now I'm between rock and stone , i.e I can't upgrade rector because I can't upgrade the extension

It may seems an hypothetic example but I've been bitten by that enough time in the past to be careful about this now :)

samsonasik commented 7 months ago

You may need to use -a /path/to/autoload.php arg config to point to target project autoload

/your/tools/rector -a /path/to/target/autoload.php

If that still no work, I think that's will hard to make work on your use case due to autoload jugling.

My suggestion is partial install/remove extension one by one, and when applied, rollback it.

allan-simon commented 7 months ago

You may need to use -a /path/to/autoload.php arg config to point to target project autoload

ah thanks I will try this then !

My suggestion is partial install/remove extension one by one, and when applied, rollback it.

for tool A and tool B having conflicting dependencies ? I did that in the past but that's a PITA but thanks for the suggestion :) honestly having tools in their dedicated composer.json has been working fine for me for the last 5 years and today is the first time it is an issue with this specific rector rules, so to be honest in the worst case I would just make a local patch to rector so that the rule pass and I will revert it

allan-simon commented 7 months ago

awesome, it works with -a

allan-simon commented 7 months ago

Is there a way to configure this directly in the rector.php ? (so that my teammate don't need to remember the trick, especially as without it still does something , so people may not directly see that not everything was done )

samsonasik commented 7 months ago

There is autoloadPaths() method for that

https://github.com/rectorphp/rector-src/blob/be924be778eeceffa6105ab330ea7eb366ebb44b/packages/Config/RectorConfig.php#L281

https://getrector.com/documentation/static-reflection-and-autoload