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
- The current
setUp()
andtearDown()
methods are replaced withsetUpBeforeClass()
andtearDownAfterClass()
:
- function setUp() { - parent::setUp(array('taxonomy', 'node')); + function setUpBeforeClass() { + parent::setUpBeforeClass(array('taxonomy', 'node'));
setUpBeforeClass()
andtearDownAfterClass()
are only invoked once for each test class.setUp()
andtearDown()
are however retained, and invoked before and after each test method in a class.- All test methods in a class share the same test environment.
- Test methods are executed in the order they are defined.
- 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:
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:
- Ask for git write access to the Platform sandbox, if you haven't already. IRC is fastest; but a comment here works, too.
- Setup the Platform sandbox for your local D8 git repo/clone.
- 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.
- To create patches for the testbot, diff against the -base branch:
git diff platform/test-once-base test-once-1411074-[username]
- 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. - 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.
Comment | File | Size | Author |
---|---|---|---|
#31 | 1411074_poc_do_not_test.patch | 7.34 KB | Mile23 |
#25 | test.once_.25.patch | 10.97 KB | sun |
#22 | test.once_.22.patch | 14.85 KB | sun |
#10 | test.once_.10.patch | 211.78 KB | sun |
#8 | test.once_.8.patch | 201.84 KB | sun |
Comments
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedPHPUnit 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!
Comment #3
xjm#2 sounds like an excellent pattern to me.
Comment #4
sunThis 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! :)
Comment #6
sunFixed fixtures.
Comment #8
sunFixed more fixtures.
Comment #10
sunAnd more.
Comment #11.0
sunUpdated issue summary.
Comment #11.1
sunUpdated issue summary.
Comment #12
carlescliment CreditAttribution: carlescliment commentedTake care. The abuse of this method can drive you to coupling between tests and lack of isolation.
Comment #13
BTMash CreditAttribution: BTMash commentedHmm...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.
Comment #14
sunThe 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.
Comment #15
BTMash CreditAttribution: BTMash commented@sun, Thanks for the clarification. I was reading through it and the line:
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.
Comment #16
sunLacking progress here, let me propose a different plan of attack:
WebRealTest
[or whatever name].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.
Comment #17
lucascaro CreditAttribution: lucascaro commented@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?
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedwe 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.
Comment #19
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedIs there any issues regarding PHPUnit migration ? A [meta] issue or something that records every moves to PHPUnit ?
Comment #20
sun@Sylvain Lecoy: There's no official meta issue yet; only #1567500: [meta] Pave the way to 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.
Comment #21
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedOk 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.
Comment #21.0
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedAdded API changes.
Comment #22
sunTrying once more, with a new approach that hopefully works better:
→ Introduced new
SingleEnvInterface
to explicitly opt-in tosetUpBeforeClass()
+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.Comment #23
sunSaid spin-off: #2201783: Simplify execution logic in TestBase::run()
Comment #25
sun#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:
setUpBeforeClass()
is not invoked for tests.SingleEnvInterface
, then it has to implementsetUpBeforeClass()
+tearDownAfterClass()
.setUpBeforeClass()
once before all tests, then proceeds as usual, but Drupal is only installed once for all test methods.WebTestBase::setUpBeforeClass()
performs the Drupal installation, andWebTestBase::setUp()
turns into a no-op.Comment #26
tstoecklerNow 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().
Comment #27
sunThe 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()
andtearDownAfterClass()
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.Comment #28
tstoecklerHmm... 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:
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.
Comment #29
Mile23I'm not sure why this can't simply be another base class, where setUp() is marked final, and setUpOnce() gets called on subclasses.
Comment #30
sunI 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, whereasWebTestBase
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.Comment #31
Mile23How about adding a flag in getInfo()? This will have the advantage of being easy to backport, too.
Comment #32
webchickI 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?
Comment #33
moshe weitzman CreditAttribution: moshe weitzman commented@webchick setUpBeforeClass() and tearDownAfterClass() are already part of phpunit. This issue brings us *closer* to PHPUnit, not farther.
Comment #34
webchickRock! :) Carry on, then!
Comment #35
Mile23Pretty 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
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()?
Comment #36
jhedstromPatch no longer applies, but it is probably more work than a simple re-roll at this point since much has changed in the past year.
Comment #43
apadernoComment #52
mondrakeComment #54
joachim CreditAttribution: joachim as a volunteer commentedThis should be resurrected - but should be split into separate issues for Kernel / Browser / JS tests.
Comment #55
Mile23Comment #56
Chi CreditAttribution: Chi commentedIt feels if we had fixed this issue in 2012 the Drupal Association could save tens of thousands of dollars on CI environments.
Comment #57
joachim CreditAttribution: joachim as a volunteer commented(Posted on wrong issue)
Comment #58
mgiffordHow often are we running these tests? Seems like a good investment of time to cut the execution time in half, even if it's a matter of allowing us to know earlier that the patch works.