diff --git a/README.md b/README.md index 75b6fe9..e73c901 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,13 @@ It also contains this framework-specific rule (can be enabled separately): * Check that both values passed to `assertSame()` method are of the same type. +It also contains this strict framework-specific rules (can be enabled separately): + +* Check that you are not using `assertSame()` with `true` as expected value. `assertTrue()` should be used instead. +* Check that you are not using `assertSame()` with `false` as expected value. `assertFalse()` should be used instead. +* Check that you are not using `assertSame()` with `null` as expected value. `assertNull()` should be used instead. +* Check that you are not using `assertSame()` with `count($variable)` as second parameter. `assertCount($variable)` should be used instead. + ## How to document mock objects in phpDocs? If you need to configure the mock even after you assign it to a property or return it from a method, you should add `PHPUnit_Framework_MockObject_MockObject` to the phpDoc: @@ -81,3 +88,9 @@ To perform framework-specific checks, include also this file: ``` - vendor/phpstan/phpstan-phpunit/rules.neon ``` + +To perform addition strict PHPUnit checks, include also this file: + +``` + - vendor/phpstan/phpstan-phpunit/strictRules.neon +``` diff --git a/build.xml b/build.xml index 8fc91d1..7b50b9c 100644 --- a/build.xml +++ b/build.xml @@ -42,7 +42,6 @@ - @@ -59,7 +58,6 @@ - diff --git a/phpcs.xml b/phpcs.xml index 28ee4e9..2e09373 100644 --- a/phpcs.xml +++ b/phpcs.xml @@ -36,4 +36,5 @@ + tests/*/data diff --git a/src/Rules/PHPUnit/AssertRuleHelper.php b/src/Rules/PHPUnit/AssertRuleHelper.php new file mode 100644 index 0000000..6e2e864 --- /dev/null +++ b/src/Rules/PHPUnit/AssertRuleHelper.php @@ -0,0 +1,52 @@ +getType($node->var); + } elseif ($node instanceof Node\Expr\StaticCall) { + if ($node->class instanceof Node\Name) { + $class = (string) $node->class; + if (in_array( + strtolower($class), + [ + 'self', + 'static', + 'parent', + ], + true + )) { + $calledOnType = new ObjectType($scope->getClassReflection()->getName()); + } else { + $calledOnType = new ObjectType($class); + } + } else { + $calledOnType = $scope->getType($node->class); + } + } else { + return false; + } + + if (!$testCaseType->isSuperTypeOf($calledOnType)->yes()) { + return false; + } + + return true; + } + +} diff --git a/src/Rules/PHPUnit/AssertSameBooleanExpectedRule.php b/src/Rules/PHPUnit/AssertSameBooleanExpectedRule.php new file mode 100644 index 0000000..715ec96 --- /dev/null +++ b/src/Rules/PHPUnit/AssertSameBooleanExpectedRule.php @@ -0,0 +1,53 @@ +args) < 2) { + return []; + } + if (!is_string($node->name) || strtolower($node->name) !== 'assertsame') { + return []; + } + + $leftType = $scope->getType($node->args[0]->value); + + if ($leftType instanceof TrueBooleanType) { + return [ + 'You should use assertTrue() instead of assertSame() when expecting "true"', + ]; + } + + if ($leftType instanceof FalseBooleanType) { + return [ + 'You should use assertFalse() instead of assertSame() when expecting "false"', + ]; + } + + return []; + } + +} diff --git a/src/Rules/PHPUnit/AssertSameDifferentTypesRule.php b/src/Rules/PHPUnit/AssertSameDifferentTypesRule.php index c16c6bc..84d130e 100644 --- a/src/Rules/PHPUnit/AssertSameDifferentTypesRule.php +++ b/src/Rules/PHPUnit/AssertSameDifferentTypesRule.php @@ -4,7 +4,6 @@ use PhpParser\Node; use PHPStan\Analyser\Scope; -use PHPStan\Type\ObjectType; class AssertSameDifferentTypesRule implements \PHPStan\Rules\Rule { @@ -21,33 +20,7 @@ public function getNodeType(): string */ public function processNode(Node $node, Scope $scope): array { - $testCaseType = new ObjectType(\PHPUnit\Framework\TestCase::class); - if ($node instanceof Node\Expr\MethodCall) { - $calledOnType = $scope->getType($node->var); - } elseif ($node instanceof Node\Expr\StaticCall) { - if ($node->class instanceof Node\Name) { - $class = (string) $node->class; - if (in_array( - strtolower($class), - [ - 'self', - 'static', - 'parent', - ], - true - )) { - $calledOnType = new ObjectType($scope->getClassReflection()->getName()); - } else { - $calledOnType = new ObjectType($class); - } - } else { - $calledOnType = $scope->getType($node->class); - } - } else { - return []; - } - - if (!$testCaseType->isSuperTypeOf($calledOnType)->yes()) { + if (!AssertRuleHelper::isMethodOrStaticCallOnTestCase($node, $scope)) { return []; } diff --git a/src/Rules/PHPUnit/AssertSameNullExpectedRule.php b/src/Rules/PHPUnit/AssertSameNullExpectedRule.php new file mode 100644 index 0000000..318c947 --- /dev/null +++ b/src/Rules/PHPUnit/AssertSameNullExpectedRule.php @@ -0,0 +1,46 @@ +args) < 2) { + return []; + } + if (!is_string($node->name) || strtolower($node->name) !== 'assertsame') { + return []; + } + + $leftType = $scope->getType($node->args[0]->value); + + if ($leftType instanceof NullType) { + return [ + 'You should use assertNull() instead of assertSame(null, $actual).', + ]; + } + + return []; + } + +} diff --git a/src/Rules/PHPUnit/AssertSameWithCountRule.php b/src/Rules/PHPUnit/AssertSameWithCountRule.php new file mode 100644 index 0000000..d90612e --- /dev/null +++ b/src/Rules/PHPUnit/AssertSameWithCountRule.php @@ -0,0 +1,49 @@ +args) < 2) { + return []; + } + if (!is_string($node->name) || strtolower($node->name) !== 'assertsame') { + return []; + } + + $right = $node->args[1]->value; + + if ( + $right instanceof Node\Expr\FuncCall + && $right->name instanceof Node\Name + && strtolower($right->name->toString()) === 'count' + ) { + return [ + 'You should use assertCount($expectedCount, $variable) instead of assertSame($expectedCount, count($variable)).', + ]; + } + + return []; + } + +} diff --git a/strictRules.neon b/strictRules.neon new file mode 100644 index 0000000..db0671e --- /dev/null +++ b/strictRules.neon @@ -0,0 +1,13 @@ +services: + - + class: PHPStan\Rules\PHPUnit\AssertSameBooleanExpectedRule + tags: + - phpstan.rules.rule + - + class: PHPStan\Rules\PHPUnit\AssertSameNullExpectedRule + tags: + - phpstan.rules.rule + - + class: PHPStan\Rules\PHPUnit\AssertSameWithCountRule + tags: + - phpstan.rules.rule diff --git a/tests/Rules/PHPUnit/AssertSameBooleanExpectedRuleTest.php b/tests/Rules/PHPUnit/AssertSameBooleanExpectedRuleTest.php new file mode 100644 index 0000000..a745ffc --- /dev/null +++ b/tests/Rules/PHPUnit/AssertSameBooleanExpectedRuleTest.php @@ -0,0 +1,37 @@ +analyse([__DIR__ . '/data/assert-same-boolean-expected.php'], [ + [ + 'You should use assertTrue() instead of assertSame() when expecting "true"', + 10, + ], + [ + 'You should use assertFalse() instead of assertSame() when expecting "false"', + 11, + ], + [ + 'You should use assertTrue() instead of assertSame() when expecting "true"', + 14, + ], + [ + 'You should use assertFalse() instead of assertSame() when expecting "false"', + 17, + ], + ]); + } + +} diff --git a/tests/Rules/PHPUnit/AssertSameDifferentTypesRuleTest.php b/tests/Rules/PHPUnit/AssertSameDifferentTypesRuleTest.php index ac66d1c..1a925b0 100644 --- a/tests/Rules/PHPUnit/AssertSameDifferentTypesRuleTest.php +++ b/tests/Rules/PHPUnit/AssertSameDifferentTypesRuleTest.php @@ -35,9 +35,21 @@ public function testRule() 'Call to assertSame() with different types array and array will always result in test failure.', 14, ], + [ + 'Call to assertSame() with different types string and int will always result in test failure.', + 16, + ], + [ + 'Call to assertSame() with different types string and int will always result in test failure.', + 17, + ], + [ + 'Call to assertSame() with different types string and int will always result in test failure.', + 18, + ], [ 'Call to assertSame() with different types array and array will always result in test failure.', - 35, + 39, ], ]); } diff --git a/tests/Rules/PHPUnit/AssertSameNullExpectedRuleTest.php b/tests/Rules/PHPUnit/AssertSameNullExpectedRuleTest.php new file mode 100644 index 0000000..a56d8c7 --- /dev/null +++ b/tests/Rules/PHPUnit/AssertSameNullExpectedRuleTest.php @@ -0,0 +1,29 @@ +analyse([__DIR__ . '/data/assert-same-null-expected.php'], [ + [ + 'You should use assertNull() instead of assertSame(null, $actual).', + 10, + ], + [ + 'You should use assertNull() instead of assertSame(null, $actual).', + 13, + ], + ]); + } + +} diff --git a/tests/Rules/PHPUnit/AssertSameWithCountRuleTest.php b/tests/Rules/PHPUnit/AssertSameWithCountRuleTest.php new file mode 100644 index 0000000..8171bcd --- /dev/null +++ b/tests/Rules/PHPUnit/AssertSameWithCountRuleTest.php @@ -0,0 +1,25 @@ +analyse([__DIR__ . '/data/assert-same-count.php'], [ + [ + 'You should use assertCount($expectedCount, $variable) instead of assertSame($expectedCount, count($variable)).', + 10, + ], + ]); + } + +} diff --git a/tests/Rules/PHPUnit/data/Foo.php b/tests/Rules/PHPUnit/data/Foo.php new file mode 100644 index 0000000..c5248d7 --- /dev/null +++ b/tests/Rules/PHPUnit/data/Foo.php @@ -0,0 +1,13 @@ +assertSame(true, 'a'); + $this->assertSame(false, 'a'); + + $truish = true; + $this->assertSame($truish, true); + + $falsish = false; + $this->assertSame($falsish, false); + + /** @var bool $a */ + $a = null; + $this->assertSame($a, 'b'); // OK + } + +} diff --git a/tests/Rules/PHPUnit/data/assert-same-count.php b/tests/Rules/PHPUnit/data/assert-same-count.php new file mode 100644 index 0000000..0716ec7 --- /dev/null +++ b/tests/Rules/PHPUnit/data/assert-same-count.php @@ -0,0 +1,18 @@ +assertSame(5, count([1, 2, 3])); + } + + public function testAssertSameWithCountMethodIsOK() + { + $this->assertSame(5, $this->count()); // OK + } + +} diff --git a/tests/Rules/PHPUnit/data/assert-same-null-expected.php b/tests/Rules/PHPUnit/data/assert-same-null-expected.php new file mode 100644 index 0000000..e4cec93 --- /dev/null +++ b/tests/Rules/PHPUnit/data/assert-same-null-expected.php @@ -0,0 +1,22 @@ +assertSame(null, 'a'); + + $a = null; + $this->assertSame($a, 'b'); + + $this->assertSame('a', 'b'); // OK + + /** @var string|null $c */ + $c = null; + $this->assertSame($c, 'foo'); // nullable is OK + } + +} diff --git a/tests/Rules/PHPUnit/data/assert-same.php b/tests/Rules/PHPUnit/data/assert-same.php index 8d3a18c..48b81e4 100644 --- a/tests/Rules/PHPUnit/data/assert-same.php +++ b/tests/Rules/PHPUnit/data/assert-same.php @@ -12,6 +12,10 @@ public function testObviouslyNotSameAssertSame() $this->assertSame(1, $this->returnsString()); $this->assertSame('1', self::returnsInt()); $this->assertSame(['a', 'b'], [1, 2]); + + self::assertSame('1', 2); // test self + static::assertSame('1', 2); // test static + parent::assertSame('1', 2); // test parent } private function returnsString(): string @@ -46,6 +50,19 @@ public function testLogicallyCorrectAssertSame() $this->assertSame(1, self::returnsInt()); $this->assertSame(['a'], ['a', 1]); $this->assertSame(['a', 2, 3.0], ['a', 1]); + self::assertSame(1, 2); // test self + static::assertSame(1, 2); // test static + parent::assertSame(1, 2); // test parent + } + + public function testOther() + { + // assertEquals is not checked + $this->assertEquals('1', 1); + + // only calls on \PHPUnit\Framework\TestCase are analyzed + $foo = new \Dummy\Foo(); + $foo->assertSame(); } }