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

Enable PHPStan in tests #3260

Closed
wants to merge 85 commits into from
Closed

Conversation

Majkl578
Copy link
Contributor

Q A
Type improvement
BC Break no

Summary

Enabling PHPStan in tests/ at level 3, just like for lib/. 🎉
Rewritten some Prophecy-based unit tests into PHPUnit mock system to avoid having another (unofficial) extension for Prophecy.

Replaces incomplete #3203 with no changes like (Class -> mixed).

Currently depends on my fork of phpstan-phpunit due to phpstan/phpstan-phpunit#28.

);

self::assertCount(1, $results);
self::assertInstanceOf(__NAMESPACE__.'\\MyFetchClass', $results[0]);
self::assertInstanceOf(MyFetchClass::class, $results[0]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly needed, but affected by phpstan/phpstan-phpunit#27.

composer.json Outdated
@@ -21,6 +21,7 @@
"doctrine/coding-standard": "^4.0",
"jetbrains/phpstorm-stubs": "^2018.1.2",
"phpstan/phpstan": "^0.10.1",
"phpstan/phpstan-phpunit": "dev-GetMockBuilderDynamicReturnTypeExtension-with-undefined-class as 0.10.0.999",
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: make sure this patch is released first.

@@ -24,7 +24,7 @@ class PDOExceptionTest extends DbalTestCase
/**
* The wrapped PDO exception mock.
*
* @var \PDOException|\PHPUnit_Framework_MockObject_MockObject
* @var \PDOException
Copy link
Member

Choose a reason for hiding this comment

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

Yay!

@@ -15,7 +16,7 @@ class MySqlSchemaManagerTest extends \PHPUnit\Framework\TestCase
*/
private $manager;

/** @var Connection */
/** @var Connection|MockObject */
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the referenced code isn't acutally a mock at all, see https://github.com/doctrine/dbal/pull/3260/files#diff-90748caec80a98aa0fcfef640916661bR39. :)

Copy link
Member

Choose a reason for hiding this comment

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

I see. Makes sense.

I thought, using phpstan-phpunit would eliminate the necessity to explicitly annotate mocks as such. Looks like it's only the case for the return values of methods like createMock() which automatically get the traits of the object under test and the mock. But for properties, you still have to specify the type explictly. Does it sound about right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it changes return types of these factory methods to return intersections of the mock type and the mocked type.
Also, because intersection support sucks in IDEs, it should convert unions like Type|MockObject to an intersection Type&MockObject (not sure how this is implemented).
Additional constraints are there for assertion methods (i.e. assertInstanceOf), see readme: https://github.com/phpstan/phpstan-phpunit#phpstan-phpunit-extensions-and-rules

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it converts them in this case, currently hardcoded in PHPStan, but will be cleaner and part of the phpunit extension in 0.11.

@@ -24,7 +25,7 @@ class DateImmutableTypeTest extends \PHPUnit\Framework\TestCase
protected function setUp()
{
$this->type = Type::getType('date_immutable');
$this->platform = $this->prophesize(AbstractPlatform::class);
$this->platform = $this->getMockBuilder(AbstractPlatform::class)->getMock();
Copy link
Member

Choose a reason for hiding this comment

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

Probably could use $this->createMock() instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, forgot to change this file. 😃

* TaskMock used for testing the CLI interface.
* @author Nils Adermann <naderman@naderman.de>
*/
class TaskMock extends \Doctrine\Common\Cli\Tasks\AbstractTask
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Majkl578 Majkl578 force-pushed the phpstan-tests branch 3 times, most recently from 385b730 to 6a2b788 Compare August 18, 2018 19:35
@Majkl578 Majkl578 added this to the 2.9.0 milestone Aug 18, 2018
@morozov morozov self-assigned this Aug 28, 2018
@morozov morozov removed this from the 2.9.0 milestone Dec 2, 2018
Ocramius and others added 16 commits April 15, 2019 20:56
Changed the type of $char in AbstractPlatform::getTrimExpression()`from string|false to ?string
Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures
Reworked AbstractPlatform::get*Expression() methods
…n-key-rule-def

Removed DB2SchemaManager::_getPortableForeignKeyRuleDef()
Removed errorCode() and errorInfo() methods from Connection and Statement APIs
Reworked AbstractSchemaManager::extractDoctrineTypeFromComment(), removed ::removeDoctrineTypeFromComment()
@Majkl578
Copy link
Contributor Author

Majkl578 commented Apr 16, 2019

Rebased on top of develop, green on level 3, 65 more errors on level 7.

@Majkl578 Majkl578 changed the base branch from master to develop April 16, 2019 19:44
@morozov
Copy link
Member

morozov commented Apr 16, 2019

Rebased on top of develop, green on level 3, 65 more errors on level 7.

We can wrap it up at level 3 and continue to improve later in order to avoid future conflicts and new issues on Level 3 and lower. BTW, why develop? Is it possible to move it to master, same as lib/, especially given that we should have more freedom in making changes in tests?

@Majkl578
Copy link
Contributor Author

BTW, why develop?

Picked wrong target when rebasing and didn't want to start again on top of master. :|

@morozov
Copy link
Member

morozov commented Apr 16, 2019

Picked wrong target when rebasing and didn't want to start again on top of master. :|

Could you at least roll it back? I would like to have it done in master, and backporting to master and rebasing develop on top of it would be unnecessary extra work.

@morozov
Copy link
Member

morozov commented Jun 2, 2019

Closing in favor of #3582. Thanks, @Majkl578.

@morozov morozov closed this Jun 2, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants