-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[9.x] Opened test traits initialization #39395
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
[9.x] Opened test traits initialization #39395
Conversation
|
Can we re-add the old methods that call the new ones to preserve BC? Otherwise this is going to have an enormous impact. |
|
Didn’t thought of that. Good catch.
I’ll add as soon Pfizer stops giving me this fucking headache.
Italo Baeza C.
… El 28-10-2021, a la(s) 03:49, Dries Vints ***@***.***> escribió:
Can we re-add the old methods that call the new ones to preserve BC? Otherwise this is going to have an enormous impact.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
| $this->disableMiddlewareForAllTests(); | ||
| } | ||
| foreach ($uses = class_uses_recursive(static::class) as $trait) { | ||
| if (method_exists($this, $method = 'setUp'.$trait)) { |
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.
Can we do method_exists($trait, $method = 'setUp'.$trait) instead?
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.
Does it break something?
|
|
IMO this should only apply to userland traits. All built-in Laravel traits should be excluded and should be kept exactly as they were and in the same order as they were. |
|
I assume these depend on each other. These will run first, before any userland trait.
1. About the {teardown}, I’m against. Technically it runs before the app is destroyed.
2. Do traits appear in the order they were used? If that’s the case, you could easily use WithUser after WithProject. Otherwise the best solution would be to create a simple way to force a trait to be initialized only after another (like the HTTP Kernel does on middleware).
Italo Baeza C.
… El 28-10-2021, a la(s) 09:55, Taylor Otwell ***@***.***> escribió:
IMO this should only apply to userland traits. All built-in Laravel traits should be excluded and should be kept exactly as they were and in the same order as they were.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Thanks for your pull request to Laravel! Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include. If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions! If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response. |
(Updated)
What?
Test traits now can be initialized automatically using
setUp{trait},created{trait}anddestryoing{trait}, making traits shareable across multiple Test Classes.setUp{trait}runs as soon the application is already created.created{trait}runs after all traits have been set up.destroying{trait}runs before the application is shut down.How?
The main
TestCase::setUpTraits()has been changed to get an array of all traits used recursively by the Test Case. While this may take the main Test Case Concerns (likeInteractsWithContainer(), the difference in performance is negligible, specially if OPcache is enabled on CLI.Why?
To allow reusing the same trait across multiple Test Classes out of the box. This traits setups everything with just including it in the Test Classes the developer may need it.
Later in their Test Classes:
It also works for deep nested traits. Since `
BC?
Yes, changes traits names, therefore:
setUp,createdordestroyingmethods will have to change. (minor).Caveats
By reversing the array of traits, deeply nested traits are executed first, making composite traits possible. The only caveat is that traits declared last will be executed first, but that could be fixed with a revised version of
traits_uses_recursive().For example, this would be the order:
Bonus:
A trait for an authenticated user could come with the default
laravel/laravelscaffold.