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.

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

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

Merged
merged 17 commits into from
Feb 18, 2024
Merged
32 changes: 32 additions & 0 deletions .github/workflows/tests-levels-matrix.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php declare(strict_types = 1);

shell_exec('php vendor/bin/phpunit --list-tests-xml test-list.xml');

$simpleXml = simplexml_load_file('test-list.xml');
if ($simpleXml === false) {
throw new RuntimeException('Error loading test-list.xml');
}

$testFilters = [];
foreach($simpleXml->testCaseClass as $testCaseClass) {
foreach($testCaseClass->testCaseMethod as $testCaseMethod) {
if ((string) $testCaseMethod['groups'] !== 'levels') {
continue;
}

$testCaseName = (string) $testCaseMethod['id'];

[$className, $testName] = explode('::', $testCaseName, 2);
$fileName = 'tests/'. str_replace('\\', DIRECTORY_SEPARATOR, $className) . '.php';

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@staabm staabm Feb 17, 2024

Choose a reason for hiding this comment

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

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

See your command in #2916 (comment)

Copy link
Contributor Author

@staabm staabm Feb 17, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets give it a try and we will see

Copy link
Contributor Author

@staabm staabm Feb 17, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Phpunit examples in docs also mention double backslashes

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its interpreted as regex. Let mit try preg_quote()

Copy link
Member

Choose a reason for hiding this comment

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

I know. It's probably a bash thing.

Copy link
Contributor Author

@staabm staabm Feb 18, 2024

Choose a reason for hiding this comment

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

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

}
}

if ($testFilters === []) {
throw new RuntimeException('No tests found');
}

echo json_encode($testFilters);
36 changes: 35 additions & 1 deletion .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,45 @@ jobs:
- name: "Tests"
run: "make tests-integration"

tests-levels-matrix:
name: "Determine levels tests matrix"
runs-on: ubuntu-latest
timeout-minutes: 60

steps:
- name: "Checkout"
uses: actions/checkout@v3

- name: "Install PHP"
uses: "shivammathur/setup-php@v2"
with:
coverage: "none"
php-version: "8.3"
tools: pecl
extensions: ds,mbstring
ini-file: development
ini-values: memory_limit=1G

- name: "Install PHPUnit 10.x"
run: "composer remove --dev brianium/paratest && composer require --dev --with-all-dependencies phpunit/phpunit:^10"

- id: set-matrix
run: echo "matrix=$(php .github/workflows/tests-levels-matrix.php)" >> $GITHUB_OUTPUT

outputs:
matrix: ${{ steps.set-matrix.outputs.matrix }}

tests-levels:
needs: tests-levels-matrix

name: "Levels tests"
runs-on: ubuntu-latest
timeout-minutes: 60

strategy:
matrix:
test-filter: "${{fromJson(needs.tests-levels-matrix.outputs.matrix)}}"

steps:
- name: "Checkout"
uses: actions/checkout@v3
Expand All @@ -121,7 +155,7 @@ jobs:
run: "composer install --no-interaction --no-progress"

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

tests-old-phpunit:
name: "Tests with old PHPUnit"
Expand Down
Original file line number Diff line number Diff line change
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