Use Paratest in GitHub Action #1346
Conversation
There was a problem hiding this comment.
When I execute it locally I get at least this error:
Processes: 12
Runtime: PHP 8.3.15
Configuration: /mbin-repo/phpunit.xml.dist............................................................. 61 / 1007 ( 6%)
............................................................. 122 / 1007 ( 12%)
............................................................. 183 / 1007 ( 18%)
............................................................. 244 / 1007 ( 24%)
............................................................. 305 / 1007 ( 30%)
............................................................. 366 / 1007 ( 36%)
............................................................. 427 / 1007 ( 42%)
............................................................. 488 / 1007 ( 48%)
............................................................. 549 / 1007 ( 54%)
............................................................. 610 / 1007 ( 60%)
............................................................. 671 / 1007 ( 66%)
............................................................. 732 / 1007 ( 72%)
............................................................. 793 / 1007 ( 78%)
............................................................. 854 / 1007 ( 84%)
...........F................................................. 915 / 1007 ( 90%)
............................................................. 976 / 1007 ( 96%)
............................... 1007 / 1007 (100%)Time: 02:48.961, Memory: 84.00 MB
There was 1 failure:
- App\Tests\Functional\Controller\Entry\EntryEditControllerTest::testAuthorCanEditOwnEntryImage
Failed asserting that null is not null./mbin-repo/tests/Functional/Controller/Entry/EntryEditControllerTest.php:100
FAILURES!
Tests: 1007, Assertions: 18730, Failures: 1.
As I said there are some flaky tests. If you didn't try to run it locally please do that and try it a bunch of times to check if there still are some flaky tests
|
I ran it inside my Codespaces container and got no errors. If this is only happening in the Docker setup, I don’t think that should be a blocker if it runs fine elsewhere. Also, please note that this change only affects the GitHub Action (where it also runs fine, as you can see here) and nothing else. You can still run tests the normal way in any environment. |
garrettw
added
enhancement
|
If it fails on my PC (likely because of the higher thread count) that only highlights that the test code is not thread-safe, which it should be. If it is not it is only a matter of time until a test fails in the workflow |
|
I tried a bunch of things now, but the most I got was 2 consecutive ok runs. The The bottom line is that the tests are sadly not thread safe. Most of the issues have to do with image uploads, but not all of them do. In my mind all tests would have to pass like 100 to 1000 times without any failures to count as thread safe. If you want to pursue this (which I would encourage) I can push a few fixes I already did to this branch. That is up to you though, as I understand (and feel the same way myself) that it is rather tedious work |
|
Ah, I did forget to try the |
|
I have an idea on how to still leverage paratest. We can put all non thread-safe tests in a group and then run that group with phpunit and afterwards run all thread-safe tests with paratest. The practice of identifying those tests is very annoying and time consuming I assume, but 🤷 |
|
If you want I can do it, but that will take time |
…h phpunit - create a PHPUnit group `NonThreadSafe` which marks tests that should only be serially executed with PHPUnit and not with paratest - create a new task in the action.yaml for executing tests with paratest - only execute tests in the group `NonThreadSafe` with PHPUnit
| @@ -101,17 +101,27 @@ jobs: | |||
| DATABASE_PORT: 5432 | |||
| REDIS_HOST: valkey | |||
| REDIS_PORT: 6379 | |||
| run: php bin/phpunit tests/Unit | |||
| run: php vendor/bin/paratest --passthru-php="'-d' 'memory_limit=192M'" tests/Unit | |||
There was a problem hiding this comment.
is the memory limit increase needed for a unit test? no right? You might want to add it to the functional test I guess..?
There was a problem hiding this comment.
why would the unit test even require a database connection.. I know it might be off-topic for this specific PR, but I don't believe our unit tests are correctly stubbing the Database calls. Which an unit test should do actually
There was a problem hiding this comment.
Because they use the same test setup which needs a db connection. We could maybe fix that, but I just ignored to get the tests back up and running.
The memory passthrough might not be needed at all, but I included it, because @garrettw included in his initial PR
Integration test step is now down from 23 mins to 9.5 mins on the GitHub Action.