Skip to content

[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

Closed
wants to merge 2 commits into from

Conversation

inxilpro
Copy link
Contributor

@inxilpro inxilpro commented Dec 6, 2021

Right now, the Laravel docs need to display this warning because the framework's base TestCase relies on logic in the setUp and tearDown methods:

image

This PR moves the setUp and tearDown functionality into new setUpLaravelApplication and tearDownLaravelApplication methods with the @before and @after annotations applied to them respectively.

PHPUnit sets up and tears down tests in the following order (ignoring the undocumented preCondition/postCondition phases):

  1. Methods with the @before annotation (in order: traits, base class, extending class(es))
  2. The setUp() method
  3. The actual test method
  4. The tearDown() method
  5. Methods with the @after annotation (in order: extending class(es), base class, traits)

This means that the new setUpLaravelApplication() method is called before the test's setUp method, and the tearDownLaravelApplication is called after the test's tearDown method.

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:

  1. Methods defined in the actual class
  2. Methods defined in parent classes
  3. Methods defined in traits (in the order of the use statements)

PHPUnit then adds these hooks to an array using array_unshift for @before and array_push for @after. These arrays are called in order during the test run. That means @before hooks in the parent class are prepended last and run first, and @after hooks in the parent class are appended last and run last.

The end result is that framework users can go on using setUp and tearDown exactly as they do right now, but don't have to worry about remembering to call parent::setUp() or parent::tearDown() (just like normal PHPUnit tests).

I'm submitting this against 9.x because it could theoretically affect someone using a method called setUpLaravelApplication or intentionally trying to override the default Laravel setUp method (in which case the solution is just to rename their custom method setUpLaravelApplication).

@bogdankharchenko
Copy link
Contributor

this is the sexiest thing that the laravel test suite needed. 👍

@GrahamCampbell
Copy link
Member

cc @crynobone

@mfn
Copy link
Contributor

mfn commented Dec 7, 2021

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?

  • Do I just need to call setUpLaravelApplication instead of ::setUp?
  • Will @before interfere with that, i.e. already always execute it before?

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 🙏🏼

@inxilpro
Copy link
Contributor Author

inxilpro commented Dec 7, 2021

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.

Yep, in complex cases where you need fine-grained control, you can always just implement tearDownLaravelApplication (with the annotation) and call the parent when you need to.

@inxilpro
Copy link
Contributor Author

inxilpro commented Dec 7, 2021

@mfn Another option would be to do that work in createApplication, which is called at the beginning of Laravel's set up.

@mfn
Copy link
Contributor

mfn commented Dec 7, 2021

Yep, in complex cases where you need fine-grained control, you can always just implement tearDownLaravelApplication (with the annotation) and call the parent when you need to.

I tested this on L8 and can confirm it works 👍🏼, thanks!

@joelharkes
Copy link
Contributor

joelharkes commented Dec 8, 2021

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.

@inxilpro
Copy link
Contributor Author

inxilpro commented Dec 8, 2021

@joelharkes The main benefit is that you don't have to remember to call parent::setUp() in your tests. This makes Laravel's TestCase behave just like the base PHPUnit TestCase (in traditional PHPUnit tests, you do not need to call the parent). I think it makes the DX better in general and is friendlier to newer developers and developers who are used to PHPUnit but just learning Laravel.

The downside seems minimal. Most people extending setUp or tearDown don't need to change their code at all. The only people who do are folks who need to run setup code both before and after the Laravel setup, which you can still easily do.

@taylorotwell
Copy link
Member

Is the order in which PHPUnit executes @before, setUp, teardown, etc. explicitly documented on the PHPUnit website? If so, can you link to it?

@inxilpro
Copy link
Contributor Author

@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:

  • setUpBeforeClass
  • setUp
  • assertPreConditions
  • assertPostConditions
  • tearDown
  • tearDownAfterClass

The @beforeClass annotations are prepended to the the queue using array_unshift as are the @before annotations. This means that any @before... annotations run before setUpBeforeClass and setUp.

The @afterClass and @after annotations are pushed to the queue using the $array[] = syntax. This means that @after... annotations run after tearDown and tearDownAfterClass.

The other thing to consider is how ReflectionClass::getMethods returns methods that are composed via traits or inherited (PHPUnit uses reflection to find annotations).

This order is:

  • Methods defined in the actual class
  • Methods defined in parent classes
  • Methods defined in traits (in the order of the use statements)

(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 @before annotations in the Laravel TestCase are processed after annotations in the actual test case in question, and @before annotations are pushed to the beginning of the queue, we can rely on this interaction to ensure that the Laravel setup happens before the userland test setup.

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.

@taylorotwell
Copy link
Member

taylorotwell commented Dec 16, 2021

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. 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants