PHPStan improvements #100

uuf6429 · 2025-01-05T20:43:26Z

No description provided.

stof · 2025-01-06T10:52:33Z

src/FixturesKernel.php

@@ -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';


why making those public constants ?

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

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

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.

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

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.

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.

tests/Basic/CookieTest.php

stof · 2025-01-06T11:02:40Z

tests/Form/GeneralTest.php

@@ -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'));


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).

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:

keep it as is with the FixturesKernel (feels like a bad idea, from your description)

put it in TestCase (should be fine considering that tests/ must be exported)

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.

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)

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.

tests/TestCase.php

stof · 2025-01-06T11:04:45Z

tests/TestCase.php

        }
    }

    /**
-     * @return AbstractConfig
-     *
-     * @throws \UnexpectedValueException if the global driver_config_factory returns an invalid object


the description should probably be kept

I wasn't sure if it would make sense to move it to createConfig or have it in both cases, but now that I think about it, it doesn't make sense at all: this is a private method of a class (not a trait even) so we don't really need to document it. Plus, the exception message is understandable enough.
(e.g. it would have made more sense to have it for createDriver and the like)

I'm also for keeping the description. It's now always clear when an exception is thrown just by looking at the DocBlock itself.

If the method is private it doesn't mean it should be less documented, because same documentation is also used by maintainers.

I'm also for keeping the description. It's now always clear when an exception is thrown just by looking at the DocBlock itself.

That will only apply to createConfig() which I expect to be rarely seen. getConfig would be more common and protected/public usages of getConfig even more than that.

If we care about documentation, then we should mention that exception in all cases (like I said, especially the API ones) not just to the internal one that no will ever actually see.

Otherwise I don't see the point of a 4-line comment explaining verbatim what the next 10 lines of code clearly do.

tests/Js/EventsTest.php

http-kernel-fixtures/sub-folder/cookie_page4.php

src/FixturesKernel.php

tests/Basic/BasicAuthTest.php

tests/Basic/BestPracticesTest.php

tests/Form/GeneralTest.php

tests/Js/ChangeEventTest.php

tests/Basic/BasicAuthTest.php

src/FixturesKernel.php

aik099 · 2025-01-08T18:10:28Z

There no issues left except for that exception comment in a DocBlock.

@uuf6429 , please test this new version of the driver test suite with all active Mink drivers using the lowest possible dependency version (likely you need to use PHP 7.2 for that).

uuf6429 · 2025-01-08T18:37:48Z

There no issues left except for that exception comment in a DocBlock.

@aik099 I'd like to see some go-ahead from @stof first, anyway.

please test this new version of the driver test suite with all active Mink drivers...

That's a whole lot of work (also because I don't know which are these active versions.. how about we add a new GH workflow (that is triggered manually) to do that? (as a separate PR of course, we could merge it before this one)

aik099 · 2025-01-09T09:28:14Z

Here are test suite tests (with changes from this PR) on the drivers:

MinkSelenium2Driver - no new failures (the right click test on Selenium 3 was already failing before)
webdriver-classic-driver - passed
MinkBrowserKitDriver - passed

.

Separate workflow for testing test suite changes for all active drivers is implemented in #103 .

uuf6429 added 2 commits January 5, 2025 21:20

Fix phpstan warnings; use central constants for fixtures

ff64b4e

Increase PHPStan level

e4f3f1e

This was referenced Jan 5, 2025

Test using invalid values for checkbox #102

Merged

Test switching to missing window #101

Merged

stof reviewed Jan 6, 2025

View reviewed changes

uuf6429 added 5 commits January 6, 2025 13:07

Move fixture constants to TestCase

54126b5

Tiny improvement

a8c2fb2

Fix typo

efcc5de

Remove (currently pointless) default value

613d036

Use correct phpstan annotation

a64cf16

aik099 requested changes Jan 7, 2025

View reviewed changes

uuf6429 added 2 commits January 7, 2025 15:53

Review fixes

9a9d3e2

Use getString instead

8e296ed

uuf6429 requested review from aik099 and stof January 7, 2025 15:02

aik099 requested changes Jan 7, 2025

View reviewed changes

tests/Basic/BasicAuthTest.php Show resolved Hide resolved

src/FixturesKernel.php Outdated Show resolved Hide resolved

Use old-symfony-compatible version

786d9b7

aik099 requested changes Jan 7, 2025

View reviewed changes

src/FixturesKernel.php Outdated Show resolved Hide resolved

Use correct getter

a1950ba

uuf6429 requested a review from aik099 January 8, 2025 17:46

aik099 approved these changes Jan 8, 2025

View reviewed changes

stof approved these changes Jan 13, 2025

View reviewed changes

aik099 merged commit d549552 into minkphp:master Jan 13, 2025
2 checks passed

uuf6429 deleted the chore/phpstan-improvements branch January 13, 2025 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPStan improvements #100

PHPStan improvements #100

uuf6429 commented Jan 5, 2025

stof Jan 6, 2025

uuf6429 Jan 6, 2025 •

edited

Loading

aik099 Jan 6, 2025

uuf6429 Jan 6, 2025 •

edited

Loading

uuf6429 Jan 6, 2025 •

edited

Loading

stof Jan 7, 2025

uuf6429 Jan 7, 2025 •

edited

Loading

stof Jan 6, 2025

uuf6429 Jan 6, 2025

stof Jan 7, 2025

uuf6429 Jan 7, 2025

stof Jan 6, 2025

uuf6429 Jan 6, 2025 •

edited

Loading

aik099 Jan 8, 2025

uuf6429 Jan 8, 2025

aik099 commented Jan 8, 2025

uuf6429 commented Jan 8, 2025 •

edited

Loading

aik099 commented Jan 9, 2025 •

edited

Loading

PHPStan improvements #100

PHPStan improvements #100

Conversation

uuf6429 commented Jan 5, 2025

Choose a reason for hiding this comment

uuf6429 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uuf6429 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

uuf6429 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uuf6429 Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uuf6429 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aik099 commented Jan 8, 2025

uuf6429 commented Jan 8, 2025 • edited Loading

aik099 commented Jan 9, 2025 • edited Loading

uuf6429 Jan 6, 2025 •

edited

Loading

uuf6429 Jan 6, 2025 •

edited

Loading

uuf6429 Jan 6, 2025 •

edited

Loading

uuf6429 Jan 7, 2025 •

edited

Loading

uuf6429 Jan 6, 2025 •

edited

Loading

uuf6429 commented Jan 8, 2025 •

edited

Loading

aik099 commented Jan 9, 2025 •

edited

Loading