Skip to content

PHPStan improvements #100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jan 13, 2025
Merged

PHPStan improvements #100

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ff64b4e
Fix phpstan warnings; use central constants for fixtures
uuf6429 Jan 5, 2025
e4f3f1e
Increase PHPStan level
uuf6429 Jan 5, 2025
54126b5
Move fixture constants to TestCase
uuf6429 Jan 6, 2025
a8c2fb2
Tiny improvement
uuf6429 Jan 6, 2025
efcc5de
Fix typo
uuf6429 Jan 6, 2025
613d036
Remove (currently pointless) default value
uuf6429 Jan 6, 2025
a64cf16
Use correct phpstan annotation
uuf6429 Jan 6, 2025
9a9d3e2
Review fixes
uuf6429 Jan 7, 2025
8e296ed
Use getString instead
uuf6429 Jan 7, 2025
786d9b7
Use old-symfony-compatible version
uuf6429 Jan 7, 2025
a1950ba
Use correct getter
uuf6429 Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion http-kernel-fixtures/cookie_page2.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
<body>
Previous cookie: <?php
/** @var \Symfony\Component\HttpFoundation\Request $request */
echo $request->cookies->has('srvr_cookie') ? html_escape_value($request->cookies->get('srvr_cookie')) : 'NO';
echo $request->cookies->has('srvr_cookie') ? html_escape_value($request->cookies->get('srvr_cookie') ?? '') : 'NO';
?>
</body>
</html>
1 change: 1 addition & 0 deletions http-kernel-fixtures/sub-folder/cookie_page1.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<?php
/** @var \Symfony\Component\HttpFoundation\Request $request */
$requestUri = $request->server->get('REQUEST_URI');
assert(is_string($requestUri));
$resp = new Symfony\Component\HttpFoundation\Response();
$cook = Symfony\Component\HttpFoundation\Cookie::create('srvr_cookie', 'srv_var_is_set_sub_folder', 0, dirname($requestUri));
$resp->headers->setCookie($cook);
Expand Down
5 changes: 4 additions & 1 deletion http-kernel-fixtures/sub-folder/cookie_page4.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
<?php
/** @var \Symfony\Component\HttpFoundation\Request $request */
$resp = new Symfony\Component\HttpFoundation\Response();
$cookiePath = dirname($request->server->get('REQUEST_URI')) . '/';
$requestUri = $request->server->get('REQUEST_URI');
assert(is_string($requestUri));
$cookiePath = dirname($requestUri) . '/';
assert(is_string($cookiePath));
$cookie = Symfony\Component\HttpFoundation\Cookie::create('srvr_cookie', 'srv_var_is_set', 0, $cookiePath);
$resp->headers->setCookie($cookie);
?>
Expand Down
3 changes: 1 addition & 2 deletions phpstan.dist.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
parameters:
level: 8
level: 9
paths:
- src
- tests
- http-kernel-fixtures
- web-fixtures
checkMissingIterableValueType: false

includes:
- vendor/phpstan/phpstan-phpunit/extension.neon
Expand Down
12 changes: 8 additions & 4 deletions src/FixturesKernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@

class FixturesKernel implements HttpKernelInterface
{
public const WEB_FIXTURES_DIR = __DIR__ . '/../web-fixtures';
public const KERNEL_FIXTURES_DIR = __DIR__ . '/../http-kernel-fixtures';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why making those public constants ?

Copy link
Member Author

@uuf6429 uuf6429 Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the main point actually, so that code needing those paths:

  • do not need to define their own path relative to their location
  • we get to see where those paths are used from one central place (instead of searching for 'web-fixtures' string)
  • if the classes using those constants are relocated, the path in constant value remains valid

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are values of these constants used anywhere else outside of this class?

Copy link
Member Author

@uuf6429 uuf6429 Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes (WEB_FIXTURES is used by FixturesKernel, GeneralTest and ChangeEventTest), testcases can use them when they need to use a specific fixture.

KERNEL_FIXTURES isn't used elsewhere in this case, it's not entirely clear to me where/when it is needed, but it feels like it should be public too.

Copy link
Member Author

@uuf6429 uuf6429 Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kernel fixtures doesn't seem to be used outside at all: https://github.com/search?q=%22%2Fhttp-kernel-fixtures%22&type=code

Web fixtures is used also by users:
https://github.com/robertfausk/mink-panther-driver/blob/52aa735bc0cf05f5fd9406f4da56136b00b9c5cb/tests/Custom/EventsTest.php#L145

__DIR__.'/../../vendor/mink/driver-testsuite/web-fixtures/ could be replaced with TestCase::WEB_FIXTURES_DIR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what mink-panther-driver does is a huge hack though. They are altering the shared testsuite, meaning that what they run is not actually the shared testsuite (and so their driver might not respect the expected shared behavior, creating interoperability issues). I don't think we should consider this a valid use case.

Copy link
Member Author

@uuf6429 uuf6429 Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a hack, yes, but the intention is clear - they want to extend tests, not alter existing ones (they're adding a new fixture and the original tests are unaltered nor excluded in the config).

From user-perspective, there could be better ways (e.g. putting the fixture in a "custom" directory or somehow altering the php server), but that feels like it's also our responsibility to provide that.

In this case, for example, we could have provided a CUSTOM_WEB_FIXTURES constant which is exactly for this case. There might still be uses for WEB_FIXTURES... but in any case being public is just one side of that constant.


public function handle(Request $request, $type = 1 /* self::MAIN_REQUEST */ , $catch = true): Response
{
$this->prepareSession($request);
Expand All @@ -25,8 +28,8 @@ public function handle(Request $request, $type = 1 /* self::MAIN_REQUEST */ , $c

private function handleFixtureRequest(Request $request): Response
{
$fixturesDir = realpath(__DIR__ . '/../web-fixtures');
$overwriteDir = realpath(__DIR__ . '/../http-kernel-fixtures');
$fixturesDir = realpath(self::WEB_FIXTURES_DIR);
$overwriteDir = realpath(self::KERNEL_FIXTURES_DIR);

require_once $fixturesDir . '/utils.php';

Expand Down Expand Up @@ -60,8 +63,9 @@ private function prepareSession(Request $request): void

$cookies = $request->cookies;

if ($cookies->has($session->getName())) {
$session->setId($cookies->get($session->getName()));
$value = $cookies->get($session->getName());
if ($value !== null) {
$session->setId($value);
} else {
$session->migrate(false);
}
Expand Down
9 changes: 5 additions & 4 deletions tests/Basic/BasicAuthTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@ public function testSetBasicAuth(string $user, string $pass, string $pageText):
$this->assertStringContainsString($pageText, $session->getPage()->getContent());
}

/**
* @return iterable<string, array{string, string, string}>
*/
public static function setBasicAuthDataProvider(): iterable
{
return [
['mink-user', 'mink-password', 'is authenticated'],
['', '', 'is not authenticated'],
];
yield 'valid credentials' => ['mink-user', 'mink-password', 'is authenticated'];
yield 'no credentials' => ['', '', 'is not authenticated'];
}

public function testBasicAuthInUrl(): void
Expand Down
15 changes: 9 additions & 6 deletions tests/Basic/BestPracticesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ public function testImplementFindXpath(): void
{
$driver = $this->createDriver();

$this->assertNotImplementMethod('find', $driver, 'The driver should overwrite `findElementXpaths` rather than `find` for forward compatibility with Mink 2.');
$this->assertImplementMethod('findElementXpaths', $driver, 'The driver must be able to find elements.');
$this->assertNotImplementMethod('setSession', $driver, 'The driver should not deal with the Session directly for forward compatibility with Mink 2.');
$this->assertMethodIsNotImplemented('find', $driver, 'The driver should overwrite `findElementXpaths` rather than `find` for forward compatibility with Mink 2.');
$this->assertMethodIsImplemented('findElementXpaths', $driver, 'The driver must be able to find elements.');
$this->assertMethodIsNotImplemented('setSession', $driver, 'The driver should not deal with the Session directly for forward compatibility with Mink 2.');
}

/**
Expand All @@ -36,9 +36,12 @@ public function testImplementBasicApi(string $method): void
{
$driver = $this->createDriver();

$this->assertImplementMethod($method, $driver, 'The driver is unusable when this method is not implemented.');
$this->assertMethodIsImplemented($method, $driver, 'The driver is unusable when this method is not implemented.');
}

/**
* @return iterable<array{string}>
*/
public static function provideRequiredMethods(): iterable
{
return [
Expand All @@ -53,7 +56,7 @@ public static function provideRequiredMethods(): iterable
];
}

private function assertImplementMethod(string $method, object $object, string $reason = ''): void
private function assertMethodIsImplemented(string $method, object $object, string $reason = ''): void
{
$ref = new \ReflectionClass(get_class($object));
$refMethod = $ref->getMethod($method);
Expand All @@ -67,7 +70,7 @@ private function assertImplementMethod(string $method, object $object, string $r
$this->assertNotSame(CoreDriver::class, $refMethod->getDeclaringClass()->name, $message);
}

private function assertNotImplementMethod(string $method, object $object, string $reason = ''): void
private function assertMethodIsNotImplemented(string $method, object $object, string $reason = ''): void
{
$ref = new \ReflectionClass(get_class($object));
$refMethod = $ref->getMethod($method);
Expand Down
3 changes: 3 additions & 0 deletions tests/Basic/ContentTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ public function testGetAttribute(string $attributeName, ?string $attributeValue)
$this->assertSame($attributeValue, $element->getAttribute($attributeName));
}

/**
* @return iterable<array{string, mixed}>
*/
public static function getAttributeDataProvider(): iterable
{
return [
Expand Down
7 changes: 7 additions & 0 deletions tests/Basic/CookieTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@

use Behat\Mink\Tests\Driver\TestCase;

/**
* @phpstan-type TCookieRemovalMode 'session_reset'|'cookie_delete'
*/
final class CookieTest extends TestCase
{
/**
Expand Down Expand Up @@ -87,6 +90,9 @@ public function testCookieWithPaths(string $cookieRemovalMode): void
$this->assertStringContainsString('Previous cookie: NO', $session->getPage()->getText());
}

/**
* @return iterable<array{TCookieRemovalMode}>
*/
public static function cookieWithPathsDataProvider(): iterable
{
return [
Expand All @@ -96,6 +102,7 @@ public static function cookieWithPathsDataProvider(): iterable
}

/**
* @param TCookieRemovalMode $cookieRemovalMode
* @dataProvider cookieWithPathsDataProvider
*/
public function testCookieInSubPath(string $cookieRemovalMode): void
Expand Down
10 changes: 4 additions & 6 deletions tests/Basic/IFrameTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,11 @@ public function testIFrame(string $iframeIdentifier, string $elementSelector, st
}

/**
* @return array
* @return iterable<string, array{string, string, string}>
*/
public static function iFrameDataProvider()
public static function iFrameDataProvider(): iterable
{
return array(
'by name' => array('subframe_by_name', '#text', 'iFrame div text'),
'by id' => array('subframe_by_id', '#foobar', 'Some accentués characters'),
);
yield 'by name' => ['subframe_by_name', '#text', 'iFrame div text'];
yield 'by id' => ['subframe_by_id', '#foobar', 'Some accentués characters'];
}
}
21 changes: 13 additions & 8 deletions tests/Form/GeneralTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Behat\Mink\Exception\DriverException;
use Behat\Mink\Tests\Driver\TestCase;
use Behat\Mink\Tests\Driver\Util\FixturesKernel;

final class GeneralTest extends TestCase
{
Expand Down Expand Up @@ -77,14 +78,15 @@ public function testFormSubmitWays(string $submitVia): void
}
}

/**
* @return iterable<array{string}>
*/
public static function formSubmitWaysDataProvider(): iterable
{
return [
['Save'],
['input-type-image'],
['button-without-type'],
['button-type-submit'],
];
yield ['Save'];
yield ['input-type-image'];
yield ['button-without-type'];
yield ['button-type-submit'];
}

public function testFormSubmit(): void
Expand Down Expand Up @@ -189,7 +191,7 @@ public function testAdvancedForm(): void
$notes->setValue('new notes');
$this->assertEquals('new notes', $notes->getValue());

$about->attachFile($this->mapRemoteFilePath(__DIR__ . '/../../web-fixtures/some_file.txt'));
$about->attachFile($this->mapRemoteFilePath(FixturesKernel::WEB_FIXTURES_DIR . '/some_file.txt'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making GeneralTest depend on the kernel is bad, because this introduces a hard dependency on symfony/http-kernel, while it is currently an optional dependency (this FixturesKernel is currently only used by the browserkit-driver testsuite, when running it with a KernelBrowser instead of doing actual HTTP requests to the test server).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the beginning I was going to do it the other way round:

class TestCase {
    public const XXXXX;
}

class FixturesKernel {
    ...
    TestCase::XXXXX;
    ...
}

But then I figured that tests/ is normally expected not be exported (although that is not the case in this project).

That begs the follow-up question...since this test suite IS the main point of project, shouldn't everything in tests/ actually be in src/?

Anyway, we have these options:

  1. keep it as is with the FixturesKernel (feels like a bad idea, from your description)
  2. put it in TestCase (should be fine considering that tests/ must be exported)
  3. make a separate class in src/ just for these constants

I'll go for option 2, let me know if 3 sounds better or you have a different idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That begs the follow-up question...since this test suite IS the main point of project, shouldn't everything in tests/ actually be in src/?

no. I'd rather keep the shared testsuite under a tests folder for consistency (and also to avoid breaking the test setup of all drivers by forcing them to change the path to tests in their phpunit.xml)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't suggesting to merge src/ and tests/ now, I was more curious if there were specific reasons other than historical/compatibility. But this is a bit off-topic anyway.

I'd just like to know out of those 3 options if option 2 is fine.


$button = $page->findButton('Register');
$this->assertNotNull($button);
Expand Down Expand Up @@ -352,7 +354,7 @@ public function testSubmitEmptyTextarea(): void
/**
* @dataProvider provideInvalidValues
*
* @param mixed $value
* @param array<array-key, mixed>|bool|string $value
*/
public function testSetInvalidValueInField(string $field, $value): void
{
Expand All @@ -366,6 +368,9 @@ public function testSetInvalidValueInField(string $field, $value): void
$color->setValue($value);
}

/**
* @return iterable<string, array{string, mixed}>
*/
public static function provideInvalidValues(): iterable
{
$trueValue = ['true', true];
Expand Down
5 changes: 4 additions & 1 deletion tests/Form/Html5Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ public function testHtml5FormMethod(): void
/**
* @dataProvider provideInvalidValues
*
* @param mixed $value
* @param array<array-key, mixed>|bool|string $value
*/
public function testSetInvalidValueInField(string $field, $value): void
{
Expand All @@ -177,6 +177,9 @@ public function testSetInvalidValueInField(string $field, $value): void
$color->setValue($value);
}

/**
* @return iterable<string, array{string, mixed}>
*/
public static function provideInvalidValues(): iterable
{
$trueValue = ['true', true];
Expand Down
3 changes: 3 additions & 0 deletions tests/Form/RadioTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ public function testSetBooleanValue(bool $value): void
$option->setValue($value);
}

/**
* @return iterable<array{bool}>
*/
public static function provideBooleanValues(): iterable
{
yield [true];
Expand Down
6 changes: 6 additions & 0 deletions tests/Form/SelectTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ public function testElementSelectedStateCheck(string $selectName, string $option
$this->assertTrue($option->isSelected());
}

/**
* @return iterable<array{string, string, string}>
*/
public static function elementSelectedStateCheckDataProvider(): iterable
{
return [
Expand Down Expand Up @@ -119,6 +122,9 @@ public function testSetBooleanValue(bool $value): void
$select->setValue($value);
}

/**
* @return iterable<array{bool}>
*/
public static function provideBooleanValues(): iterable
{
yield [true];
Expand Down
41 changes: 21 additions & 20 deletions tests/Js/ChangeEventTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Behat\Mink\Tests\Driver\Js;

use Behat\Mink\Tests\Driver\TestCase;
use Behat\Mink\Tests\Driver\Util\FixturesKernel;

/**
* @group slow
Expand Down Expand Up @@ -63,20 +64,18 @@ public function testSetValueChangeEvent(string $elementId, string $valueForEmpty
}
}

/**
* @return iterable<string, array{string, string, string}>
*/
public static function setValueChangeEventDataProvider(): iterable
{
$file1 = __DIR__ . '/../../web-fixtures/file1.txt';
$file2 = __DIR__ . '/../../web-fixtures/file2.txt';

return [
'input default' => ['the-input-default', 'from empty', 'from existing'],
'input text' => ['the-input-text', 'from empty', 'from existing'],
'input email' => ['the-email', 'from empty', 'from existing'],
'textarea' => ['the-textarea', 'from empty', 'from existing'],
'file' => ['the-file', $file1, $file2],
'select' => ['the-select', '30'],
'radio' => ['the-radio-m', 'm'],
];
yield 'input default' => ['the-input-default', 'from empty', 'from existing'];
yield 'input text' => ['the-input-text', 'from empty', 'from existing'];
yield 'input email' => ['the-email', 'from empty', 'from existing'];
yield 'textarea' => ['the-textarea', 'from empty', 'from existing'];
yield 'file' => ['the-file', FixturesKernel::WEB_FIXTURES_DIR . '/file1.txt', FixturesKernel::WEB_FIXTURES_DIR . '/file2.txt'];
yield 'select' => ['the-select', '30', ''];
yield 'radio' => ['the-radio-m', 'm', ''];
}

/**
Expand All @@ -95,12 +94,13 @@ public function testSelectOptionChangeEvent(string $elementId, string $elementVa
$this->assertElementChangeCount($elementId);
}

/**
* @return iterable<string, array{string, string}>
*/
public static function selectOptionChangeEventDataProvider(): iterable
{
return [
'select' => ['the-select', '30'],
'radio' => ['the-radio-m', 'm'],
];
yield 'select' => ['the-select', '30'];
yield 'radio' => ['the-radio-m', 'm'];
}

/**
Expand Down Expand Up @@ -145,12 +145,13 @@ public function testUncheckChangeEvent(bool $useSetValue): void
$this->assertElementChangeCount('the-checked-checkbox');
}

/**
* @return iterable<array{mixed}>
*/
public static function checkboxTestWayDataProvider(): iterable
{
return [
[true],
[false],
];
yield [true];
yield [false];
}

private function assertElementChangeCount(string $elementId, string $message = ''): void
Expand Down
Loading
Loading