chore: first step towards Psalm level 6 by carlos-granados · Pull Request #1554 · Behat/Behat

carlos-granados · 2024-12-03T07:55:54Z

We should be increasing our Psalm level. Moving to error level 6 produces more than 100 errors, so, to make this more manageable, we can introduce the level 6 rules in small batches. This PR introduces one of these rules with the corresponding needed changes

carlos-granados · 2024-12-03T07:56:27Z

psalm.xml

        </MethodSignatureMismatch>
+
+        <InvalidArgument>
+            <errorLevel type="error"/>


By adding this, we ask Psalm to apply this rule and report any errors found

carlos-granados · 2024-12-03T07:57:27Z

psalm.xml

+        <InvalidArgument>
+            <errorLevel type="error"/>
+            <errorLevel type="suppress">
+                <file name="src/Behat/Testwork/EventDispatcher/TestworkEventDispatcher.php" />


These two files need to be excluded because they produce false positives due to the code that is needed to keep the bc compatibility for all Symfony versions

carlos-granados · 2024-12-03T07:57:49Z

src/Behat/Behat/Context/Cli/InteractiveContextIdentifier.php

        $message = $this->translator->trans('snippet_context_choice', array('%count%' => $suiteName), 'output');
        $choices = array_values(array_merge(array('None'), $contextClasses));
-        $default = 1;
+        $default = '1';


$default needs to be a string

carlos-granados · 2024-12-03T07:58:12Z

src/Behat/Behat/Context/Exception/WrongContextClassException.php

-     * @param string  $class
     */
-    public function __construct($message, $class)
+    public function __construct(string $message, string $class)


doc-block was wrong, I changed it to use php types

carlos-granados · 2024-12-03T07:59:06Z

src/Behat/Behat/Definition/Search/RepositorySearchEngine.php


            $result = $newResult;
-            $definitions[] = $newResult->getMatchedDefinition();
+            $matchedDefinition = $newResult->getMatchedDefinition();


getMatchDefinition() can return null, we need to check this and only add to the list of definitions if its not null

carlos-granados · 2024-12-03T07:59:54Z

src/Behat/Behat/Output/Node/EventListener/AST/OutlineListener.php

        $this->exampleSetupPrinter->printSetup($formatter, $event->getSetup());
-        $this->examplePrinter->printHeader($formatter, $event->getFeature(), $this->example);
+
+        $this->example = $event->getScenario();


We need to make sure that the ScenarioNode returned by getScenario() is an ExampleNode

The structure of the way the event listeners is registered is somewhat complex, but I think the assumption is that events only reach this listener if we're in the course of running an Outline and therefore the scenario should be an ExampleNode?

If so, getting an unexpected type of ScenarioNode here would suggest either there's a problem with our event binding, or with our assumptions about how the nodes / events are structured.

In which case it might be better / safer to throw an exception (and hopefully fail CI) rather than silently not print anything? This would be the intentional version of the current behaviour where it will hit a TypeError.

carlos-granados · 2024-12-03T08:00:27Z

src/Behat/Behat/Output/Node/Printer/JUnit/JUnitFeaturePrinter.php

            $totalCount = 0;
        } else {
-            $totalCount = array_sum($stats);
+            $totalCount = (int) array_sum($stats);


$totalCount needs to be an int

carlos-granados · 2024-12-03T08:00:44Z

src/Behat/Behat/Output/Node/Printer/Pretty/PrettySetupPrinter.php

        $style = $this->resultConverter->convertResultCodeToString($resultCode);
        $hook = $callResult->getCall()->getCallee();
        $path = $hook->getPath();
+        $hookName = (string)$hook;


We make the conversion to string explicit

Suggested change

$hookName = (string)$hook;

$hookName = (string) $hook;

To ensure code style consistency :)

carlos-granados · 2024-12-03T08:01:39Z

src/Behat/Behat/Output/Statistics/PhaseStatistics.php

     * Returns counters for different scenario result codes.
     *
-     * @return array[]
+     * @return int[]


doc-block was wrong

carlos-granados · 2024-12-03T08:03:02Z

src/Behat/Testwork/EventDispatcher/TestworkEventDispatcher.php

    }

-    private function bcAwareDispatch(object $event, $eventName)
+    private function bcAwareDispatch(?object $event, $eventName)


$event can be null

stof · 2024-12-03T08:20:48Z

src/Behat/Behat/Output/Node/EventListener/AST/OutlineListener.php

 use Behat\Testwork\Event\Event;
 use Behat\Testwork\Output\Formatter;
 use Behat\Testwork\Output\Node\EventListener\EventListener;
+use phpDocumentor\Reflection\DocBlock\Tags\Example;


this use statement looks wrong.

stof · 2024-12-03T08:25:16Z

src/Behat/Behat/Output/Statistics/TotalStatistics.php

     * Returns counters for different scenario result codes.
     *
-     * @return array[]
+     * @return int[]


I suggest documenting this as array<TestResults::*, int> to be more precise about the array keys.

fmatsos · 2024-12-05T15:14:20Z

src/Behat/Behat/Output/Node/Printer/Pretty/PrettySetupPrinter.php

        $style = $this->resultConverter->convertResultCodeToString($resultCode);
        $hook = $callResult->getCall()->getCallee();
        $path = $hook->getPath();
+        $hookName = (string)$hook;


Suggested change

$hookName = (string)$hook;

$hookName = (string) $hook;

To ensure code style consistency :)

fmatsos · 2024-12-05T15:15:08Z

src/Behat/Behat/Output/Node/Printer/Pretty/PrettySetupPrinter.php

        $style = $this->resultConverter->convertResultCodeToString($resultCode);
        $hook = $callResult->getCall()->getCallee();
        $path = $hook->getPath();
+        $hookName = (string)$hook;


Suggested change

$hookName = (string)$hook;

$hookName = (string) $hook;

Same here.

Updated, thanks!!

acoulton

Good work @carlos-granados just a question about how best to handle unexpected types

acoulton · 2024-12-06T10:05:16Z

src/Behat/Behat/Output/Node/EventListener/AST/OutlineListener.php

        $this->exampleSetupPrinter->printSetup($formatter, $event->getSetup());
-        $this->examplePrinter->printHeader($formatter, $event->getFeature(), $this->example);
+
+        $this->example = $event->getScenario();


The structure of the way the event listeners is registered is somewhat complex, but I think the assumption is that events only reach this listener if we're in the course of running an Outline and therefore the scenario should be an ExampleNode?

If so, getting an unexpected type of ScenarioNode here would suggest either there's a problem with our event binding, or with our assumptions about how the nodes / events are structured.

In which case it might be better / safer to throw an exception (and hopefully fail CI) rather than silently not print anything? This would be the intentional version of the current behaviour where it will hit a TypeError.

acoulton · 2024-12-06T10:05:45Z

src/Behat/Behat/Output/Node/EventListener/AST/OutlineTableListener.php

+        if ($example instanceof ExampleNode) {
+            $this->exampleRowPrinter->printExampleRow($formatter, $this->outline, $example, $this->stepAfterTestedEvents);
+        }


Same observation as the other Outline-related listener above, I think perhaps we should throw?

I've updated the code to use assertions, which is what PHPStan recommends for this kind of type narrowing. What do you think?

That looks like a good solution to me

acoulton

Great, thanks @carlos-granados

carlos-granados force-pushed the psalm-level-6-1 branch from a0c8d06 to 28b6eb1 Compare December 3, 2024 08:02

carlos-granados commented Dec 3, 2024

View reviewed changes

stof reviewed Dec 3, 2024

View reviewed changes

fmatsos reviewed Dec 5, 2024

View reviewed changes

carlos-granados added 3 commits December 5, 2024 19:22

chore: first step towards Psalm level 6

cf921d8

remove wrong use statement, add more detailed array types

e8667c9

fix cs

53a1a43

carlos-granados force-pushed the psalm-level-6-1 branch from 7d05e69 to 53a1a43 Compare December 5, 2024 22:36

acoulton reviewed Dec 6, 2024

View reviewed changes

Change checks to assertions

e65a537

carlos-granados requested a review from acoulton December 7, 2024 21:10

acoulton approved these changes Dec 9, 2024

View reviewed changes

acoulton merged commit 91c9e0d into Behat:master Dec 9, 2024

carlos-granados deleted the psalm-level-6-1 branch December 9, 2024 10:01

Uh oh!

Comments

Conversation

carlos-granados commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acoulton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acoulton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carlos-granados commented Dec 3, 2024 •

edited

Loading