-
Notifications
You must be signed in to change notification settings - Fork 13
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
Do not extend final class TestRunner
and just copy-and-paste its contents
#8
Conversation
It looks like Travis checks are not running. Can you check if the authentication has to be renewed? |
I learned about phpunit-slicer from your issue over at sebastianbergmann/phpunit#4121 I've been using this PR (git commit hash 306846a) a few weeks now since moving over to Github Actions in a private repo and it's category "game changer" in terms of cutting down the runtime (~10k tests => went down from initial 2 suites ~15 mins to 6 slices done in ~4-5 mins). I already tried to come up with something better but ran into all the problems you probably did too and would have ended up copying the file over 🤷♀️ I lack the knowledge of phpunit internals (and a bit time) to dig into that. But it seems phpunit is on a pace to seal many of it's openness in order to reduce complexity and too-creative-use on users side. Likely due to all the petty support issues to have to deal with (just a speculation, reading their issues for a while gives me the impression a bit). |
Couldn't upgrade to phpunit9 due to version constraint in composer.json Have not yet found time (will try later) but anyone already knowing if it "just works" or not? |
I got it working but I was basically doing the same copypaste galore you already did but with PHPUnit9 code 😄 It becomes pretty clear that any package trying to do this has to be creative because the way phpunit keeps changing currently you can't simply have a single code base support all those versions. Either you jump through hoops with PHPunit version checks or Of course only now I realized that @spawnia was already working on this in https://github.com/mll-lab/phpunit-slicer/commits/phpunit-9 but not PR Not sure who runs the repo, last commits are from @wizacedric and @philippe-vandermoere : are you still interesting in keeping the repo running for newer versions, any thoughts on the matter? |
@mfn quick heads up, the PHPUnit 9 branch only works with version 9.2.5 (potentially lower, have not tried that). 9.2.6 breaks things again. I locked the requirement to make sure https://github.com/mll-lab/phpunit-slicer/commits/phpunit-9 keeps working for now. |
@spawnia thanks! I did not notice anymore as I assimilated the slicer in a private code repo already with 9.2.6 . Call that luck 😅 So we would end up continuously having to copy/paste adapt the two classes… When I assimilated the code (I need to move fast here) it turns out it's not THAT bad in this bad situation:
So every time the release a new version it's the same over and over again… I can see myself patching this for a company driven project but not maintaining in an OSS project when other people start to depend on this, as I'll just schedule the upgrades when I've really time for this. Damn :) |
@spawnia @mfn Thanks for both your inputs. I don't think we can accept the PR as it is. It would create a hard link between a version of phpunit/phpunit and a version of wizaplace/phpunit-slicer, which would be a problem for most users. We would need to have as many versions of wizaplace/phpunit-slicer as there are versions of phpunit/phpunit... |
Can https://phpunit.readthedocs.io/en/9.2/extending-phpunit.html#extending-the-testrunner be a way to move forward on this? |
@wizacedric PHPUnit brought us into this mess by locking down their implementation, adding
Maybe using According to sebastianbergmann/phpunit#4121, PHPUnit would most likely add a solution that allows extensions to control PHPUnit test selection through an XML export/import.
Agree it is problematic, still better than no solution at all.
Not totally true, we would have to cover more specific ranges, down to the minor version. Looks like @mfn and me maintain something like this on the side anyways. It is a bit ugly, but not too much of a hassle. Might as well join efforts. |
I gave it a cursory look so I might be wrong, but the hook doesn't even receive the test lest can return a value or signal in any way how to proceed or not. Probably would require calling The whole extending phpunit chapter, whilst looking nice from above (hooks everywhere), since you basically can't "control" anything isn't really useful for us. I could see a hacky way using global state somewhere, calculating the slice based on counting the test executed (hopefully tests are always run in the same order…) and throw a I've a >10k test suite, if I make 5 slices this means I'll 40k times throw this error to skip the tests… nah, I don't think I'll even attempt this. Want to see a "funny" coincidence?
I can hardly believe (see the timestamps involved) this is a coincidence.
Agree, I made #9 to show the intent I had in mind. It's still copypasta galore, so no real improvement over the status quo. |
I've yet to look further but I wonder if the concept of test filters in phpunit can be better (ab)used to "slice", see https://github.com/sebastianbergmann/phpunit/blob/ff047828b43b7ba88300372fb41943ddceb2db03/src/Runner/Filter/Factory.php#L50-L60 🤷♀️ |
How about we give in and try to get something like sebastianbergmann/phpunit#3387 implemented in PHPUnit? |
Yeah I remember it was mentioned in sebastianbergmann/phpunit#4121 (comment) ;) Btw. this issue links to sebastianbergmann/phpunit#3605 which I wasn't really aware of And I thought "aha, 'phpunit chunk'" and voila https://www.google.com/search?hl=en&q=phpunit%20chunk => https://github.com/jwage/phpchunkit Not sure if @jwage is still active in this area? |
After having to go through the pain recently again of adapting sources for PHPUnit 9.3, I finally gave it a stab at sebastianbergmann/phpunit#4449 |
I'm happy (…) to report that the copypaste approach still works with PHPUnit 9.5 :} I tried #10 but I couldn't get it working, left comments there. Only now I saw I got feedback in my phpunit PR which I was not ware of, will look at this ASAP too. |
Since the signs are not good that the copypaste approach will continue to work as phpunit is changing their internals faster then I change my underwear and sebastianbergmann/phpunit#4449 also doesn't show any movement, I devised a new strategy. I don't think I'm the first to come up with this but TBH I have not seen this solution somewhere else:
This sounds involved, but it's just a few lines added to e.g. Github Action step and presto, you can get almost the same benefit as from jobs:
phpunit:
strategy:
matrix:
phpunit-slices: ['1/6', '2/6', '3/6', '4/6', '5/6', '6/6']
steps:
# … other steps before
- name: 'phpunit: export all tests as XML'
run: vendor/bin/phpunit --list-tests-xml all_tests.xml
- name: 'phpunit: slice tests ${{ matrix.phpunit-slices }}'
run: phpunit_xml_slicer.php all_tests.xml ${{ matrix.phpunit-slices }} > slice.xml
- name: 'phpunit: convert classes to files and inject them back into phpunit XML config'
run: phpunit_xml_class_to_file.php composer.json phpunit.xml.dist slice.xml > phpunit-ci.xml
- run: vendor/bin/phpunit --configuration phpunit-ci.xml Biggest difference
I had concerns that this would create an imbalance of my test suite I'm testing this with (~15k tests) so that some slices would run much longer than others, but turns out in practice the difference was not noticeable. Might me a lucky case for me though, YMMV. Why?!I was exploring https://laravel.com/docs/8.x/testing#running-tests-in-parallel the other day and I could not really use What about the phpunit PR you mentioned?sebastianbergmann/phpunit#4449 Not sure when this progresses, but this would be the ideal case for a solution her:
Let's see I spent already way more on these kind of topics I ever thought I would, happy to hear about other perspectives / solutions / approaches 😄 |
@spawnia it's PHP, and its open source. I'm facing the same problem as yours. Long run time and in-consistent test results, make me nuts. for the time being, |
This is definitely a hack and a maintenance nightmare - but it works for now.
We can look for a better alternative later, see #7