Switch the order in which @before methods and setUp() are executed #1616

Closed
beberlei opened this Issue Feb 14, 2015 · 33 comments

Projects

None yet
@beberlei
Contributor

The methods that are tagged with @before are called after the setUp() method.

That makes it impossible to use traits for some generic setup code and then use the already existing services from the generic setup to do a specialed setUp, example:

class MyTest extends PHPunit_Framework_TestCase
{
    use ContainerSetup;

    public function setUp()
    {
           $this->container->get('..'); // if ContainerSetup has a `@before` hook, its not called here yet.
    }
}
@TomasVotruba

I would love this feature too.

๐Ÿ‘

@sebastianbergmann

Can you send a pull request, @beberlei? I think this can go into 4.5.

@Dr-Head
Dr-Head commented Feb 27, 2015

๐Ÿ‘ agreed

@giorgiosironi
Contributor

I want to open a PR for this. Some questions to avoid implementing the wrong thing:

  1. what should be the behavior for @after? By simmetry it should be called after tearDown()?
  2. what should be the behavior of @beforeClass and @afterClass? Will they follow the same pattern of being called respectively before and after setUpBeforeClass() and tearDownAfterClass()?
@giorgiosironi giorgiosironi added a commit to giorgiosironi/phpunit that referenced this issue May 24, 2015
@giorgiosironi giorgiosironi @before methods are run before setUp()
Partially fixed #1616
65ae6c0
@chungth
chungth commented Jul 28, 2015

+1

@Thijmen
Thijmen commented Aug 15, 2015

+1

@deeky666

+1

@KingMob
KingMob commented Sep 23, 2015

+1 @giorgiosironi what's the status of PR 1722? I see it failed a CI check.

@giorgiosironi
Contributor

It failed the run on PHP 7 which was unstable at the time, no test failure on current versions of PHP.
My questions at #1616 (comment) stills stand to define some other behavior so that I can finish it.

@sebastianbergmann

I don't know what the right behavior would be, @giorgiosironi. Mostly due to the fact that I don't use @before, etc.

@sebastianbergmann

This should do what you propose, @beberlei, right?

diff --git a/src/Util/Test.php b/src/Util/Test.php
index 1dcc921..28594de 100644
--- a/src/Util/Test.php
+++ b/src/Util/Test.php
@@ -806,19 +806,31 @@ public static function getHookMethods($className)

                 foreach ($class->getMethods() as $method) {
                     if (self::isBeforeClassMethod($method)) {
-                        self::$hookMethods[$className]['beforeClass'][] = $method->getName();
+                        array_unshift(
+                            self::$hookMethods[$className]['beforeClass'],
+                            $method->getName()
+                        );
                     }

                     if (self::isBeforeMethod($method)) {
-                        self::$hookMethods[$className]['before'][] = $method->getName();
+                        array_unshift(
+                            self::$hookMethods[$className]['before'],
+                            $method->getName()
+                        );
                     }

                     if (self::isAfterMethod($method)) {
-                        self::$hookMethods[$className]['after'][] = $method->getName();
+                        array_unshift(
+                            self::$hookMethods[$className]['after'],
+                            $method->getName()
+                        );
                     }

                     if (self::isAfterClassMethod($method)) {
-                        self::$hookMethods[$className]['afterClass'][] = $method->getName();
+                        array_unshift(
+                            self::$hookMethods[$className]['afterClass'],
+                            $method->getName()
+                        );
                     }
                 }
             } catch (ReflectionException $e) {
@sebastianbergmann sebastianbergmann added this to the PHPUnit 5.1 milestone Oct 14, 2015
@sebastianbergmann

As discussed with @beberlei, only the order for @before and @beforeClass must be changed.

@GrahamCampbell
Contributor

๐Ÿ‘Ž

@GrahamCampbell
Contributor

This breaks every single Laravel application.

@taylorotwell

This is indeed a breaking change.

@sebastianbergmann
Owner

I do not consider this a breaking change. The behavior was undefined before, now it is defined. You were relying on undocumented behavior, sorry.

@GrahamCampbell
Contributor

While the order of the before annotations was not defined, the fact they were after the setup method, was defined. This is breaking.

@marcioAlmada
Contributor

@GrahamCampbell could you explain how this "breaks every single Laravel application"?

I wonder if projects that use phpunit as a dependency, like codeception, would have issues too.

@GrahamCampbell
Contributor

Everyone will now get a fatal error because the setup method created an object that before methods then used.

@MadaraUchiha

@GrahamCampbell That's not how breaking changes work. If you rely on undocumented, untested behavior, nothing is set in stone.

@GrahamCampbell
Contributor

I don't see how the actual details matter though. It's a breaking change, if you consider it's behaviour to have been defined.

@GrahamCampbell
Contributor

Just because something's undocumented, doesn't mean you should break it. There's plenty other stuff not documented that people would be unhappy about if it broke.

@GrahamCampbell
Contributor

That's an example of the mess this makes: https://travis-ci.org/StyleCI/StyleCI/jobs/94697689.

@taylorotwell

Yeah, to me a breaking change is defined as something worked before and now it doesn't. That's "breaking" - as in you broke applications. Is that not a good definition of "breaking"?

@Ma27
Contributor
Ma27 commented Dec 7, 2015

why not making the behavior configurable through the phpunit.xml?
We could release a phpunit 5.1.1 which fixes that and in phpunit 6 the old behavior can be dropped as breaking changes can be done in major releases according to semantic versioning.

@sebastianbergmann
Owner

@Ma27 PHPUnit already has way too many configuration settings, so no on that.

@MadaraUchiha

@taylorotwell No, if you rely, for example, on the existence of an internal global variable, and you use it for your purposes, and someone refactors that global variable away, it is not a breaking change.

Breaking changes are changes to the documented, tested, defined, public API. If you rely on behavior that only incidentally works from the way the code was written, but that behavior was not defined or tested, defining/testing/changing it does not constitute a breaking change.

Things don't need to be explicitly defined to be undefined, in order to be undefined.

@sebastianbergmann sebastianbergmann added a commit that referenced this issue Dec 7, 2015
@sebastianbergmann Revert "Closes #1616"
This reverts commit 8cb498a.
6c3df67
@taylorotwell

I'm going to look at this in a few minutes and see how hard it is for us to be agonstic about what order these things are run in.

@sebastianbergmann
Owner

@taylorotwell The change has been reverted for PHPUnit 5.1 and was rescheduled for PHPUnit 6. If you want to look at something today ... why not look at laravel/framework#10808 ;-)

@taylorotwell

Just wanted to note that on the Laravel side we insulated ourselves from this change. Not sure if that effects you wanting to go ahead and put this in a 5.x series release since we were somewhat edge case in our usage.

@GrahamCampbell
Contributor

The change has been reverted for PHPUnit 5.1 and was rescheduled for PHPUnit 6.

๐Ÿ‘ for that, regardless of what laravel's doing.

@tmatsuo
tmatsuo commented May 13, 2016 edited

@sebastianbergmann
Do you still think it's not worthwhile to introduce a configuration?

I'm asking because currently PHPUnit 5.x has the order setUp -> @before (also setUpBeforeClass -> @beforeClass), and PHPUnit 6.x will change it, right? Then it's hard for us to build something reliable leveraging this feature that works on both. Are there any suggestions for that?

In particular, I'm trying to build an e2e test harness for a PaaS. It's very similar to the first comment from @beberlei but I'm actually trying to do the opposite. I want to run something before @beforeClass.

class MyTest extends PHPunit_Framework_TestCase
{
    use PaasDeploymentTestTrait; // It deploys an app in @beforeClass

    public static function setUpBeforeClass()
    {
           // I want to run this before the PaasDeploymentTestTrait's @beforeClass
           self::dumpConfigFromEnv(); 
    }
}
@sebastianbergmann sebastianbergmann changed the title from Consider switching `@before` and setUp() order to Switch the order in which `@before` methods and `setUp()` are executed Jun 5, 2016
@sebastianbergmann sebastianbergmann changed the title from Switch the order in which `@before` methods and `setUp()` are executed to Switch the order in which @before methods and setUp() are executed Jun 5, 2016
@sebastianbergmann sebastianbergmann added a commit that referenced this issue Aug 19, 2016
@sebastianbergmann Revert "Revert "Closes #1616""
This reverts commit 6c3df67.
c2165be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment