POC: Speed optimize class-loading for levels tests #2916

staabm · 2024-02-14T11:20:16Z

level tests are the slowest part of the CI process and take a considerable amount of time.
they usually take arround something like 10 minutes

locally I have reduced rule level tests to a small subset to easy reproducing the problem.
my reduced set of rule-level tests before this PR run like

Warning:       No code coverage driver available

.                                                                   1 / 1 (100%)

Time: 01:12.819, Memory: 32.00 MB

after the change in autoload rules it runs like

Warning:       No code coverage driver available

.                                                                   1 / 1 (100%)

Time: 00:46.306, Memory: 24.00 MB

which is ~37% faster.

lets see wheter this speedup reproduces in CI and the change itself is acceptable

~~side-note: the PR as is currently yields to 2 notices when running phpunit, which obviously should be fixed~~
fixed by 11ffaa8

$ time vendor/bin/phpunit tests/PHPStan/Generics/GenericsIntegrationTest.php --group levels
PHP Warning:  Class "ReturnTypes\Foo" not found in C:\dvl\Workspace\phpstan-src-staabm\tests\phpstan-bootstrap.php on line 8

Warning: Class "ReturnTypes\Foo" not found in C:\dvl\Workspace\phpstan-src-staabm\tests\phpstan-bootstrap.php on line 8
PHP Warning:  Class "TestAccessProperties\FooAccessProperties" not found in C:\dvl\Workspace\phpstan-src-staabm\tests\phpstan-bootstrap.php on line 9

Warning: Class "TestAccessProperties\FooAccessProperties" not found in C:\dvl\Workspace\phpstan-src-staabm\tests\phpstan-bootstrap.php on line 9

staabm · 2024-02-14T12:55:21Z

In CI it seems we save ~2 minutes with this PR in Tests / Levels tests (reduced from previously ~10 minutes).

the approach of this PR is faster, because ComposerJsonAndInstalledJsonSourceLocatorMaker analyses PHPStan composer.json and needs to verify/check sha1 checksums for all files configured in composer autoloading. In RuleLevelTests we trigger a lot of phpstan processes which all have to re-verify the hash-sums of all files and therefore it gets slow.

With every autoloadable symbol added via a new test to PHPStan the rule-level test gets a tiny bit slower.

ondrejmirtes · 2024-02-14T18:50:04Z

.github/workflows/test-levels-composer.patch

+ 		"classmap": [
+-			"tests/e2e",
+-			"tests/PHPStan"
+			"tests/PHPStan/Rules/Methods/data/returnTypes.php",


I understand the intention, but I don't love it. It's going to be really hard to maintain this list and debug problems as time goes on.

I'd be more interested in parallelizing running the tests. Basically each "topic" in LevelsTestCase could have its own GitHub Actions job.

It's not exactly the same but the idea is here: https://rias.be/blog/running-phpunit-tests-in-parallel-using-github-actions

I have adjusted the PR. It now filters the existing tests based on class-name and builds a dynamic matrix with it.

this means we run now 40 tests in parallel (which each takes ~1m 15s) instead of one large job which runs ~10 minutes

the tests.yml github action workflow for this PR finished in 4m 15s, while it takes ~10m 30s before this PR 🚀

I understand the intention, but I don't love it. It's going to be really hard to maintain this list and debug problems as time goes on.

I'd be more interested in parallelizing running the tests. Basically each "topic" in LevelsTestCase could have its own GitHub Actions job.

It's not exactly the same but the idea is here: rias.be/blog/running-phpunit-tests-in-parallel-using-github-actions

This is very similar to what i did in psalm years ago:
vimeo/psalm#4985

But in psalm, each job also runs test in parallel.

@bendavies the tests jobs - even before this PR - are already highly parallelized - within the github action jobs and also across several jobs.

This PR splits up one very slow part of the pre-existing tests into even more jobs because these were particular slow

👍 apologies.
i couldn't see (still can't) where tests are split across jobs.

apologies

no worries.

i couldn't see (still can't) where tests are split across jobs.

see the screen in my above comment. there you can see the "levels test" matrix, with several jobs.
if you compare to other existing PR, this step is just a single job.

the PR implements this by inspecting the test-suite level-tests test-cases. Because of the used dataprovider, this means the testLevels method represents several test-cases. each of these test-cases is run in a separate github action job, because I feed this list of test-cases as a json array into the "test-levels" github action job matrix.

This reverts commit 11ffaa8.

This reverts commit 57bcbd2.

staabm · 2024-02-16T13:59:25Z

.github/workflows/tests-levels-matrix.php

@@ -0,0 +1,31 @@
+<?php
+
+$testList = shell_exec('php vendor/bin/phpunit --list-tests');


currently the --list-tests phpunit feature does not allow filtering by test-groups.

I have opened a feature request about that, so we may can get rid/simplify this script in the future.

sebastianbergmann/phpunit#5702

phpstan-bot · 2024-02-16T14:13:29Z

This pull request has been marked as ready for review.

ondrejmirtes

I really love this! Just found a few minor things :)

ondrejmirtes · 2024-02-17T13:48:00Z

.github/workflows/tests-levels-matrix.php

+
+	if (
+		!str_contains($cleanedLine, 'PHPStan\Generics\GenericsIntegrationTest') &&
+		!str_contains($cleanedLine, 'PHPStan\Levels\LevelsIntegrationTest')


We should check the @group inside the docblock instead of hardcoding these two classes.

to make this work based on the phpdoc, I would have needed to reflect on classes and read all class-comments.

instead I updated phpunit to 10.x for the matrix job and used --list-tests-xml which returns a xml containing group information. its a option which does not exist in phpunit 9.x

ondrejmirtes · 2024-02-17T13:48:46Z

.github/workflows/tests-levels-matrix.php

@@ -0,0 +1,33 @@
+<?php


declare strict types please

ondrejmirtes · 2024-02-17T13:49:02Z

.github/workflows/tests.yml

+        uses: "shivammathur/setup-php@v2"
+        with:
+          coverage: "none"
+          php-version: "8.1"


Let's do 8.3 here :)

ondrejmirtes · 2024-02-17T14:04:58Z

.github/workflows/tests.yml

@@ -121,7 +155,7 @@ jobs:
        run: "composer install --no-interaction --no-progress"

      - name: "Tests"
-        run: "make tests-levels"
+        run: "php vendor/bin/phpunit --filter '${{ matrix.test-filter }}' --group levels"


I just did some local testing and in this format PHPUnit will still execute all data providers from all tests. It would be good to also add the file in which the filtered test is. I think we can get even faster that way.

See the comparison:

time vendor/bin/phpunit --filter 'PHPStan\\Type\\UnionTypeTest::testSorting' 15.7s time vendor/bin/phpunit tests/PHPStan/Type/UnionTypeTest.php --filter 'PHPStan\\Type\\UnionTypeTest::testSorting' 1.1s

indeed, it got even faster now :)

ondrejmirtes · 2024-02-17T20:32:57Z

.github/workflows/tests-levels-matrix.php

+
+		$filter = str_replace('\\', '\\\\', $testCaseName);
+
+		$testFilters[] = sprintf("%s --filter '%s'", $fileName, $filter);


Why is doubling backslashes needed? I’d say that instead of that we need escapeshellarg() for both fileName and filter.

Running the plain command on the CLI only works only with double backslashes (in the filter arg)

See your command in #2916 (comment)

I guess we can add escapeshellarg(), but we still need this manual escaping

lets give it a try and we will see

https://github.com/phpstan/phpstan-src/actions/runs/7944377066/job/21689826256 shows that it doesn't work when we only use escapeshellarg (no tests executed).

Why is that? I noticed it doesn't work but I don't know what eats the characters, whether it's bash or PHP. Doubling the backslashes seems very peculiar to me, maybe there are other things we need to fix too but we're currently unaware of that.

Phpunit examples in docs also mention double backslashes

https://docs.phpunit.de/en/9.6/textui.html#textui-examples-filter-patterns

Its interpreted as regex. Let mit try preg_quote()

I know. It's probably a bash thing.

preg_quote would escape the following characters: . \ + * ? [ ^ ] $ ( ) { } = ! < > | : - #

the only one of these which can show up in a class/method name is \. I think we can use preg_quote if you prefer but it should be enough to escape the backslash IMO

edit: I had an idea

ondrejmirtes · 2024-02-18T08:31:10Z

Thank you.

staabm added 3 commits February 14, 2024 12:12

Speed optimize class-loading for levels tests

57bcbd2

Prevent unnecessary IO in ComposerJsonAndInstalledJsonSourceLocatorMaker

9d04fe6

fix warnings

11ffaa8

ondrejmirtes reviewed Feb 14, 2024

View reviewed changes

staabm added 6 commits February 16, 2024 13:48

Revert "fix warnings"

37e325c

This reverts commit 11ffaa8.

Revert "Speed optimize class-loading for levels tests"

4eb6d67

This reverts commit 57bcbd2.

Implement parallelized test levels via github actions matrix

030b8c0

separate test-levels-matrix job

b025c05

fix deprecation

45b12b0

fix typo

36f26cf

staabm commented Feb 16, 2024

View reviewed changes

staabm marked this pull request as draft February 16, 2024 14:05

backslash escapeing

9adf76d

staabm marked this pull request as ready for review February 16, 2024 14:13

staabm mentioned this pull request Feb 16, 2024

combine --list-tests with --group sebastianbergmann/phpunit#5702

Closed

ondrejmirtes requested changes Feb 17, 2024

View reviewed changes

staabm added 5 commits February 17, 2024 16:52

review feedback

947ddb7

typo

6185833

use list-tests-xml

5fe67ce

simplify

6dc056d

Update tests.yml

0bcd114

ondrejmirtes requested changes Feb 17, 2024

View reviewed changes

staabm added 2 commits February 17, 2024 22:26

use escapeshellarg()

b77b48a

added back escaping

f5aeead

ondrejmirtes merged commit 938321c into phpstan:1.10.x Feb 18, 2024
471 checks passed

staabm deleted the faster-level-tests branch February 18, 2024 08:34

staabm mentioned this pull request Feb 18, 2024

Use preg_qoute() escaping for --filter phpunit option #2923

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POC: Speed optimize class-loading for levels tests #2916

POC: Speed optimize class-loading for levels tests #2916

staabm commented Feb 14, 2024 •

edited

staabm commented Feb 14, 2024 •

edited

ondrejmirtes Feb 14, 2024

staabm Feb 16, 2024 •

edited

bendavies Feb 17, 2024 •

edited

staabm Feb 17, 2024

bendavies Feb 17, 2024 •

edited

staabm Feb 17, 2024 •

edited

staabm Feb 16, 2024 •

edited

phpstan-bot commented Feb 16, 2024

ondrejmirtes left a comment

ondrejmirtes Feb 17, 2024

staabm Feb 17, 2024 •

edited

ondrejmirtes Feb 17, 2024

ondrejmirtes Feb 17, 2024

ondrejmirtes Feb 17, 2024

staabm Feb 17, 2024 •

edited

ondrejmirtes Feb 17, 2024

staabm Feb 17, 2024 •

edited

staabm Feb 17, 2024 •

edited

staabm Feb 17, 2024

staabm Feb 17, 2024 •

edited

ondrejmirtes Feb 18, 2024

staabm Feb 18, 2024

staabm Feb 18, 2024

ondrejmirtes Feb 18, 2024

staabm Feb 18, 2024 •

edited

ondrejmirtes commented Feb 18, 2024

		@@ -0,0 +1,31 @@
		<?php

		$testList = shell_exec('php vendor/bin/phpunit --list-tests');


		$filter = str_replace('\\', '\\\\', $testCaseName);

		$testFilters[] = sprintf("%s --filter '%s'", $fileName, $filter);

POC: Speed optimize class-loading for levels tests #2916

POC: Speed optimize class-loading for levels tests #2916

Conversation

staabm commented Feb 14, 2024 • edited

staabm commented Feb 14, 2024 • edited

Choose a reason for hiding this comment

staabm Feb 16, 2024 • edited

Choose a reason for hiding this comment

bendavies Feb 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendavies Feb 17, 2024 • edited

Choose a reason for hiding this comment

staabm Feb 17, 2024 • edited

Choose a reason for hiding this comment

staabm Feb 16, 2024 • edited

Choose a reason for hiding this comment

phpstan-bot commented Feb 16, 2024

ondrejmirtes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm Feb 17, 2024 • edited

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

staabm Feb 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm Feb 17, 2024 • edited

Choose a reason for hiding this comment

staabm Feb 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm Feb 17, 2024 • edited

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

staabm Feb 18, 2024 • edited

Choose a reason for hiding this comment

ondrejmirtes commented Feb 18, 2024

staabm commented Feb 14, 2024 •

edited

staabm commented Feb 14, 2024 •

edited

staabm Feb 16, 2024 •

edited

bendavies Feb 17, 2024 •

edited

bendavies Feb 17, 2024 •

edited

staabm Feb 17, 2024 •

edited

staabm Feb 16, 2024 •

edited

staabm Feb 17, 2024 •

edited

staabm Feb 17, 2024 •

edited

staabm Feb 17, 2024 •

edited

staabm Feb 17, 2024 •

edited

staabm Feb 17, 2024 •

edited

staabm Feb 18, 2024 •

edited