Skip to content
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

Jump to bottom

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

Merged
merged 17 commits into from Feb 18, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
57bcbd2
Speed optimize class-loading for levels tests
staabm Feb 14, 2024
9d04fe6
Prevent unnecessary IO in ComposerJsonAndInstalledJsonSourceLocatorMaker
staabm Feb 14, 2024
11ffaa8
fix warnings
staabm Feb 14, 2024
37e325c
Revert "fix warnings"
staabm Feb 16, 2024
4eb6d67
Revert "Speed optimize class-loading for levels tests"
staabm Feb 16, 2024
030b8c0
Implement parallelized test levels via github actions matrix
staabm Feb 16, 2024
b025c05
separate test-levels-matrix job
staabm Feb 16, 2024
45b12b0
fix deprecation
staabm Feb 16, 2024
36f26cf
fix typo
staabm Feb 16, 2024
9adf76d
backslash escapeing
staabm Feb 16, 2024
947ddb7
review feedback
staabm Feb 17, 2024
6185833
typo
staabm Feb 17, 2024
5fe67ce
use list-tests-xml
staabm Feb 17, 2024
6dc056d
simplify
staabm Feb 17, 2024
0bcd114
Update tests.yml
staabm Feb 17, 2024
b77b48a
use escapeshellarg()
staabm Feb 17, 2024
f5aeead
added back escaping
staabm Feb 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions .github/workflows/test-levels-composer.patch
View file
Open in desktop
@@ -0,0 +1,18 @@
diff --git a/composer.json b/composer.json
index 6dde20b5f..7ea244ee1 100644
--- a/composer.json
+++ b/composer.json
@@ -116,8 +116,11 @@
]
},
"classmap": [
- "tests/e2e",
- "tests/PHPStan"
+ "tests/PHPStan/Rules/Methods/data/returnTypes.php",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@staabm staabm Feb 16, 2024 •
edited

Choose a reason for hiding this comment

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

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

grafik

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

Copy link
Contributor

@bendavies bendavies Feb 17, 2024 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@bendavies bendavies Feb 17, 2024 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@staabm staabm Feb 17, 2024 •
edited

Choose a reason for hiding this comment

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

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.

+ "tests/PHPStan/Rules/Methods/data/returnTypes-defined.php",
+ "tests/PHPStan/Rules/Properties/data/access-properties.php",
+ "tests/PHPStan/Generics",
+ "tests/PHPStan/Levels"
]
},
"minimum-stability": "dev",
3 changes: 3 additions & 0 deletions .github/workflows/tests.yml
View file
Open in desktop
Expand Up @@ -117,6 +117,9 @@ jobs:
ini-file: development
ini-values: memory_limit=1G

- name: "Speed optimize class-loading for levels tests"
run: "patch -b composer.json .github/workflows/test-levels-composer.patch"

- name: "Install dependencies"
run: "composer install --no-interaction --no-progress"

Expand Down
1 change: 1 addition & 0 deletions ...lection/BetterReflection/SourceLocator/ComposerJsonAndInstalledJsonSourceLocatorMaker.php
View file
Open in desktop
Expand Up @@ -112,6 +112,7 @@ public function create(string $projectInstallationPath): ?SourceLocator
foreach ($classMapPaths as $classMapPath) {
if (is_dir($classMapPath)) {
$locators[] = $this->optimizedDirectorySourceLocatorRepository->getOrCreate($classMapPath);
continue;
}
if (!is_file($classMapPath)) {
continue;
Expand Down