Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

ParaTest: leverage WrapperRunner for quicker tests #1115

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

Merged
ondrejmirtes merged 6 commits into phpstan:1.5.x from Slamdunk:paratest_wrapperrunner
Mar 27, 2022
Merged

ParaTest: leverage WrapperRunner for quicker tests #1115

ondrejmirtes merged 6 commits into phpstan:1.5.x from Slamdunk:paratest_wrapperrunner
Mar 27, 2022

Conversation

@Slamdunk
Copy link
Contributor

@Slamdunk Slamdunk commented Mar 25, 2022

Hi, opened this PR as draft to see if the Windows builds get a significant improvement.

staabm and herndlm reacted with heart emoji
Copy link
Contributor

staabm commented Mar 25, 2022

I see fatal erros in tests I recently added. It looks like these should use separate php namespaces (they overlap across files atm)

Copy link
Contributor Author

Yup, working on it. In the meantime, results are encouraging:

Before After
image image

Copy link
Contributor

staabm commented Mar 25, 2022

Yep, I also saw these numbers. Looks really nice

Copy link
Contributor Author

@staabm I'm having trouble fixing the class name collision introduced in #1082

Could you have a look at it please while I'm figuring out the paratest error on Win+PHP7.2?

The short command to raise the error is:

vendor/bin/phpunit --filter '/(value-of-enum|testValueOfEnum)/' --debug
staabm reacted with thumbs up emoji

Copy link
Contributor

staabm commented Mar 25, 2022

I think the fix should look like 2e2c62c

Copy link
Contributor Author

I've tried many iterations like that, but still it seems a require is being done twice :\

Copy link
Contributor

staabm commented Mar 25, 2022

Copy link
Contributor Author

Already tried that too, unsuccessfully :\

Copy link
Contributor

staabm commented Mar 25, 2022
edited
Loading

hmmm all green on my end? - see only a different problem in "old PHPUnit" jobs

#1117

Copy link
Contributor Author

Not reliable: WrapperRunner is not deterministic, the two colliding tests can randomly get executed in the same proc or in different procs.

Try that PR locally with the aforementioned command, you'll see it failing:

$ vendor/bin/phpunit --filter '/(value-of-enum|testValueOfEnum)/' --debug
PHP Fatal error: Cannot declare class ValueOfEnumPhpdoc\Country, because the name is already in use in ./phpstan-src/tests/PHPStan/Rules/PhpDoc/data/value-of-enum.php on line 7

Copy link
Contributor

staabm commented Mar 27, 2022

Try that PR locally with the aforementioned command, you'll see it failing:

I can reproduce the error locally. with the changes of #1119 the test-suite runs cleanly on my machine.

unrelated question: I am wondering why paratest uses just 6 workers on my laptop, even though it can run 12 threads (6 cores, 12 SMT threads)

Copy link
Member

I'm blown away by this! On my local machine running the tests just went down from 18s to 11s!

staabm reacted with rocket emoji

Copy link
Member

I'm gonna go ahead and merge it, thank you :)

@ondrejmirtes ondrejmirtes marked this pull request as ready for review March 27, 2022 15:47
@ondrejmirtes ondrejmirtes merged commit 734f645 into phpstan:1.5.x Mar 27, 2022
Copy link
Member

Hi, could you please work out why it doesn't finish on PHP 7.2 on Windows? I reworked how the patch is applied (d99708f) but that shouldn't cause an issue. Thank you.

Copy link
Member

Copy link
Member

I'm trying if the original works or not: #1130

Copy link
Member

Didn't help, decided to do this for now: a95ce30

Copy link
Contributor Author

It always times out after 60 minutes

Yup, I saw that, I was planning to tackle it this week 👍

Copy link
Member

Alright, you can start by reverting the commit that removes testing on Windows 😊 Thanks.

Copy link
Member

But don't sink too much time into it, it's not worth it 😊 Thanks.

Copy link
Contributor Author

Ok then.

Worth mentioning that I'm the ParaTest maintainer: ping me if you need further help on this topic.

@Slamdunk Slamdunk deleted the paratest_wrapperrunner branch March 28, 2022 07:42
Copy link
Contributor Author

Before:
image
After:
image

🚀

Copy link
Member

An idea - we could bump PHPUnit to 8.5 if there was a compatible ParaTest version that would work on PHP 7.2. Don't know if it's solve anything though.

Copy link
Contributor

staabm commented Mar 28, 2022

maybe we could just invoked paratest without --runner WrapperRunner on php72 ?

Copy link
Member

I've got the same idea but since I've already got an appetite for a faster pipeline like this, I rather removed testing on Windows on 7.2.

staabm reacted with laugh emoji

Copy link
Contributor Author

An idea - we could bump PHPUnit to 8.5 if there was a compatible ParaTest version that would work on PHP 7.2. Don't know if it's solve anything though.

When I became the main maintainer, ParaTest already had all the good ideas and stuff wrote, but with very poor CC, bad overall design and zero static analysis. I had to make sharp turns to unburden maintenance costs, so backporting would be very costly.
7.2 seems to go pretty well on Ubuntu: if you are ok to not test against Windows on PHP 7.2, you would ease my life a lot.

Starting from ParaTest 6, I can guarantee pretty decent support even in the future though, for every platform PHPUnit supports too.

Copy link
Member

Yes, the current status is fine, thank you :) It's my choice to support older PHP versions, I don't want to forward that burden to anyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /