From 0e9a3a41085e455c9d63c5ae0cc9397c46ebe5cb Mon Sep 17 00:00:00 2001
From: Martin Hujer <mhujer@gmail.com>
Date: Thu, 21 Dec 2017 17:20:26 +0100
Subject: [PATCH] AssertEqualsIsDiscouragedRule

---
 README.md                                     |  1 +
 .../PHPUnit/AssertEqualsIsDiscouragedRule.php | 92 +++++++++++++++++++
 strictRules.neon                              |  1 +
 .../AssertEqualsIsDiscouragedRuleTest.php     | 53 +++++++++++
 .../data/assert-equals-is-discouraged.php     | 34 +++++++
 5 files changed, 181 insertions(+)
 create mode 100644 src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php
 create mode 100644 tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php
 create mode 100644 tests/Rules/PHPUnit/data/assert-equals-is-discouraged.php

diff --git a/README.md b/README.md
index e73c901..e908206 100644
--- a/README.md
+++ b/README.md
@@ -24,6 +24,7 @@ It also contains this strict framework-specific rules (can be enabled separately
 * 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.
+* Check that you are not using `assertEquals()` with same types (`assertSame()` should be used) or without explanatory comment above.
 
 ## How to document mock objects in phpDocs?
 
diff --git a/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php b/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php
new file mode 100644
index 0000000..75f5e8b
--- /dev/null
+++ b/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php
@@ -0,0 +1,92 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Rules\PHPUnit;
+
+use PhpParser\Node;
+use PHPStan\Analyser\Scope;
+use PHPStan\Type\BooleanType;
+use PHPStan\Type\FloatType;
+use PHPStan\Type\IntegerType;
+use PHPStan\Type\StringType;
+
+class AssertEqualsIsDiscouragedRule implements \PHPStan\Rules\Rule
+{
+
+	public function getNodeType(): string
+	{
+		return \PhpParser\NodeAbstract::class;
+	}
+
+	/**
+	 * @param \PhpParser\Node\Expr\MethodCall|\PhpParser\Node\Expr\StaticCall $node
+	 * @param \PHPStan\Analyser\Scope $scope
+	 * @return string[] errors
+	 */
+	public function processNode(Node $node, Scope $scope): array
+	{
+		if (!AssertRuleHelper::isMethodOrStaticCallOnTestCase($node, $scope)) {
+			return [];
+		}
+
+		if (count($node->args) < 2) {
+			return [];
+		}
+		if (!is_string($node->name) || strtolower($node->name) !== 'assertequals') {
+			return [];
+		}
+
+		$leftType = $scope->getType($node->args[0]->value);
+		$rightType = $scope->getType($node->args[1]->value);
+
+		if (
+			($leftType instanceof BooleanType && $rightType instanceof BooleanType)
+			|| ($leftType instanceof IntegerType && $rightType instanceof IntegerType)
+			|| ($leftType instanceof StringType && $rightType instanceof StringType)
+		) {
+			$typeDescription = $leftType->describe();
+			if ($leftType instanceof BooleanType) {
+				$typeDescription = 'bool';
+			}
+			return [
+				sprintf(
+					'You should use assertSame instead of assertEquals, because both values are of the same type "%s"',
+					$typeDescription
+				),
+			];
+		}
+		if (
+			($leftType instanceof FloatType && $rightType instanceof FloatType)
+			&& count($node->args) < 4 // is not using delta for comparing floats
+		) {
+			return [
+				'You should use assertSame instead of assertEquals, because both values are of the same type "float" and you are not using $delta argument',
+			];
+		}
+
+		if (!$node->hasAttribute('comments')) {
+			return [
+				'You should use assertSame instead of assertEquals. Or it should have a comment above with explanation: // assertEquals because ...',
+			];
+		}
+
+		/** @var \PhpParser\Comment[] $comments */
+		$comments = $node->getAttribute('comments');
+		$comment = $comments[count($comments) - 1];
+
+		// the comment should be on the line above the assertEquals()
+		if ($comment->getLine() !== ($node->getLine() - 1)) {
+			return [
+				'You should use assertSame instead of assertEquals. Or it should have a comment above with explanation: // assertEquals because ... (The comment is not directly above the assertEquals)',
+			];
+		}
+
+		if (preg_match('~^//\s+assertEquals because(.*)~', $comment->getReformattedText()) === 0) {
+			return [
+				'You should use assertSame instead of assertEquals. Or it should have a comment above with explanation: // assertEquals because ... (There is a different comment)',
+			];
+		}
+
+		return [];
+	}
+
+}
diff --git a/strictRules.neon b/strictRules.neon
index ec02f7d..3cf4b4b 100644
--- a/strictRules.neon
+++ b/strictRules.neon
@@ -2,3 +2,4 @@ rules:
 	- PHPStan\Rules\PHPUnit\AssertSameBooleanExpectedRule
 	- PHPStan\Rules\PHPUnit\AssertSameNullExpectedRule
 	- PHPStan\Rules\PHPUnit\AssertSameWithCountRule
+	- PHPStan\Rules\PHPUnit\AssertEqualsIsDiscouragedRule
diff --git a/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php b/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php
new file mode 100644
index 0000000..3d1158c
--- /dev/null
+++ b/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php
@@ -0,0 +1,53 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Rules\PHPUnit;
+
+use PHPStan\Rules\Rule;
+
+class AssertEqualsIsDiscouragedRuleTest extends \PHPStan\Testing\RuleTestCase
+{
+
+	protected function getRule(): Rule
+	{
+		return new AssertEqualsIsDiscouragedRule();
+	}
+
+	public function testRule(): void
+	{
+		$this->analyse([__DIR__ . '/data/assert-equals-is-discouraged.php'], [
+			[
+				'You should use assertSame instead of assertEquals, because both values are of the same type "string"',
+				11,
+			],
+			[
+				'You should use assertSame instead of assertEquals, because both values are of the same type "int(1)"',
+				12,
+			],
+			[
+				'You should use assertSame instead of assertEquals, because both values are of the same type "bool"',
+				13,
+			],
+			[
+				'You should use assertSame instead of assertEquals, because both values are of the same type "float" and you are not using $delta argument',
+				16,
+			],
+			[
+				'You should use assertSame instead of assertEquals. Or it should have a comment above with explanation: // assertEquals because ... (There is a different comment)',
+				19,
+			],
+			[
+				'You should use assertSame instead of assertEquals. Or it should have a comment above with explanation: // assertEquals because ...',
+				21,
+			],
+			[
+				'You should use assertSame instead of assertEquals. Or it should have a comment above with explanation: // assertEquals because ... (There is a different comment)',
+				24,
+			],
+			[
+				'You should use assertSame instead of assertEquals. Or it should have a comment above with explanation: // assertEquals because ... (The comment is not directly above the assertEquals)',
+				28,
+			],
+		]);
+	}
+
+}
diff --git a/tests/Rules/PHPUnit/data/assert-equals-is-discouraged.php b/tests/Rules/PHPUnit/data/assert-equals-is-discouraged.php
new file mode 100644
index 0000000..18c02c1
--- /dev/null
+++ b/tests/Rules/PHPUnit/data/assert-equals-is-discouraged.php
@@ -0,0 +1,34 @@
+<?php declare(strict_types = 1);
+
+namespace ExampleTestCase;
+
+class AssertEqualsIsDiscouragedTestCase extends \PHPUnit\Framework\TestCase
+{
+
+	public function testAssertEqualsIsDiscouraged()
+	{
+		// assertSame can be used as both are of same type
+		$this->assertEquals('a', 'b');
+		$this->assertEquals(1, 2);
+		$this->assertEquals(true, false);
+
+		// comparing floats without delta
+		$this->assertEquals(1.0, 2.0);
+
+		// comparing floats with delta
+		$this->assertEquals(1.0, 2.0, '', 0.01);
+
+		$this->assertEquals(1, '1'); // assertEquals without comment on previous line
+
+		// with incorrect comment
+		$this->assertEquals(1, '1');
+
+		// assertEquals because I want it! But sadly, the comment is not just above the assert.
+
+		$this->assertEquals(1, '1');
+
+		// assertEquals because I want it!
+		$this->assertEquals(1, '1');
+	}
+
+}