Repeat: How many tests will actually fail if setUp() is only executed once for a test case?

This patch cuts down the total time for the full Drupal core test suite to 9 minutes. (from currently 20+ minutes)

API changes

  1. The current setUp() and tearDown() methods are replaced with setUpBeforeClass() and tearDownAfterClass():
    -  function setUp() {
    -    parent::setUp(array('taxonomy', 'node'));
    +  function setUpBeforeClass() {
    +    parent::setUpBeforeClass(array('taxonomy', 'node'));
  2. setUpBeforeClass() and tearDownAfterClass() are only invoked once for each test class.
  3. setUp() and tearDown() are however retained, and invoked before and after each test method in a class.
  4. All test methods in a class share the same test environment.
  5. Test methods are executed in the order they are defined.
  6. Test methods need to ensure that they are either executed in the right order, or revert any special conditions that they have changed, so following test methods being executed afterwards still work.

    This especially applies to configuration or context that is set up once for test methods and is shared between them (so called "fixtures").

    Example for stuff that breaks:

    <?php
    class WhateverTest extends WebTestBase {
      function
    setUpBeforeClass() {
       
    parent::setUpBeforeClass();
       
    $this->admin_user = $this->drupalCreateUser();
       
    // If any test method logs the user out, following tests will break.
        // Move this into setUp(), so admin_user is logged in for each test method.
       
    $this->drupalLogin($this->admin_user);
       
    $this->node = $this->drupalCreateNode();
      }
      function
    testMaintenanceAccess() {
       
    // Maintenance mode will be enabled for all following test methods, which
        // obviously makes most tests fail.
        // Reset or delete the variable to its original/default value at the end of this test.
       
    variable_set('maintenance_mode', 1);
      }
      function
    testDelete() {
       
    // This deletes the shared node and thus breaks testResave().
        // Move/relocate this test method after testResave() - or alternatively,
        // take the appropriate measures to restore the expected data at the end of
        // this test (which either means to update $this->node with a new one, or,
        // in case the test requires it, even with the identical ID $this->node->nid).
       
    node_delete($this->node->nid);
       
    $this->assertFalse(node_load($this->node->nid), FALSE);
      }
      function
    testResave() {
       
    $resaved_node = node_load($this->node->nid);
       
    node_save($resaved_node);
       
    $this->assertIdentical($this->node->nid, $resaved_node->nid);
      }
    }
    ?>

Help to get this done

@sun will not be able to champion all of the required test changes on his own. But there is a sandbox, so you can help! :)

How to help:

  1. Ask for git write access to the Platform sandbox, if you haven't already. IRC is fastest; but a comment here works, too.
  2. Setup the Platform sandbox for your local D8 git repo/clone.
  3. Checkout @sun's branch into a branch that's specific to you:
    git checkout -b test-once-1411074-[username] platform/test-once-1411074-sun
    git push -u platform test-once-1411074-[username]

    This means you're using @sun's branch as the tip to work off from. @sun will review your changes and merge them into the main branch, if appropriate.
  4. To create patches for the testbot, diff against the -base branch:
    git diff platform/test-once-base test-once-1411074-[username]
  5. To merge new changes from a test-once branch from another user:
    git merge --no-ff platform/test-once-1411074-[otheruser]

    The --no-ff option is key here.
  6. Operations you are not allowed to do:
    • git push platform without specifying a branch name: This would push all of your local branches into the platform sandbox.
    • git push platform [8.x:]test-once-base: Only @sun updates the base branch when needed.
    • git push platform test-once-1411074-sun: You only push to the branch that has your username.
    • git rebase: Rewrites history and makes your branch incompatible / unmergeable.
    • git pull 8.x: Merges in all latest 8.x code into your test-once branch, but all work is based on the -base branch.
    • git merge 8.x: Ditto.
Files: 
CommentFileSizeAuthor
#31 1411074_poc_do_not_test.patch7.34 KBMile23
#25 test.once_.25.patch10.97 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,021 pass(es).
[ View ]
#22 test.once_.22.patch14.85 KBsun
FAILED: [[SimpleTest]]: [MySQL] 64,499 pass(es), 12 fail(s), and 1 exception(s).
[ View ]
#10 test.once_.10.patch211.78 KBsun
FAILED: [[SimpleTest]]: [MySQL] 33,693 pass(es), 256 fail(s), and 68 exception(s).
[ View ]
#8 test.once_.8.patch201.84 KBsun
FAILED: [[SimpleTest]]: [MySQL] 33,628 pass(es), 295 fail(s), and 79 exception(s).
[ View ]
#6 test.once_.6.patch179.39 KBsun
FAILED: [[SimpleTest]]: [MySQL] 33,295 pass(es), 376 fail(s), and 79 exception(s).
[ View ]
#4 drupal8.test-setup-once.4.patch155.21 KBsun
FAILED: [[SimpleTest]]: [MySQL] 32,635 pass(es), 914 fail(s), and 158 exception(s).
[ View ]
drupal8.test-setup-once.0.patch3.39 KBsun
FAILED: [[SimpleTest]]: [MySQL] 29,678 pass(es), 880 fail(s), and 157 exception(es).
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, drupal8.test-setup-once.0.patch, failed testing.

PHPUnit has two methods, setUp() and setUpBeforeClass(). teardown has similar structure. So, a test author can choose to implement either. As a general rule, I agree that one drupal install per class is good.

Nice work so far!

#2 sounds like an excellent pattern to me.

Title:Question: How many tests will actually fail if setUp() is only executed once for a test case?Setup test environment only once per test class; introduce setUpBeforeClass() and tearDownAfterClass() [PHPUnit]
Status:Needs work» Needs review
Issue tags:+phpunit
StatusFileSize
new155.21 KB
FAILED: [[SimpleTest]]: [MySQL] 32,635 pass(es), 914 fail(s), and 158 exception(s).
[ View ]

This makes soooo much sense: http://www.phpunit.de/manual/3.7/en/fixtures.html :-/

So. Introducing setUpBeforeClass() and tearDownAfterClass(). A major leap towards PHPUnit migration.

Awesomeness! :)

Status:Needs review» Needs work

The last submitted patch, drupal8.test-setup-once.4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new179.39 KB
FAILED: [[SimpleTest]]: [MySQL] 33,295 pass(es), 376 fail(s), and 79 exception(s).
[ View ]

Fixed fixtures.

Status:Needs review» Needs work

The last submitted patch, test.once_.6.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new201.84 KB
FAILED: [[SimpleTest]]: [MySQL] 33,628 pass(es), 295 fail(s), and 79 exception(s).
[ View ]

Fixed more fixtures.

Status:Needs review» Needs work

The last submitted patch, test.once_.8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new211.78 KB
FAILED: [[SimpleTest]]: [MySQL] 33,693 pass(es), 256 fail(s), and 68 exception(s).
[ View ]

And more.

Status:Needs review» Needs work

The last submitted patch, test.once_.10.patch, failed testing.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Take care. The abuse of this method can drive you to coupling between tests and lack of isolation.

Hmm...I both like and the idea and am a little worried by it. The idea that a test class now only has to get setup once for all tests certainly seems pretty awesome if only for the speed bump. But at the same time (and maybe I am mistaken so someone could knock some sense into me), I can't help but think of a class now being one large continuous test. What @carlescliment said in isolating the issue (instead of reading through the setup and test function, you would now be reading through the setup + all test functions prior to and including the test function to debug what the test issue could be) does worry me a bit.

The downside of shared fixtures are well known. Please read the PHPUnit docs I linked to in #4.

However, this is nothing new in general. If you're afraid of fixtures, then don't use them, and instead create your users/nodes/fields/whatever from scratch for every test method. After all, that's what the setUp() method is for.

The real difference takes effect for the 99.9% of things your test actually does not test, but which you need nevertheless in order to execute the test in the first place. Alas, the extremely optimized testbots (which run with a database on /tmpfs, among many other performance optimizations) spend more than 10 minutes with setting up a new environment for every test method, even though none of our test methods needs a completely new.

Lastly, the shared fixtures between test methods is not concerning at all, given that we're not even able to properly isolate the test from the test runner process. We're currently bumping from one critical bug into the next, and the duct-taping costs plenty of wasted man hours every single week to keep the system running. That's why we want to move away from our homegrown Simpletest and use one of the existing standard testing frameworks in the PHP world instead. This change here is a huge leap towards that.

@sun, Thanks for the clarification. I was reading through it and the line:

It cannot be emphasized enough that sharing fixtures between tests reduces the value of the tests.

Was what brought up part of the doubt. But if we're not able to isolate the tests anyways (and this code moves us towards phpunit which hopefully does help with our test isolation issues and overall will do much more good anyways), then awesome :) I'll make some time and try to post some updates in your sandbox towards this.

Lacking progress here, let me propose a different plan of attack:

  1. Introduce a new, separate WebRealTest [or whatever name].
  2. Make this base class introduce setUpBeforeClass() and tearDownAfterClass(), as proposed here.
  3. Make this base class use the Testing profile without Node module dependency, as aimed for in #1541298: Remove Node module dependency from Testing profile. Temporarily switch the default profile for WebTest to Minimal profile instead. In other words, make the Testing profile truly mean "nothing."
  4. As a separate follow-up, make this base class implement support for #913086: Allow modules to provide default configuration for running tests
  5. Have it, and start to convert stuff.

I'm fully aware that this will introduce a "second" (duplicate) layer of how people can write tests.

However, I'd rather have this progress, instead of no progress at all.

@sun what about installing drupal normally in setUpBeforeClass and saving a database snapshot that we can restore on setUp? would that be a good compromise between speed and keeping things working?

we have discussed db restore in the past and Upal even implements that approach. But we are now using the Testing profile a lot more and we need to keep stripping that down so that it does not do menu build and node install and so on. I think we can speed up Testing and use more UnitTestCase such that we don't have to change to DB restore method.

Is there any issues regarding PHPUnit migration ? A [meta] issue or something that records every moves to PHPUnit ?

@Sylvain Lecoy: There's no official meta issue yet; only #1567500: [meta] Replace the testing framework with PHPUnit and possibly rewrite Simpletest. However, that's mainly a temporary dumping ground for collecting relevant input and information. It's definitely missing the latest and greatest results of discussions I had with various people. I will try to create a new and proper meta issue as soon as possible. Meanwhile, all related and forward-thinking issues are tagged with Testing system.

Ok so I created this [meta] issue: #1801176: Deploy a PHPUnit system with a bottom-up approach

I also contacted Moshe Weitzman to ask him if there is any work in progress that I can take over. I am so pissed-off by SimpleTest today that I am making my personal battle to work on replacing it by PHPUnit.

Issue summary:View changes

Added API changes.

Status:Needs work» Needs review
Issue tags:-API change
StatusFileSize
new14.85 KB
FAILED: [[SimpleTest]]: [MySQL] 64,499 pass(es), 12 fail(s), and 1 exception(s).
[ View ]

Trying once more, with a new approach that hopefully works better:

→ Introduced new SingleEnvInterface to explicitly opt-in to setUpBeforeClass() + tearDownAfterClass().


FWIW, in order to not run into total indentation hell, I also had to adjust the execution flow in TestBase::run(). Will likely split that out into an own issue, so that this is more reviewable.

Status:Needs review» Needs work

The last submitted patch, 22: test.once_.22.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,021 pass(es).
[ View ]

#2201783: Simplify execution logic in TestBase::run() has landed + also fixed the undefined $class variable test failures of this patch.

So this is much easier to review now — what do you think of the approach taken? In essence:

  1. For backwards-compatibility, setUpBeforeClass() is not invoked for tests.
  2. If a test class implements the new SingleEnvInterface, then it has to implement setUpBeforeClass() + tearDownAfterClass().
  3. Only through that explicit opt-in, the test execution adapts itself and calls setUpBeforeClass() once before all tests, then proceeds as usual, but Drupal is only installed once for all test methods.
  4. This is accomplished by making the test base classes aware of the different execution paths, so that e.g. WebTestBase::setUpBeforeClass() performs the Drupal installation, and WebTestBase::setUp() turns into a no-op.

Now that the test execution process has been revamped a bit I wonder whether we can't get away without the complexity that is added by the conditional logic in this patch.

I.e. couldn't we go back to something like #10 but without converting the whole slew of test classes:
- Make TestBase unconditionally call setUpBeforeClass() and tearDownAfterClass() at the appropriate places.
- Rename WebTestBase::setUp()/tearDown() to setUpBeforeClass() and tearDownAfterClass(). The same for DrupalUnitTestBase and UnitTestBase.
- Add empty setUp() and tearDown() methods to those classes.
- Convert one or two test classes to setUpBeforeClass()/tearDownAfterClass().

??
I may be missing something.

If we stick to this approach we should rename SingleEnvironmentInterface. In general, I'm not sure on the terminology. If we stick to environment the two methods really should be setUpEnvironment() and tearDownEnvironment().

The old approach requires to convert all tests to the revised test execution flow. That is, because many tests expect that every test method operates in a fresh installation. In turn, all tests would have to be adjusted.

To avoid that, the idea is to allow tests to explicitly opt-in to the different environment setup and execution flow.

Renaming the setUpBeforeClass() and tearDownAfterClass() methods is definitely a no-go, because the names are derived from PHPUnit and meant to be consistent with it. After all, our long-term goal still is to get rid of Simpletest.

Hmm... OK, that makes sense then. But the ultimate goal is still to not have this interface and to have all test classes implement this, right? (Even if we might not reach that goal for D8) And for e.g. new tests that get added they should also implement this right off the bat.

Regarding the method names, I didn't know PHPUnit supported that, that's cool. I still think the interface should be renamed to "SingleEnvironmentInterface" and it should have at least one or two more sentences of additional information:
* That implementing the interface leads to a performance increase.
* That all new test classes should implement this so that we can eventually remove the interface (given that that is true)
* That this is compliant with what PHPUnit does.

One nitpick:

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -801,6 +801,72 @@ public function run(array $methods = array()) {
+        $implements_interface = in_array('Drupal\simpletest\SingleEnvInterface', class_implements($parent_class));

This could use is_subclass_of() I think.

As the logic is a little bit involved this could use a couple more eyes, although I couldn't find any flaws.

I'm not sure why this can't simply be another base class, where setUp() is marked final, and setUpOnce() gets called on subclasses.

I already investigated the idea of a separate base class (multiple times even), but the major problem with that is that we'd have to duplicate multiple base classes, because TestBase::run() contains the actual test class/method dispatching code, whereas WebTestBase is the subclass that primarily needs the setup-once behavior. In turn, we'd have to duplicate both (or move all code into traits), but I fear that such an approach would make the overall code much more complicated to maintain.

StatusFileSize
new7.34 KB

How about adding a flag in getInfo()? This will have the advantage of being easy to backport, too.

I admit I have not read this issue, but I did cmd+F for "upstream" at least and didn't find anything mentioned.

I'd really rather not go down the same road we did with SimpleTest of essentially "forking" the library and making it full of Drupalisms, some of them actually useful (like this one sounds). Can we file an upstream patch for this feature?

@webchick setUpBeforeClass() and tearDownAfterClass() are already part of phpunit. This issue brings us *closer* to PHPUnit, not farther.

Rock! :) Carry on, then!

@webchick: I'd really rather not go down the same road we did with SimpleTest of essentially "forking" the library and making it full of Drupalisms,

Pretty sure that boat has sailed, unless the goal is to re-implement SimpleTest based on the real framework, in which case this issue needs to be waaaay postponed. https://packagist.org/packages/simpletest/simpletest

@moshe: setUpBeforeClass() and tearDownAfterClass() are already part of phpunit. This issue brings us *closer* to PHPUnit, not farther.

How to make Drupal do modern, performant functional testing using PHPUnit as a framework:

1) Decide that the whole thing can be dependency injected by saying $app = new DrupalKernel('test', $etc, $etc);

2) See: http://symfony.com/doc/current/book/testing.html#your-first-functional-test

Seems obvious to me this is not a goal for Drupal 8 or it would have happened a while back. Like, first. :-)

But as for this issue: Which do people prefer? Add an interface, or make a flag in getInfo()?