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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

DX: Use ParaUnit to speed up tests #6883

Merged
merged 5 commits into from Jul 8, 2023

Conversation

Wirone
Copy link
Member

@Wirone Wirone commented Apr 4, 2023

Currently test suites in Github Actions take ages. This PR's goal is reducing time needed for running test suite(s).

Initially started with ParaTest, but after encountering problems with test loader I've changed to ParaUnit.

Benchmark:

Process Lint Fast Lint
PHPUnit 00:26:54 馃槱 00:00:56
ParaUnit 00:04:48 00:00:28 鉂わ笍

@Wirone Wirone changed the title PoC: Use ParaTest to speed up tests DX: [PoC] Use ParaTest to speed up tests Apr 4, 2023
@Wirone

This comment was marked as outdated.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@Wirone

This comment was marked as outdated.

@Wirone

This comment was marked as outdated.

@Wirone
Copy link
Member Author

Wirone commented Jun 29, 2023

FYI: Test execution comparison, run locally on MacBook Pro M1, without smoke tests (466 test classes, 33572 tests):

Process Lint Fast Lint
PHPUnit 00:26:54 馃槱 00:00:56
ParaUnit 00:04:48 00:00:28 鉂わ笍

ParaUnit is 2x faster than regular PHPUnit using Fast Linter (FAST_LINT_TEST_CASES=1), and over 5x faster using Process Linter. 5 minutes of whole suite is actually something that can be run locally without much problem, but 27 minutes? Naaahhhhh.

@Wirone Wirone changed the title DX: [PoC] Use ParaTest to speed up tests DX: Use ParaUnit to speed up tests Jun 29, 2023
@Wirone

This comment was marked as outdated.

@Wirone

This comment was marked as outdated.

@Wirone

This comment was marked as outdated.

@Wirone Wirone marked this pull request as ready for review July 2, 2023 22:42
@Wirone Wirone self-assigned this Jul 2, 2023
@Wirone Wirone marked this pull request as draft July 3, 2023 07:58
@Wirone
Copy link
Member Author

Wirone commented Jul 3, 2023

FYI: about 462 files with NO TESTS EXECUTED, example with --debug:

PROCESS STARTED: /app/tests/RuleSet/Sets/PERRiskySetTest.php
'php' '/app/vendor/phpunit/phpunit/phpunit' '--configuration=/app/phpunit.xml' '--extensions=Paraunit\Parser\JSON\TestHook\BeforeTest,Paraunit\Parser\JSON\TestHook\Error,Paraunit\Parser\JSON\TestHook\Failure,Paraunit\Parser\JSON\TestHook\Incomplete,Paraunit\Parser\JSON\TestHook\Risky,Paraunit\Parser\JSON\TestHook\Skipped,Paraunit\Parser\JSON\TestHook\Successful,Paraunit\Parser\JSON\TestHook\Warning' '--group=auto-review' '/app/tests/RuleSet/Sets/PERRiskySetTest.php'


PROCESS TERMINATED: /app/tests/PregTest.php
 - with class name: N/A

PROCESS FULL OUTPUT:

PHPUnit 9.6.9 by Sebastian Bergmann and contributors.

No tests executed!

This test is not a part of auto-review group. So this is just difference between how PHPUnit and ParaUnit prepares test set - former filters actual set before running, while the latter runs everything but for some classes gets filtered result from the upstream.

Let's sum it up:

  • 471 test classes processed
  • 462 files with NO TESTS EXECUTED
  • 9 test classes with @group auto-review

Everything is correct 馃檪. It's not optimal, since 98% of test classes in Auto Review job are run for nothing, but the result is OK. Same with tests mentioned here - these 2 classes are from group covers-nothing which is excluded from code coverage.

@Wirone Wirone marked this pull request as ready for review July 3, 2023 08:23
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved

- name: Collect code coverage
if: matrix.calculate-code-coverage == 'yes'
run: vendor/bin/paraunit coverage --testsuite coverage --exclude-group covers-nothing --clover build/logs/clover.xml
Copy link
Member

Choose a reason for hiding this comment

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

did you tried to compare results of coverage generated by PHPUnit and by paraunit? what's the difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I did not verify it, but I'll do it 馃憤.

Copy link
Member Author

@Wirone Wirone Jul 3, 2023

Choose a reason for hiding this comment

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

In terms of coverage it looks like everything works as it should:

Copy link
Member

Choose a reason for hiding this comment

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

i have the feeling that number should not be different.

Copy link
Contributor

Choose a reason for hiding this comment

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

The number is different because the code is having global state, e.g. https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/v3.21.1/src/Tokenizer/CT.php#L99-L104 - running it with PHPUnit make that if covered (or not covered) all the time with the same result, ParaUnit make it non-deterministic.

If you run PHPUnit like ./vendor/bin/phpunit --order-by=random you will get slightly different coverage for each run.

@keradus
Copy link
Member

keradus commented Jul 3, 2023

I find this very confusing. Maybe we could at least document it in codebase (not only in PR) ?

numbers diff is impressive 馃憤馃徎

@@ -40,6 +40,7 @@
"symfony/stopwatch": "^5.4 || ^6.0"
},
"require-dev": {
"facile-it/paraunit": "^1.3 || ^2.0",
Copy link
Member

Choose a reason for hiding this comment

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

tbh, I never heard about it.

when thinking about parallel execution of PHPUnit, somehow I always had in mind https://github.com/paratestphp/paratest , and to my understanding maintainer of it, Slamdunk, is also more close to PHPUnit itself.

would you mind to share this and that about 2 solutions and arguments for decision?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, Paraunit creator here. Paratest was in disuse when I created Paraunit, but Slamdunk took it over later and revived it. Now it has been used into Laravel, hence the growing popularity.

The main difference between the two tools is basically in the approach: Paratest is "hand on", uses @internal components of PHPUnit and has a tighter integration with it (hence some additional capabilities like being able to hook into specific output formats or being able to filter by groups); Paraunit is "hands off", just executes PHPUnit from the outside, hence the simpler integration but less functionality.

BTW I know Slamdunk and he even invited me to co-maintaint Paratest, but I have to admit I'm not so well versed in it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the details!

side note: yeah, it could be interesting to have the parallel executor available, instead of few possibilities. I could suggest one step further - consider to incorporate it to PHPUnit natively [I know Sebastian wanted to have some internal cleanups first, but if I recall right, lot of them already in place]

@Wirone
Copy link
Member Author

Wirone commented Jul 3, 2023

I find this very confusing. Maybe we could at least document it in codebase (not only in PR) ?

@keradus let's finish #6839, then such change would be transparent to users because Composer script would switch from PHPUnit to ParaUnit and everything would work as we expected 馃檪. This is advantage of that PR, which I'm fighting for for months 馃槅.

@Wirone Wirone requested a review from keradus July 3, 2023 22:01
Symfony bumped minimal PHP version in 6.1 and to satisfy ParaTest we need to enforce it too.
See issue 749 in `paratestphp/paratest` (link removed in order to not pollute issue with PR references).
@keradus keradus merged commit 7d34e69 into PHP-CS-Fixer:master Jul 8, 2023
15 checks passed
@Wirone Wirone deleted the codito/paratest branch July 8, 2023 23:35
@coveralls
Copy link

Pull Request Test Coverage Report for Build 5474591896

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+1.3%) to 94.521%

Files with Coverage Reduction New Missed Lines %
src/Console/Output/ErrorOutput.php 2 92.42%
Totals Coverage Status
Change from base Build 5474542201: 1.3%
Covered Lines: 24102
Relevant Lines: 25499

馃挍 - Coveralls

mvorisek pushed a commit to mvorisek/PHP-CS-Fixer that referenced this pull request Jul 28, 2023
mvorisek pushed a commit to mvorisek/PHP-CS-Fixer that referenced this pull request Jul 28, 2023
mvorisek pushed a commit to mvorisek/PHP-CS-Fixer that referenced this pull request Jul 28, 2023
@Wirone Wirone added the topic/ci Github Actions, tooling label Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/ci Github Actions, tooling topic/tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants