-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[9.x] Use @before and @after annotations in TestCase
#39913
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
|
this is the sexiest thing that the laravel test suite needed. 👍 |
|
cc @crynobone |
|
In my test class I call parent after a specific setup in my own code, which is paramount to get my complex / private / company code base set up working. Pseudo-code: abstract class TestCase extends BaseTestCase
protected function setUp(): void
{
// Import stuff which **must** be done before setUp
// ...
parent::setUp();
// Now the rest
// ...How can I translate this with this PR?
TL;DR I need to exactly time when the test suite is set up and I'm unsure how to do this with this PR Thank you 🙏🏼 |
Yep, in complex cases where you need fine-grained control, you can always just implement |
|
@mfn Another option would be to do that work in |
I tested this on L8 and can confirm it works 👍🏼, thanks! |
|
What is actually the benefit of this change except for using a different/newer option in PHPUnit? the only benefit i can think of is that you could easily add multiple @before and @afters via traits. But since this is a base class and not a trait it wouldn't actually benefit from this. any other benefits? @inxilpro I do think more people override the default setup function than you'd think. |
|
@joelharkes The main benefit is that you don't have to remember to call The downside seems minimal. Most people extending |
|
Is the order in which PHPUnit executes @before, setUp, teardown, etc. explicitly documented on the PHPUnit website? If so, can you link to it? |
|
@taylorotwell it's not documented, but the order was discussed in a PHPUnit issue (where PHPUnit broke the existing order of things). The default order runs:
The The The other thing to consider is how This order is:
(This order is mentioned in a php.net comment, and I've independently confirmed it in PHP 7.3–8.1). This means that because It still doesn't 100% solve the order problem, but I think this, combined with #39947 would get us to a more predictable sequence of events. |
|
If it's not explicitly documented as a thing that will not change in the future I'm not sure we want to depend on it since we have been bitten before. 😞 |
Right now, the Laravel docs need to display this warning because the framework's base
TestCaserelies on logic in thesetUpandtearDownmethods:This PR moves the
setUpandtearDownfunctionality into newsetUpLaravelApplicationandtearDownLaravelApplicationmethods with the@beforeand@afterannotations applied to them respectively.PHPUnit sets up and tears down tests in the following order (ignoring the undocumented preCondition/postCondition phases):
@beforeannotation (in order: traits, base class, extending class(es))setUp()methodtearDown()method@afterannotation (in order: extending class(es), base class, traits)This means that the new
setUpLaravelApplication()method is called before the test'ssetUpmethod, and thetearDownLaravelApplicationis called after the test'stearDownmethod.More details about inheritance order
Basically, PHPUnit determines what methods to call by using the
ReflectionClass::getMethods()function. This function returns methods in the following order:usestatements)PHPUnit then adds these hooks to an array using
array_unshiftfor@beforeandarray_pushfor@after. These arrays are called in order during the test run. That means@beforehooks in the parent class are prepended last and run first, and@afterhooks in the parent class are appended last and run last.The end result is that framework users can go on using
setUpandtearDownexactly as they do right now, but don't have to worry about remembering to callparent::setUp()orparent::tearDown()(just like normal PHPUnit tests).I'm submitting this against
9.xbecause it could theoretically affect someone using a method calledsetUpLaravelApplicationor intentionally trying to override the default LaravelsetUpmethod (in which case the solution is just to rename their custom methodsetUpLaravelApplication).