[PHPUnit] Safe way to check if a class is a Test class by carusogabriel · Pull Request #374 · rectorphp/rector

carusogabriel · 2018-03-20T19:35:02Z

The current logic checks if the TestCase ends with Test, but this isn't the safest way to do it. For example:

<?php
class TestingCase extends \PHPUnit\Framework\TestCase {}

class MyTestCase extends TestingCase {}

carusogabriel · 2018-03-20T19:40:04Z

Do not merge wet, some cases fail if the test class extends a class that actually extends PHPUnit. I'll add tests to ensure that.

carusogabriel · 2018-03-20T19:45:13Z

    protected function isInTestClass(Node $node): bool
    {
-        $className = (string) $node->getAttribute(Attribute::CLASS_NAME);
+        $parentClassName = (string) $node->getAttribute(Attribute::PARENT_CLASS_NAME);


@TomasVotruba Do you know any Attribute that indicates all class parents?

All available Attribute items are in the Attribute class.

This should be resolved by TypeResolver, as it's used only sometimes. So I'd get CLASS_NODE and then get its types.

What do you think?

@TomasVotruba Do you have an example of TypeResolver that I can study and update this PR?

TomasVotruba · 2018-03-20T20:00:40Z

    protected function isInTestClass(Node $node): bool
    {
-        $className = (string) $node->getAttribute(Attribute::CLASS_NAME);
+        $parentClassName = (string) $node->getAttribute(Attribute::PARENT_CLASS_NAME);


All available Attribute items are in the Attribute class.

This should be resolved by TypeResolver, as it's used only sometimes. So I'd get CLASS_NODE and then get its types.

What do you think?

TomasVotruba · 2018-03-20T20:01:46Z

-     * Check if the file is a PHPUnit TestCase. By default, it should end with "Test", as it is the standard.
-     *
-     * @see https://phpunit.de/getting-started-with-phpunit.html
-     */


Why removing this?

What if sb will come and try to analyze nodes (much worse performance) instead of simple and fast PHPUnit approach.

Because I test Rector against an application that didn't follow this rule, plus, all its test classes extends a based one :(

I see.

Failing test would tell this for itself. What about that?

@TomasVotruba Pushed 7c79640

carusogabriel · 2018-03-25T06:04:12Z

    protected function isInTestClass(Node $node): bool
    {
-        $className = (string) $node->getAttribute(Attribute::CLASS_NAME);
+        $nodeTypeResolver = new NodeTypeResolver();


@TomasVotruba Should we revert #300? Because doing this way, isn't working by our CS :(

@TomasVotruba ping.

Is there a way to DI without __construct? If not, revert #300 will be the solution (unfortunately)

I missed this, sorry.

You can inject dependency as in Abstract class:

rector/src/Rector/AbstractRector.php

Lines 20 to 48 in 9f4ca80

/**

* @var ExpressionAdder

*/

private $expressionAdder;

/**

* @var PropertyAdder

*/

private $propertyAdder;

/**

* Nasty magic, unable to do that in config autowire _instanceof calls.

*

* @required

*/

public function setExpressionAdder(ExpressionAdder $expressionAdder): void

{

$this->expressionAdder = $expressionAdder;

}

/**

* Nasty magic, unable to do that in config autowire _instanceof calls.

*

* @required

*/

public function setPropertyToClassAdder(PropertyAdder $propertyAdder): void

{

$this->propertyAdder = $propertyAdder;

}

That's Symfony approach to this

carusogabriel · 2018-03-31T01:07:02Z

Finally done, we can now merge it 🎉

TomasVotruba

Just one detail and it's ready to merge

TomasVotruba · 2018-03-31T13:17:22Z

+        $nodeResolved = $this->nodeTypeResolver->resolve($node);

-        return Strings::endsWith($className, 'Test');
+        return ! array_intersect([TestCase::class, 'PHPUnit_Framework_TestCase'], $nodeResolved);


What happens, if sb installs a Rector but doesn't have PHPUnit with TestCase class installed?

I didn’t get it :(

Without TestCase isn’t possible to execute tests, is?

Well, somebody can use PHPUnit_Framework_TestCase, right?

We are checking it, aren't we? 🤔

return ! array_intersect([TestCase::class, 'PHPUnit_Framework_TestCase'], $nodeResolved);

Yes, but that depends on PHPUnit\Framework\TestCase class to exist. And it does in our code so it works.

Sb might download Rector and have only PHPUnit_Framework_TestCase in their code.
So PHPUnit\FrameworkTestCase will end up as un-existing class error.

That's why it should string here.

So I'll ignore this file from our CS. I used ::class here because CS complained about it

TomasVotruba · 2018-04-01T22:59:34Z

Awesome, thank you 👍

rectorphp/rector-src@ae71516 cleanup unused fixtures (#374)

carusogabriel self-assigned this Mar 20, 2018

carusogabriel requested a review from TomasVotruba March 20, 2018 19:35

carusogabriel changed the title ~~[PHPUnit] Safe way to check if a class is a Test class~~ [WIP][PHPUnit] Safe way to check if a class is a Test class Mar 20, 2018

carusogabriel commented Mar 20, 2018

View reviewed changes

TomasVotruba reviewed Mar 20, 2018

View reviewed changes

carusogabriel force-pushed the phpunit branch from 0d0c44a to 9066caa Compare March 25, 2018 06:03

carusogabriel commented Mar 25, 2018

View reviewed changes

carusogabriel force-pushed the phpunit branch from 9066caa to cd26884 Compare March 31, 2018 01:05

carusogabriel changed the title ~~[WIP][PHPUnit] Safe way to check if a class is a Test class~~ [PHPUnit] Safe way to check if a class is a Test class Mar 31, 2018

TomasVotruba reviewed Mar 31, 2018

View reviewed changes

carusogabriel added 5 commits March 31, 2018 17:01

[PHPUnit] Safe way to check if a class is a Test class

20f0bca

Add failing test for extend test classes

f99be18

Use NodeTypeResolver

2cca204

Inject NodeTypeResolver in AbstractPHPUnitRector class

c922234

Use string instead of ::class when looking for TestCase

845590d

carusogabriel force-pushed the phpunit branch from 7c145aa to 845590d Compare March 31, 2018 20:02

TomasVotruba merged commit c7b5b67 into master Apr 1, 2018

TomasVotruba deleted the phpunit branch April 1, 2018 22:59

carusogabriel assigned carusogabriel and unassigned carusogabriel Oct 14, 2018

TomasVotruba added a commit that referenced this pull request Jul 4, 2021

Updated Rector to commit ae71516

96365f9

rectorphp/rector-src@ae71516 cleanup unused fixtures (#374)

echo511 pushed a commit to echo511/rector that referenced this pull request Sep 4, 2021

cleanup unused fixtures (rectorphp#374)

ae71516

	/**
	* @var ExpressionAdder
	*/
	private $expressionAdder;

	/**
	* @var PropertyAdder
	*/
	private $propertyAdder;

	/**
	* Nasty magic, unable to do that in config autowire _instanceof calls.
	*
	* @required
	*/
	public function setExpressionAdder(ExpressionAdder $expressionAdder): void
	{
	$this->expressionAdder = $expressionAdder;
	}

	/**
	* Nasty magic, unable to do that in config autowire _instanceof calls.
	*
	* @required
	*/
	public function setPropertyToClassAdder(PropertyAdder $propertyAdder): void
	{
	$this->propertyAdder = $propertyAdder;
	}

Uh oh!

Conversation

carusogabriel commented Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carusogabriel commented Mar 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomasVotruba Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carusogabriel Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carusogabriel commented Mar 31, 2018

Uh oh!

TomasVotruba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomasVotruba Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomasVotruba commented Apr 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

carusogabriel commented Mar 20, 2018 •

edited

Loading

TomasVotruba Mar 20, 2018 •

edited

Loading

carusogabriel Mar 25, 2018 •

edited

Loading

TomasVotruba Mar 31, 2018 •

edited

Loading