Adding tests and other code quality checks, fixing a couple of issues #2

jaydiablo · 2019-04-11T16:58:33Z

This PR adds tests and other code quality checks to this plugin (seems like the foundation was there for this but not everything was in place, like config files for psalm/phpcs). I "stole" most of this skeleton from the psalm phpunit plugin (https://github.com/psalm/phpunit-psalm-plugin thanks @weirdan for your work on that).

I think this repo will have to be enabled in Travis in order to have the tests run against this PR. I did confirm that all checks are green on my branch (except for PHP 7.4 and Nightly, which are having build issues generally right now due to a missing library file). Here's the last build for my branch: https://travis-ci.org/jaydiablo/mockery-psalm-plugin/builds/518856454

I tried to add tests that covered the cases that I've run into recently when testing this plugin:

Intersection type for mocks? #1 - I've added an additional modification to the return type when the first parameter to Mockery::mock is a string (and it may or may not be in the format of ClassName[methodName1,methodName2]). The test I added for this intentionally triggers a psalm error in order to verify that the expected type is an intersection type (ClassName&\Mockery\MockInterface). I had difficulty getting that test to fail otherwise.
Could not get class storage for Mockery\Adapter\Phpunit\MockeryPHPUnitIntegrationAssertPostConditions vimeo/psalm#1537 - I've added a couple of different stubs that are loaded based on the detected PHPUnit version (using basically the same logic that Mockery is using to determine which class to use as the alias). I was able to confirm that the test I added would fail just as my installation of Psalm does without these stubs present (a Fatal Error is thrown during the analyze phase).

I've also pointed my project to this branch of my fork and confirmed that the issues I was seeing are no longer present with these changes (Mockery 1.x and PHPUnit 6.x).

I'll add some inline comments to point out the reasoning for some specific changes.

…arsing of Mockery::mock parameter

jaydiablo · 2019-04-11T17:00:01Z

Plugin.php

+        $registration->addStubFile(__DIR__ . '/stubs/Mockery.php');
+
+        if (class_exists('PHPUnit_Framework_TestCase')
+            || (class_exists('\PHPUnit\Runner\Version')


These checks are almost verbatim from Mockery (https://github.com/mockery/mockery/blob/master/library/Mockery/Adapter/Phpunit/MockeryPHPUnitIntegration.php#L25) but I added the additional class_exists check on \PHPUnit\Runner\Version just in case somebody is using Mockery outside of PHPUnit.

jaydiablo · 2019-04-11T17:02:45Z

composer.json

-        "squizlabs/php_codesniffer": "^3.3"
+        "codeception/base": "^2.5",
+        "muglug/package-versions-56": "^1.2",
+        "squizlabs/php_codesniffer": "^3.3.1",


I bumped the version number slightly here because 3.3.0 has a continue in a switch which causes a Notice in PHP 7.3, which causes the "lowest" tests to fail on that version of PHP (3.3.1 fixed the issue: squizlabs/PHP_CodeSniffer@16c771d)

jaydiablo · 2019-04-11T17:05:20Z

hooks/MockReturnTypeUpdater.php

@@ -51,7 +51,16 @@ public static function afterMethodCallAnalysis(
                && $first_arg->right instanceof PhpParser\Node\Scalar\String_
                && $first_arg->right->value[0] === '['
            ) {
-                $fq_class_name = $first_arg->left->class->getAttribute('resolvedName');
+                /** @var PhpParser\Node\Expr\ClassConstFetch $left */


Psalm was complaining that PhpParser\Node\Expr doesn't have a class property here. There's an instanceof check in the if condition above that makes sure that $first_arg->left is an instance of PhpParser\Node\Expr\ClassConstFetch, which does have the $class property: https://github.com/nikic/PHP-Parser/blob/master/lib/PhpParser/Node/Expr/ClassConstFetch.php#L12

jaydiablo · 2019-04-11T17:07:12Z

hooks/MockReturnTypeUpdater.php

+                /** @var PhpParser\Node\Expr\ClassConstFetch $left */
+                $left = $first_arg->left;
+                $fq_class_name = $left->class->getAttribute('resolvedName');
+            } elseif ($first_arg instanceof PhpParser\Node\Scalar\String_) {


This is the addition I made for #1 which allows, and interprets the class properly when these syntaxes are used:

Mockery::mock('ClassName[method1,method2]');
Mockery::mock('ClassName');

jaydiablo · 2019-04-11T17:08:03Z

stubs/Mockery.php

@@ -1,4 +1,5 @@
 <?php
+// phpcs:disable PSR1.Classes.ClassDeclaration.MultipleClasses


Since multiple classes/interfaces are defined in this file, just ignoring that php-cs rule for this file (but still processing other rules, which is what the rest of the changes in this file are)

jaydiablo · 2019-04-11T17:10:02Z

tests/_support/Helper/Acceptance.php

+     *
+     * @return void
+     */
+    public function havePsalmOfACertainVersionRangeBecauseOf(string $operator, string $version, string $reason)


These are copies from the PHPUnit version of this from the psalm phpunit plugin: https://github.com/psalm/phpunit-psalm-plugin/blob/master/tests/_support/Helper/Acceptance.php#L26 just with the repo change for the respective library I'm checking the version of.

jaydiablo · 2019-04-11T17:12:59Z

tests/acceptance/MockReturn.feature

+
+      $user = Mockery::mock('NS\User[someMethod]', []);
+
+      if (is_array($user)) {


This is_array check throws a psalm error, which is intentional so that the error message can be verified by this test (mainly that it contains the proper expected intersection type).

I tried other approaches here, but I couldn't get Psalm to throw an error when I removed the changes I made to the MockReturn hook. I did confirm that without my changes, the intersection type isn't present here, as the error message changes to this:

DocblockTypeContradiction | Cannot resolve types for $user - docblock-defined type Mockery\Mock does not contain array<array-key, mixed>

tests/acceptance/MockReturn.feature

jaydiablo · 2019-04-11T17:14:57Z

tests/acceptance/PHPUnitIntegration.feature

+      """
+    And I have Mockery newer than "0.9.99" (because of "Trait added in Mockery 1.0")
+    When I run Psalm
+    Then I see no errors


Without the stubs I added, this does throw a Fatal Error in Psalm 3.x

muglug · 2019-04-19T20:51:00Z

Thanks!

Adding test suite, fixing fatal error with mockery trait, improving p…

e26e848

…arsing of Mockery::mock parameter

jaydiablo commented Apr 11, 2019

View reviewed changes

jaydiablo added 2 commits April 11, 2019 11:24

adding another gitignore file

19ed070

removing duplicate test in favor of a simple pattern match

6b80037

muglug merged commit 5db654d into psalm:master Apr 19, 2019

jaydiablo deleted the add-tests-and-fixes branch April 20, 2019 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding tests and other code quality checks, fixing a couple of issues #2

Adding tests and other code quality checks, fixing a couple of issues #2

jaydiablo commented Apr 11, 2019

jaydiablo Apr 11, 2019

jaydiablo Apr 11, 2019

jaydiablo Apr 11, 2019

jaydiablo Apr 11, 2019

jaydiablo Apr 11, 2019

jaydiablo Apr 11, 2019

jaydiablo Apr 11, 2019

jaydiablo Apr 11, 2019

muglug commented Apr 19, 2019

		@@ -1,4 +1,5 @@
		<?php
		// phpcs:disable PSR1.Classes.ClassDeclaration.MultipleClasses


		$user = Mockery::mock('NS\User[someMethod]', []);

		if (is_array($user)) {

Adding tests and other code quality checks, fixing a couple of issues #2

Adding tests and other code quality checks, fixing a couple of issues #2

Conversation

jaydiablo commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muglug commented Apr 19, 2019