-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[12.x] Introduce WithCachedRoutes testing trait
#57623
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
Conversation
| return $this['files']->exists($this->getCachedRoutesPath()); | ||
| return ($this->bound('routes.cached') | ||
| && $this->make('routes.cached') === true) | ||
| || $this['files']->exists($this->getCachedRoutesPath()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this check can and should be cached (regardless if this PR is accepted)
Unless there's a reason to expect that during a run (either with the traditional PHP model or using Octane) this file would be deleted, we are repeatedly performing a file_exists check.
|
We went down this path with a different approach a while back, but ended up having to rip it all out because of issues laravel/laravel#5050. Keen to have something like this in the framework. We've fixed some issues recently where having cached routes ended up in different behaviour, so love that you can get closer to what you may have deployed. |
Interesting. Can you elaborate more on those cached routes issues that were recently fixed? |
|
Drafting this per more info from @timacdonald - but otherwise looks good to me. |
Realised how ambiguous my comment was now. I meant that we recently fixed issues in the framework where the cache and uncached routes would behave differently, not that we fixed issues specific to route caching itself. Was just trying to help sell the PR cause I want it! This is great. It shouldn't suffer from the problems of the previously attempted implementation, which I believe were mostly due to the fact that we were actually caching the routes rather than doing this in memory version. LGTM 🚀 |
|
(I'm also keen to see your config caching follow up 🙏 ) |
It's in draft, I'll mark it as ready now. |
|
Thanks all! |
I joined a team with a modular monolith. There are 8000+ tests, the majority of which extend from Laravel's
Illuminate\Foundation\Testing\TestCase. By the nature of the modular monolith, there are many, many routes files, as each domain has one or more files for route definitions.Leveraging the profiler in Herd and running the test suite, I noticed that we were spending a ton of cumulative time on the
ServiceProvider@loadRoutesFrom()method. This is due to reading from disk.Even if we were to cache the routes via
php artisan route:cache --env=testing, we would still be reading from disk on every test. It also presents a problem, as it's possible the cached routes would differ from local vs testing environment, and it creates the overhead of developers having to remember to run the cache command.The(?) Solution
A new testing trait called
WithCachedRoutes. This stores the routes as a static property within aTesting\CachedStateobject during test setup, and then subsequent tests do not need to read from disk.Potential pitfalls
setUp()method and tests some kind of static property/constant that changes how routes get defined, this will carry over into subsequent tests. I would imagine that this is an incredibly rare case, and the user can just remove the trait from that test case. Good documentation about the feature will mitigate this.Follow ups
I think a logical next step is to cache the configuration as well.
Benchmarks
This is mostly anecdotal, but for our test suite which reports 18,981 tests: