php artisan test super slow after updating to v10.34.2 from v10.33.0 #49243
Comments
|
@nshiro since you sent in that PR, what are your thoughts here? |
|
Additionally which may help...
|
|
@sts-ryan-holton can you verify if your tests using |
|
@driesvints |
|
<?php
namespace Database\Factories;
use Carbon\Carbon;
use Illuminate\Database\Eloquent\Factories\Factory;
use Illuminate\Support\Facades\Hash;
class UserFactory extends Factory
{
/**
* Define the model's default state.
*/
public function definition(): array
{
return [
'timezone' => 'UTC',
'first_name' => $this->faker->firstName(),
'last_name' => $this->faker->lastName(),
'email' => $this->faker->unique()->safeEmail(),
'email_verified_at' => Carbon::now(),
'password' => Hash::make('password', [
'rounds' => 5, // only for dummy users, do not use this value for production.
]),
'country_code' => 'GB',
'phone_number' => '+447900000000',
'prefers_time_sensitive_notifications' => false,
'notify_on_days' => [1, 2, 3, 4, 5, 6, 7],
'notify_time_slot' => 'anytime',
'datetime_format' => 'HH:mm, MMM Do YYYY',
'on_holiday_until' => null
];
}
/**
* Indicate that the model's email address should be unverified.
*/
public function unverified(): Factory
{
return $this->state(function (array $attributes) {
return [
'email_verified_at' => null,
];
});
}
}No difference setting |
|
@sts-ryan-holton could you pinpoint the exact version where things went slow for you? |
|
@driesvints Sure thing, I'll try my best :) Basically, I update my project once a week with dependencies. Currently, on v10.34.2 with the From: "laravel/framework": "10.34.2"To (downgrade): "laravel/framework": "10.33.0"Without changing anything else in my project and running Additionally, I even tried individual versions of the framework since that release, here's my results: |
|
@sts-ryan-holton have you tried simply reverting this change in your vendor directory and seeing if it helps? Can you also share your entire test file? What database traits are you using? |
|
It should be one of these PR's then. Would appreciate any help in figuring out which one it is: https://github.com/laravel/framework/releases/tag/v10.34.0 |
|
Thank you for reporting this issue! As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub. If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team. Thank you! |
|
@sts-ryan-holton in addition, it would probably be helpful if you could create a fresh Laravel application that recreates the issue in the test suite and push it to GitHub. Then we can clone it down and actually try it locally. |
|
I'll take a look now |
|
@driesvints @taylorotwell Okay, just tried commenting out: Removing those and reverting back to the original makes no difference in performance. For this reason then, unless anything else has changed, I don't think that particular PR is responsible for the significant performance bottleneck, but, it stood out as it's related to testing. Another PR out of that list must be responsible as I have also just tried again reverting downwards and instantly get the speed up. |
|
@sts-ryan-holton it may be the transaction related PR - are you able to create a test repo for us to clone down? |
|
@taylorotwell Is there a way I can revert that change locally to see if it is causing the problem? Creating a test repo might take longer than reverting that change and reverting the files? If I revert those files or parts of them that might pinpoint the change? |
|
@sts-ryan-holton We still need to be able to replicate the issue and determine what can be done to improve the code. The only way to be able to do that is by having a reproducible code. v10.34.0: https://github.com/laravel/framework/actions/runs/7020631916/job/19100818953 There no significant time increased in Framework own tests to suggest there is an issue with Transaction related changes. |
|
@sts-ryan-holton maybe - you would probably need to revert ManagesTransaction, DatabaseTransactionManager, and maybe Connection to make it happen. Agree with @crynobone a repo is likely still necessary for us to actually fix it even if that is causing the issue. |
|
Not much to add in way of a solution, but I've been noticing an increase in parallel worker crashes in our app pipelines since upgrading packages on Monday. Didn't look into it too deeply, as I figured it may have just been an environment issue on our end. |
|
@taylorotwell Okay, I've done some debugging. And things are getting interesting, please take the time to review these additional findings thus far:
So strangely, I don't think the transaction PR is to blame here either. And, to further clarify, whilst reverting those files back, I ran v10.34.2 (with reverted transaction db files)
v10.33.0
Also, now that v10.35.0 is out, I also tested on here. On v10.35.0 there's still a significant performance degradation. As for performance differences, note that in the two sets of actions, in the newer version of v10.34.2, your tests are 25% slower than v10.33.0, which this is quite significant in my opinion. |
10.33.010.34.0Compared version using PHP 8.2 (prefer stable), the closest to your reported issue with the latest version having 29 more tests and only 2 extra seconds is minimal, I wouldn't call that significant. |
10.35.0 (PHP 8.2 stable) |
|
I'm simply comparing the difference in times from the links you'd sent above: 10.33.0 link
10.34.0 link
An entire minute increase in changing the version of the framework. Which, arguably, based on the number of tests shown in your most recent screenshot would imply that if quite a few tests were added and the time doesn't go up by that much that a minute increase is quite significant I'd agree with @cosmastech that something is up, and seeing as it's not technically "broken", most people might be late in identifying this. Yet I update on the day for most weeks. In addition, we see the following error running tests locally as well on the newer version half-way through running tests on v10.34.0 which we don't see on v10.33.0 (note, I didn't see this, but this can be platform specific)
|
|
@sts-ryan-holton that time difference includes setting up the container, running composer install, and other requirements, you should instead just look at PHPUnit execution times. We can only move forward with reproducible code. We have reviewed our other internal application and are unable to replicate this issue, and this doesn't seem to be a widespread issue as others would already submit similar issues. |
|
I agree it's tough for us to move this issue forward without a repo we can clone down to reproduce the issue. Happy to look if something like that can be produced. 👍 |
|
@sts-ryan-holton I wonder if there maybe isn't another shared package in both of our applications that is causing this to happen? 🤔 |
|
I've got a list of dependencies in my original description :) |
|
In your test class that sees the big performance difference, what base test class are you extending, which traits are applied to the class? Is there any other setup code with those could be relevant here? |
|
We're going to close this as there's only one single report for this so far. So most likely this is something for your specific use case and not affecting other Laravel apps. Please try a support channel going forward, thanks. |
|
I noticed my tests running slowly recently. It looks like there is an issue somewhere, 10.33.0 tests: Tests: 2 risky, 96 passed (307 assertions) 10.34.0 Tests: 2 risky, 96 passed (307 assertions) After updating to the latest: Tests: 2 risky, 96 passed (307 assertions) However, I've isolated why my tests are running slow, if I comment out this sqliteCreateFunction from my test setup then they run fast again. DB::connection()->getPdo()->sqliteCreateFunction(
'CONVERT_TZ',
function ($value, $fromTimezone, $toTimezone) {
return Carbon::createFromFormat('Y-m-d H:i:s', $value, $fromTimezone)->setTimezone($toTimezone);
}
);Not spotted the issue in the framework that's causing this. |
|
Larwvel 10.34 and on certainly has slowed tests down significantly. We've had to factor this additional time into our build processes |
|
I back @sts-ryan-holton. Digging around to find discrepancies in the process, I started dumping raw migrations SQLs in files and compare between 10.33 and 10.44. 10.44 run the following two during migrations: Digging around a little bit more, I found that these have been introduced because of the change in Commit: This foreach calls and that calls Reverting the code in To: brings the testing speeds back to normal. Digging a bit more (sorry I know 😢) I found that this:
This is my suggestion: |
|
Tests have been super slow since updating and have been since. It's been 4 months+ now, yet still slow. |
|
Benchmarks added: |
|
Have you tried
|
Yes I'm using it for some time now. My test suite is made of nearly 10k tests and the 100ms delay adds about 15mins to the total CI pipeline. As I mentioned in my latest PR, hasTable returns a Boolean and the table size is irrelevant, especially when it adds unnecessary overhead. Unless I missed something through the flow? Apologies if I did. |
|
@hafezdivandari I'll jump back in an just mention, that although there might be alternative traits and features to improve the performance, but fundamentally, when the issue was initially open, simply upgrading without implementing said features caused a significant performance impact. |
|
@sts-ryan-holton 10.48.5 went out an hour ago. I'm keen to know if my fix has sorted out your issue. It worked for me. |
|
@apspan I can confirm after upgrading to this version, that prior to upgrading would take 156 seconds. After upgrading it took just 38 seconds, so yes, that change worked. Worth also noting, I had a few tests on my most recent Laravel 11 project which has also become a few seconds faster. Honestly can't believe that this bug has been around since December 2023, but glad it's resolved now 👍 |
Laravel Version
10.34.2
PHP Version
8.2.12
Database Driver & Version
MySQL
Description
When running
php artisan test, each test is now taking significantly longer to run since updating from v10.33.0 to v10.34.2. I'm usingassertJsonFragmentin my test cases.The screenshots attached highlight the differences where all I did was both upgrade & downgrade. I have in excess of 50 tests, and here's the comparison of test times between framework versions:
I suspect that this change might be responsible for the performance bottle neck.
Steps To Reproduce
php artisan testFramework v10.33.0
Framework v10.34.2
Here's my
composer.jsonfile:{ "name": "laravel/laravel", "type": "project", "description": "The Laravel Framework.", "keywords": [ "framework", "laravel" ], "license": "MIT", "require": { "php": "^8.1", "deployer/deployer": "7.3.3", "doctrine/dbal": "^3.6", "guzzlehttp/guzzle": "^7.2", "lab404/laravel-impersonate": "1.7.4", "laravel-notification-channels/webhook": "2.5.0", "laravel/fortify": "1.19.0", "laravel/framework": "10.34.2", "laravel/horizon": "5.21.4", "laravel/sanctum": "3.3.2", "laravel/slack-notification-channel": "3.1.0", "laravel/telescope": "4.17.2", "laravel/tinker": "^2.8", "mongodb/laravel-mongodb": "4.0.2", "mtownsend/collection-xml": "^1.7", "mtownsend/request-xml": "2.3.0", "mtownsend/response-xml": "2.2.0", "nesbot/carbon": "2.71.*", "php-http/message-factory": "^1.1", "propaganistas/laravel-disposable-email": "2.2.15", "propaganistas/laravel-phone": "5.0.3", "psr/http-factory-implementation": "*", "rappasoft/laravel-authentication-log": "3.0.0", "rinvex/countries": "9.0.1", "sentry/sentry-laravel": "4.1.0", "spatie/cpu-load-health-check": "1.0.4", "spatie/laravel-health": "1.23.7", "spatie/laravel-permission": "6.1.0", "spatie/laravel-schedule-monitor": "3.4.1", "square1/laravel-collection-rolling-average": "1.0.0", "staudenmeir/laravel-adjacency-list": "1.13.11", "thomasjohnkane/snooze": "2.3.1", "torann/geoip": "3.0.5", "yoeriboven/laravel-log-db": "1.1.1" }, "require-dev": { "fakerphp/faker": "^1.9.1", "laravel/pint": "^1.0", "phpspec/prophecy": "~1.0", "laravel/sail": "1.26.2", "mockery/mockery": "^1.4.4", "nunomaduro/collision": "^7.0", "php-mock/php-mock": "^2.4", "phpunit/phpunit": "^10.0", "spatie/laravel-ignition": "^2.0" }, "autoload": { "psr-4": { "App\\": "app/", "Database\\Factories\\": "database/factories/", "Database\\Seeders\\": "database/seeders/" }, "files": [ "app/helpers.php" ] }, "autoload-dev": { "psr-4": { "Tests\\": "tests/" } }, "scripts": { "post-autoload-dump": [ "Illuminate\\Foundation\\ComposerScripts::postAutoloadDump", "@php artisan package:discover --ansi" ], "post-update-cmd": [ "@php artisan vendor:publish --tag=laravel-assets --ansi --force" ], "post-root-package-install": [ "@php -r \"file_exists('.env') || copy('.env.example', '.env');\"" ], "post-create-project-cmd": [ "@php artisan key:generate --ansi" ] }, "extra": { "laravel": { "dont-discover": [] } }, "config": { "optimize-autoloader": true, "preferred-install": "dist", "sort-packages": true, "allow-plugins": { "pestphp/pest-plugin": true, "php-http/discovery": true } }, "minimum-stability": "stable", "prefer-stable": true }The text was updated successfully, but these errors were encountered: