Merge pull request #4 from DaveLiddament/feature/improve-dx

Improve DX
DaveLiddament · Dec 14, 2023 · ca0a9f6 · ca0a9f6
2 parents 4dc5854 + 514ff9c
commit ca0a9f6
Show file tree

Hide file tree

Showing 17 changed files with 479 additions and 127 deletions.
diff --git a/README.md b/README.md
@@ -1,22 +1,23 @@
 # PHPStan rule testing helper
 
-This is a helper library for slight improvement to DX for testing PHPStan rules.
-It allows you to write the expected error message in the fixture file. 
-Anything after `// ERROR ` is considered the expected error message.
-The test classes are simplified as you now specify just the fixture files, and this library will extract the expected error and calculate the correct line number.
+This library offers a couple of improvements to  PHPStan's [custom rule test harness](https://phpstan.org/developing-extensions/testing#custom-rules).
 
-You can also use an `ErrorMessageFormatter` to further decouple tests from the actual error message. 
-See [ErrorMessageFormatter](#error-formatter) section.
+This library provides [AbstractRuleTestCase](src/AbstractRuleTestCase.php), which extends PHPStan's `RuleTestCase`.
 
-## Example
+It offers a simpler way to write tests for custom rules. Specifically:
+
+1. No need to specify line numbers in the test code.
+2. You can specify the expected error message once.
+
+## Improvement 1: No more line numbers in tests
+
+The minimal test case specifies the Rule being tested and at least one test.
+Each test must call the `assertIssuesReported` method, which takes the path of one or more fixture files.
 
-Test code extends [AbstractRuleTestCase](src/AbstractRuleTestCase.php). 
-As with the PHPStan's `RuleTestCase` use the `getRule` method to setup the rule used by the test.
-For each test list the fixture file(s) needed by the test, using the `assertIssuesReported` method.
 
 #### Test code:
 ```php
-use DaveLiddament\PhpstanRuleTestingHelper\AbstractRuleTestCase;
+use DaveLiddament\PhpstanRuleTestHelper\AbstractRuleTestCase;
 
 class CallableFromRuleTest extends AbstractRuleTestCase
 {
@@ -46,154 +47,173 @@ class SomeCode
 }
 ```
 
-Every line that contains `// ERROR ` is considered an issue that should be picked up by the rule. 
+Every line that contains `// ERROR ` is considered an issue that should be picked up by the rule.
 The text after `// ERROR` is the expected error message.
 
-With this approach you don't need to work out the line number of the error.
-There are further benefits by using the [ErrorMessageFormatter](#error-formatter) to decouple the error messages from the test code.
-
-
-
-NOTE: You can pass in multiple fixture files. E.g.
-```php
-$this->assertIssuesReported(
-    __DIR__ . '/Fixtures/SomeCode.php', 
-    __DIR__ . '/Fixtures/SomeCode2.php',
-    // And so on...    
- );
-```
-
-
-## Installation
-
-```shell
-composer require --dev dave-liddament/phpstan-rule-test-helper
-```
-
-## Error Formatter
-
-The chances are when you developing PHPStan rules the error message for violations will change. 
-Making any change will require you to update all the related tests.
-
-### Constant string error messages
+With this approach you don't need to work out the line number of the error. 
+This is particularly handy when you update the Fixture file, you no longer have to update all the line numbers in the test.
 
-In the simplest case the error is a message that does provide any context, other than line number. 
-E.g. in the example the error is `Can not call method`. No additional information (e.g. who was trying to call the method) is provided.
 
-Create a class that extends `ConstantErrorFormatter` and pass the error message to the constructor.
+# Improvement 2: Specify the expected error message once
 
-Update the test to tell it to use a `ConstantStringErrorMessageFormatter`.
+Often you end up writing the same error message for every violation. To get round this use the `getErrorFromatter` method to specify the error message.
 
+#### Test code:
 ```php
-class CallableFromRuleTest extends AbstractRuleTestCase 
+use DaveLiddament\PhpstanRuleTestHelper\AbstractRuleTestCase;
+
+class CallableFromRuleTest extends AbstractRuleTestCase
 {
-   // getRule method omitted for brevity
-   // testAllowedCall method omitted for brevity
+    protected function getRule(): Rule
+    {
+        return new CallableFromRule($this->createReflectionProvider());
+    }
 
-    protected function getErrorFormatter(): ErrorMessageFormatter
+    public function testAllowedCall(): void
+    {
+        $this->assertIssuesReported(__DIR__ . '/Fixtures/SomeCode.php');
+    }
+
+    protected function getErrorFormatter(): string
     {
-        return new ConstantStringErrorMessageFormatter("Can not call method");
+        return "Can not call method";
     }
 }
 ```
 
-Now if the error message is changed, the text only needs to be updated in one place.
-
-Finally, the fixture can be simplified.
-There is no need to specify the error message in the fixture file, we just need to specify where the error is.
+The fixture file is simplified as there is no need to specify the error message. Any lines where an error is expected need to end with `// ERROR`.
+#### Fixture:
 
-Updated fixture:
-```php
+```php 
 class SomeCode
 {
     public function go(): void
     {
         $item = new Item("hello");
         $item->updateName("world");  // ERROR
     }
+
+    public function go2(): void
+    {
+        $item = new Item("hello");
+        $item->remove();  // ERROR
+    }
 }
 ```
 
-### Error messages with context
-
-Good error message will provide context. 
-For example, the error message could be improved to give the name of the calling class. 
-The calling class is `SomeClass` so let's update the error message to `Can not call method from SomeCode`.
+### Adding context to error messages
 
-The fixture is updated to include the calling class name after `// ERROR`
+Good error message require context. The context is added to the fixture file after `// ERROR `. Multiple pieces of context can be added by separating them with the `|` character.
 
+#### Test code:
 ```php
-class SomeCode
+use DaveLiddament\PhpstanRuleTestHelper\AbstractRuleTestCase;
+
+class CallableFromRuleTest extends AbstractRuleTestCase
 {
-    public function go(): void
+    protected function getRule(): Rule
     {
-        $item = new Item("hello");
-        $item->updateName("world");  // ERROR SomeCode
+        return new CallableFromRule($this->createReflectionProvider());
+    }
+
+    public function testAllowedCall(): void
+    {
+        $this->assertIssuesReported(__DIR__ . '/Fixtures/SomeCode.php');
+    }
+
+    protected function getErrorFormatter(): string
+    {
+        return "Can not call {0} from within class {1}";
     }
 }
 ```
 
-The `CallableFromRuleErrorFormatter` is updated. 
-Firstly it now extends `ErrorMessageFormatter` instead of `ConstantErrorFormatter`.
-An implementation of `getErrorMessage` is added. 
-This is passed everything after `\\ ERROR`, with whitespace trimmed from each side, and must return the expected error message.
+The fixture file is simplified as there is no need to specify the error message. Any lines where an error is expected need to end with `// ERROR`.
+#### Fixture:
 
-```php
-class CallableFromRuleErrorFormatter extends ErrorMessageFormatter
+```php 
+class SomeCode
 {
-    public function getErrorMessage(string $errorContext): string
+    public function go(): void
+    {
+        $item = new Item("hello");
+        $item->updateName("world");  // ERROR Item::updateName|SomeCode
+    }
+
+    public function go2(): void
     {
-        return 'Can not call method from ' . $errorContext;
+        $item = new Item("hello");
+        $item->remove();  // ERROR Item::remove|SomeCode
     }
 }
 ```
 
-### Error message helper methods
+The expected error messages would be:
+
+- Line 6: `Can not call Item::updateName from within class SomeCode`
+- Line 11: `Can not call Item::remove from within class SomeCode`
 
-Sometimes the contextual error messages might have 2 or more pieces of information. 
-Continuing the example above, the error message could be improved to give the name of the calling class and the method being called.
-E.g. `Can not call Item::updateName from SomeCode`.
+### More flexible error messages
 
-The fixture is updated to include both `Item::updateName` and `SomeCode` seperated by the `|` character. 
+If you need more flexibility in the error message, you can return an object that implements the `ErrorMessageFormatter` [interface](src/ErrorMessageFormatter.php).
 
-E.g. `// ERROR`
+In the example below the message changes depending on the number of parts in the error context.
 
 ```php
-class SomeCode
+use DaveLiddament\PhpstanRuleTestHelper\AbstractRuleTestCase;
+
+class CallableFromRuleTest extends AbstractRuleTestCase
 {
-    public function go(): void
+protected function getRule(): Rule
+{
+return new CallableFromRule($this->createReflectionProvider());
+}
+
+    public function testAllowedCall(): void
     {
-        $item = new Item("hello");
-        $item->updateName("world");  // ERROR Item::updateName|SomeCode
+        $this->assertIssuesReported(__DIR__ . '/Fixtures/SomeCode.php');
     }
+
+    protected function getErrorFormatter(): ErrorMessageFormatter
+    {
+        new class() extends ErrorMessageFormatter {
+            public function getErrorMessage(string $errorContext): string
+            {
+                $parts = $this->getErrorMessageAsParts($errorContext);
+                $calledFrom = count($parts) === 2 ?  'class '.$parts[1] : 'outside an object';
+
+                return sprintf('Can not call %s from %s', $parts[0], $calledFrom);
+            }
+        };
+   }
 }
 ```
 
-Use the `getErrorMessageAsParts` helper method to do this, as shown below:
-
-```php
+#### Fixture:
 
-class CallableFromRuleErrorFormatter extends ErrorMessageFormatter
+```php 
+class SomeCode
 {
-    public function getErrorMessage(string $errorContext): string
+    public function go(): void
     {
-        $parts = $this->getErrorMessageAsParts($errorContext, 2);
-        return sprintf('Can not call %s from %s', $parts[0], $parts[1]);
+        $item = new Item("hello");
+        $item->updateName("world");  // ERROR Item::updateName|SomeCode
     }
 }
+
+$item = new Item("hello");
+$item->remove();  // ERROR Item::remove
 ```
 
-The signature of `getErrorMessageAsParts` is:
+The expected error messages would be:
+
+- Line 6: `Can not call Item::updateName from class SomeCode`
+- Line 11: `Can not call Item::remove from outside an object`
+
 
-```php
-/**
- * @return list<string>
- */
-protected function getErrorMessageAsParts(
-    string $errorContext, 
-    int $expectedNumberOfParts,
-    string $separator = '|', 
-): array
-```
 
-If you use the `getErrorMessageAsParts` and the number of parts is not as expected, the test will error with a message that tells you file and line number of the invalid error.
+## Installation
+
+```shell
+composer require --dev dave-liddament/phpstan-rule-test-helper
+```
diff --git a/fixtures/goto.php b/fixtures/goto.php
@@ -1,10 +1,10 @@
 <?php
 
-goto foo; // ERROR goto statement is not allowed
+goto foo; // ERROR foo
 
 foo:
 echo 'foo';
-goto bar; // ERROR goto statement is not allowed
+goto bar; // ERROR bar
 
 bar:
 echo 'bar';
diff --git a/fixtures/gotoWithMessageInComment.php b/fixtures/gotoWithMessageInComment.php
@@ -0,0 +1,10 @@
+<?php
+
+goto foo; // ERROR goto statement is not allowed. Label: foo
+
+foo:
+echo 'foo';
+goto bar; // ERROR goto statement is not allowed. Label: bar
+
+bar:
+echo 'bar';
diff --git a/src/AbstractRuleTestCase.php b/src/AbstractRuleTestCase.php
@@ -5,6 +5,7 @@
 namespace DaveLiddament\PhpstanRuleTestHelper;
 
 use DaveLiddament\PhpstanRuleTestHelper\Internal\FixtureFileReader;
+use DaveLiddament\PhpstanRuleTestHelper\Internal\InvalidFixtureFile;
 use PHPStan\Testing\RuleTestCase;
 
 /**
@@ -14,10 +15,16 @@
  */
 abstract class AbstractRuleTestCase extends RuleTestCase
 {
+    /** @throws InvalidFixtureFile */
     final protected function assertIssuesReported(string ...$fixtureFiles): void
     {
         $fixtureFileReader = new FixtureFileReader();
         $errorFormatter = $this->getErrorFormatter();
+
+        if (is_string($errorFormatter)) {
+            $errorFormatter = new StringErrorMessageConverter($errorFormatter);
+        }
+
         $expectedErrors = [];
         foreach ($fixtureFiles as $fixture) {
             $expectedErrors = array_merge(
@@ -29,7 +36,7 @@ final protected function assertIssuesReported(string ...$fixtureFiles): void
         $this->analyse($fixtureFiles, $expectedErrors);
     }
 
-    protected function getErrorFormatter(): ErrorMessageFormatter
+    protected function getErrorFormatter(): ErrorMessageFormatter|string
     {
         return new DefaultErrorMessageFormatter();
     }

diff --git a/src/ConstantStringErrorMessageFormatter.php b/src/ConstantStringErrorMessageFormatter.php
@@ -4,6 +4,21 @@
 
 namespace DaveLiddament\PhpstanRuleTestHelper;
 
+/**
+ * @deprecated From v 0.3.0 this is no longer needed.`
+ *
+ * Before 0.3.0 `getErrorFormatter` had to return an instance of `ErrorMessageFormatter`.
+ *
+ * public function getErrorFormatter() {
+ *    return new ConstantStringErrorMessageFormatter('My error message');
+ * }
+ *
+ * Since 0.3.0 you can return a string or an instance of `ErrorMessageFormatter`.
+ *
+ * public function getErrorFormatter() {
+ *   return 'My error message';
+ * }
+ */
 class ConstantStringErrorMessageFormatter extends ErrorMessageFormatter
 {
     public function __construct(