Increase simpletest speed by running on a simplified profile

I already investigated this a while back (patch attached). We already can get a significant performance improvement by doing intra-test-class caching (ie. we don't reinstall Drupal between test methods, simply copy back the clean environment).

Caching test environments *between* test-classes (ie. caching a clean installation of a given set of modules) would require locking somehow (because tests can be run concurrently), and I'm not sure how to do this now.

*But:* copying DB tables in a clean, portable manner is not easy. For MySQL, we need to add NO_AUTO_VALUE_ON_ZERO... but this seems to break some part of Drupal (especially where we do INSERT INTO table VALUES (DEFAULT)).

As a conclusion: Crell talked about implementing a clean interface for copying tables in our database layer. We should bother him :)

would using mysql transactions help?
begin transaction
run test
rollback transaction
its a strategy i used in a previous life w/postgres

#4 gives me: Notice: Undefined index: test_id in _simpletest_batch_finished() (line 403 of /home/catch/www/7/modules/simpletest/simpletest.module).

@catch#8: that's one of the issue with that patch, linked to the NO_AUTO_VALUE_ON_ZERO setting.

When assigning a new test_id, simpletest.module does:

  $test_id = db_insert('simpletest_test_id')->useDefaults(array('test_id'))->execute();

This translates to:

INSERT INTO simpletest_test_id VALUES(DEFAULT)

And that fails when NO_AUTO_VALUE_ON_ZERO is set (DEFAULT translates to 0 so test_id = 0 on the first run, and a duplicate error is generated on the second run).

I still need to study how to solve this issue in a cross-engine manner (inserting NULL would work on MySQL, but probably fail on PostgreSQL).

Specifying the value for the index of course will use the value given regardless of the DB engine.

Not at all :)

On MySQL, you are supposed to insert NULL (or omit) a column to have the autoincrement value:

NO_AUTO_VALUE_ON_ZERO affects handling of AUTO_INCREMENT columns. Normally, you generate the next sequence number for the column by inserting either NULL or 0 into it. NO_AUTO_VALUE_ON_ZERO suppresses this behavior for 0 so that only NULL generates the next sequence number.

But on PostgreSQL, serials are implemented by (1) sequences and (2) variable default values:

CREATE TABLE tablename (
colname SERIAL
);

is equivalent to specifying:

CREATE SEQUENCE tablename_colname_seq;
CREATE TABLE tablename (
colname integer NOT NULL DEFAULT nextval('tablename_colname_seq')
);
ALTER SEQUENCE tablename_colname_seq OWNED BY tablename.colname;

So you are supposed to insert DEFAULT (or omit) to get the automatic value.

I wish there was a way to quadruple-subscribe to an issue.

I want this in core so bad it hurts. ;) Will try and take a look tomorrow when I am more conscious.

Subscribe. It takes more than half an hour to run through all tests on my server, unless the server gets a hiccup halfway in and stops the testing.

Interesting. (Subscribe.)

Subscribing

@boombatower: be careful, that's already a big patch, and it risks facing the same death as the previous attempt at refactoring Simpletest.

I suggest working on "baby"-steps:

1. first, we should refactor database prefixes to make them per-connection. The patch for this is nearly ready in #195416: Table prefixes should be per database connection, but desperately crying for attention. This way, you could have a "child" connection separated from the "parent" connection.
2. second, we should implement a DatabaseConnection::duplicate() method (or a similar name), that would copy all schema and data of an existing database. This needs to be done per database engine, because some engines might have nice shortcuts (for example on SQLite we could just copy the database file, etc.).
3. third, we need to add .test files to the registry, in order to be able to easily get the filename in which a test class is defined.
4. fourth, we need to implement a "test runner" that run an individual test. It would be perfect if this runner could avoid bootstrapping Drupal to BOOTSTRAP_FULL, in order to ensure that the environment is really clean for the ground up. For that we need at least 3 and probably 1.

There is really no need for a gigantic patch to do that ;)

Step #0 - write instructions about writing a tests! Mostly concerned about optimizations to stop anarchy in test writing.

- Less test* methods
- Less drupalGet()
- commit #500866: [META] remove t() from assert message I think - t() eats a lot

I agree with andypost: We have to either massively optimize the creation of new test environments or we have to discourage people from creating a new test*() method for every test thought they have.

So let's agree on where the low-hanging fruit is. The first thing we can do is to set the standard and document it.

As far as I can tell in D7, the creation of a new db for every test* method trumps every other performance issue by thousands of times. So that argues for grouping functionality into less test methods.

However, that conflicts with the general functionality idea that you should keep test methods specific in their aim so that it's easy to understand what each one is about.

I'd hate to lose the forest for the trees. Changing from a better software approach to a faster one seems like giving up ground.

But we have to remember that our test suite is taking longer and longer to execute. Even DamZ's qa-1 now takes quite a lot longer than it used to. So our path is not sustainable (I don't think) and we really have to fix performance before it because a true thorn in our side.

We're still in the range of successful testing times, although they've grown quite a lot

That would be a BIG plus. The tests remain separate, but can be marked as not having dependency issues.

Why not doing something like this?

Some tests will probably fail, because they are not designed to work in an unclean environment, but this should help a ton for the database tests.

This looks pretty reasonable to me. I think we should give it a try.

+++ modules/simpletest/drupal_web_test_case.php
@@ -63,6 +63,15 @@ abstract class DrupalTestCase {
+  protected $singleEnvironmentMode = FALSE;

I think we could use a better name than $singleEnvironmentMode to drive it, as the word "environment" is so terribly overloaded already. How about $sharedInstallMode, or something like that

Powered by Dreditor.

I think it's better to make a parameter that point which install profile should be used in test method

The reason we used standard profile because we wanted 'realistic' tests. A bit hard to explain at 2am :) anyways i am all for speeding up tests, i am totally against any major test refactoring in D7, i am all for major test refactoring in D8.

@chx: I thought it was because simpletest went in before the minimal profile, and because we didn't have a way to specify which profile to use for tests, but 'free' code coverage may have been another reason.

"the simple way to fix this is just to turn on a bunch more modules on the tested-side until it work"
I think it'd be a good first run for a patch to default to the minimal profile then explicitly list dependencies (or specify the standard profile). Then followup patches can try to reduce the dependencies, doing it all in one mega-patch seems like it'll end up going into D8 whatever happens.

The work for switching which profile to test is being pursued here: #279515: setting an installation profile for a test doesn't run hook_install_tasks().

We need to be able to hide the new Testing profile, because we definitely don't want to put that in the face of newcomers on the first installation screen. I suggest we just add hidden = TRUE in the .info and make sure this is taken into account.

Other then that, this is a pretty harmless patch that increase the testing speed by as much as 30%!

982,879	 File	simpletest.profile_1.patch	 Result received from test client #253.	 07/11/2010 - 13:13:55
982,874	 File	simpletest.profile_1.patch	 Requested by test client #253.	 07/11/2010 - 12:51:00
982,824	 File	bartik-cleanup-general.patch	 Result received from test client #253.	 07/11/2010 - 12:13:15
982,764	 File	bartik-cleanup-general.patch	 Requested by test client #253.	 07/11/2010 - 11:41:00

(32 minutes vs. 23 minutes!)

Big +1 here.

Yippee! I haven't tried it, but I can see this definitely being a +1.

+++ modules/aggregator/aggregator.test	11 Jul 2010 14:41:06 -0000
@@ -292,6 +294,8 @@ class AddFeedTestCase extends Aggregator
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/aggregator/aggregator.test	11 Jul 2010 14:41:06 -0000
@@ -362,6 +368,8 @@ class RemoveFeedTestCase extends Aggrega
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/aggregator/aggregator.test	11 Jul 2010 14:41:06 -0000
@@ -642,6 +656,8 @@ class ImportOPMLTestCase extends Aggrega
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/locale/locale.test	11 Jul 2010 14:41:20 -0000
@@ -1215,6 +1220,8 @@ class LocaleLanguageSwitchingFunctionalT
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/poll/poll.test	11 Jul 2010 14:41:23 -0000
@@ -640,4 +642,4 @@ class PollExpirationTestCase extends Pol
\ No newline at end of file

Please add one.

+++ modules/simpletest/drupal_web_test_case.php	11 Jul 2010 14:41:27 -0000
@@ -1125,6 +1125,11 @@ class DrupalWebTestCase extends DrupalTe
+ ¶

Trailing whitespace

+++ modules/simpletest/tests/bootstrap.test	11 Jul 2010 14:41:28 -0000
@@ -224,6 +225,7 @@ class BootstrapPageCacheTestCase extends
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/simpletest/tests/path.test	11 Jul 2010 14:41:32 -0000
@@ -131,6 +131,8 @@ class DrupalMatchPathTestCase extends Dr
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/system/system.test	11 Jul 2010 14:41:34 -0000
@@ -1636,6 +1656,7 @@ array_space[a b] = Value';
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/trigger/trigger.test	11 Jul 2010 14:41:36 -0000
@@ -539,6 +550,7 @@ class TriggerUserActionTestCase extends 
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/trigger/trigger.test	11 Jul 2010 14:41:36 -0000
@@ -682,6 +694,7 @@ class TriggerOtherTestCase extends Trigg
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/update/update.test	11 Jul 2010 14:41:36 -0000
@@ -52,6 +52,7 @@ class UpdateTestHelper extends DrupalWeb
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/user/user.test	11 Jul 2010 14:41:37 -0000
@@ -714,6 +716,7 @@ class UserCancelTestCase extends DrupalW
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/user/user.test	11 Jul 2010 14:41:37 -0000
@@ -1029,6 +1032,8 @@ class UserPermissionsTestCase extends Dr
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/user/user.test	11 Jul 2010 14:41:37 -0000
@@ -1085,6 +1090,8 @@ class UserAdminTestCase extends DrupalWe
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/user/user.test	11 Jul 2010 14:41:37 -0000
@@ -1145,6 +1152,8 @@ class UserTimeZoneFunctionalTest extends
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/user/user.test	11 Jul 2010 14:41:37 -0000
@@ -1185,6 +1194,8 @@ class UserAutocompleteTestCase extends D
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/user/user.test	11 Jul 2010 14:41:37 -0000
@@ -1438,6 +1449,8 @@ class UserEditTestCase extends DrupalWeb
+  protected $profile = 'standard'; ¶

Trailing whitespace.

+++ modules/user/user.test	11 Jul 2010 14:41:37 -0000
@@ -1686,6 +1699,8 @@ class UserTokenReplaceTestCase extends D
+  protected $profile = 'standard'; ¶

Trailing whitespace.

Sorry for printing each one out individually, just makes it easier to reroll. Will post a new patch in a minute.

Powered by Dreditor.

Here we go.

+1 from me too. If anyone is motivated, I think it makes sense to keep converting tests to slim while this sits in rtbc.

The performance benefits of this patch are very compelling, and I especially love this hunk:

+    // Allow tests to use a profile other than the testing one.
+    if (!isset($this->profile)) {
+      $this->profile = 'testing';
+    }

The changes are not nearly as invasive as I would've thought, too, which is nice. And I like this approach much better than previous recommendations, like shoving all of the tests for a module into a single testX() function.

However, as a release manager, I'm obviously incredibly nervous about this:

In general I think it would be best if we continue to convert most of the tests to the slim profile + dependencies for performance and consistency reasons

...when there are 50-something critical issues and growing every day, lately. We passed "polish" phase months and months ago, and distracting a bunch of the Really Smart People on the task of test polishing at this point, which is likely to grow the queue by another 50+ clean-up issues that will all need committer attention makes me go "margh". :\

However, since the testing bot now forms the backbone of our core development process, and since this performance increase will have massive implications going forward in the future, I think the benefits of this patch, despite its unfortunate timing, outweigh the possible detriments, so I'm willing to commit it.

I will leave this RTBC for 24 hours or so to give Dries a chance to chime in, though.

Well, upon further review... a couple of things:

+++ modules/file/tests/file.test	11 Jul 2010 15:10:27 -0000
@@ -16,6 +16,7 @@ class FileFieldTestCase extends DrupalWe
+    $this->createDefaultContentTypes();

+++ modules/simpletest/drupal_web_test_case.php	11 Jul 2010 15:10:35 -0000
@@ -3065,6 +3075,37 @@ class DrupalWebTestCase extends DrupalTe
+  protected function createDefaultContentTypes() {

Let's put this function in one place and call it from both the tests and from standard.install. I can see these getting annoyingly out of sync going forward which is going to be terribly confusing for test debuggers.

+++ modules/trigger/trigger.test	11 Jul 2010 15:10:42 -0000
@@ -252,7 +253,15 @@ class TriggerCronTestCase extends Trigge
   function setUp() {
-    parent::setUp('trigger');
+    $args = func_get_args();
+    if (isset($args[0]) && is_array($args[0])) {
+      $modules = $args[0];
+      array_unshift($modules, 'trigger');
+      parent::setUp($modules);
+    }
+    else {
+      parent::setUp('trigger');
+    }
   }

Dear lord. What is all this belly-dancing? Please document it.

Powered by Dreditor.

Presume this was a cross-post. And yes, good point. ;) Maybe. ;)

Speeding up tests increases our velocity when fixing criticals. Lets try not to see this as distracting core devs.

The patch implemented the correct logic in install_select_profile_form(), but you forgot to put 'hidden = TRUE' in testing.info :)

This time I didn't forget the new files.

Actually I did... (how embarassing...)

While I'm at it, removing the whitespace that crept in there again.

Darn it!!! How DO you add new directories with a patch???

For now I just copied the stuff from your earlier patch into the bottom of my patch file...

You can add new files using fakeadd: http://wimleers.com/blog/cvs-diff-new-files-fakeadd. Otherwise, use bzr or git to generate the diff. There are well known core repositories for each.

Where did we lose the notion of using 'minimal' and came up with a new profile? What's the point?

90%...? Is minimal not minimal enough?? As far as I am aware the only thing it does is adding a few blocks, easy to be get rid of if you dont want. One insert query surely is not 10%?

I'm for using the testing profile, on the grounds of separation.

Then we need a hidden profile feature?

Looks good to me.

This is badly needed, but only almost RTBC.

+++ modules/simpletest/drupal_web_test_case.php	16 Jul 2010 04:26:20 -0000
@@ -3065,6 +3075,17 @@ class DrupalWebTestCase extends DrupalTe
+   * Create default content types.
+   * ¶

(and elsewhere) Trailing white-space.

Please, if your IDE does not support auto-elimination of trailing white-space, and it also does not support to display/highlight EOL/CRLF markers, then please grep your patches for /\s$/ before submitting them. Not doing so wastes a lot of developer time.

+++ modules/simpletest/drupal_web_test_case.php	16 Jul 2010 04:26:20 -0000
@@ -3065,6 +3075,17 @@ class DrupalWebTestCase extends DrupalTe
+  protected function drupalCreateDefaultContentTypes() {
+    module_load_install('standard');
+    standard_create_default_content_types();

1) Replace 'standard' with $this->profile

2) Replace that other thing with

  $function = $this->profile . '_create_default_content_types';
  if (function_exists($function)) {
    $function();
  }

3) More on the actual function/hook name below.

+++ profiles/standard/standard.install	16 Jul 2010 04:26:28 -0000
@@ -427,3 +399,39 @@ function standard_install() {
+ * Create some default content types.
+ * ¶
+ * For a complete list of available node type attributes, refer to the node ¶
+ * type API documentation at: ¶
+ * http://api.drupal.org/api/HEAD/function/hook_node_info.
+ */
+function standard_create_default_content_types() {
...
+  foreach ($types as $type) {
+    $type = node_type_set_defaults($type);

1) "content type" is a Node module concept. First subject for this install profile hook therefore is "node".

2) Follow [subject]_[verb] pattern like modern code elsewhere in core; i.e. "content_types" before "create".

3) Singular, not plural, talking entity. Also, we already have a node type API containing a couple of node_type_* functions.

4) Not sure whether the foreach() shouldn't be part of the function that invokes this install profile hook.

5) Squeezing "install" into the hook name might make sense.

Effectively: hook_node_type_install() ?

Powered by Dreditor.

The actual code changes in the patch are minimal and, now that the content type stuff is out have been reviewed numerous times.

Just a note that this patch adds two module dependencies for tests:
1. 'block' for one of the block tests
2. 'block', 'dashboard', 'comment' for the dashboard test.
I don't know whether this is intentional or not, but
1. since they are mostly obvious (I just don't get 'comment'...) and
2. the tests pass and
3. we want to revisit that in detail in a follow-up anyway
I think the patch is good to go still. Given that it increases test runs dramatically IMO it's not worth holding this up any longer on something that will be discussed (and changed, if needed) in a follow-up anyway.

We (Dries, Damien, and I) talked about this at the Drupalcon CPH Core Developer Summit (tm). Both Dries and I are on board with committing this patch. One caveat: given where we are in the release cycle (and even where we were in the release cycle when this initiative picked up), it makes more sense for the testing profile to default to 'standard' in Drupal 7. Otherwise, every single person who's written a test for their contrib/custom modules in D7 (which is becoming more and more as time goes on) has to add this line to their tests, and as we can see from the 70K patch core needed, this is quite a lot of work. :P In Drupal 8, though, we should invert this and make 'testing' the default.

Patch needs a re-roll, both for this change, and also because testing.profile is missing. ;)

un-tagging.

This should work. Follow-up patches will use the testing profile in more places.

+++ modules/simpletest/drupal_web_test_case.php
@@ -1213,7 +1220,12 @@ class DrupalWebTestCase extends DrupalTestCase {
     // Run default profile tasks.
-    module_enable(array('standard'), FALSE);
+    $install_profile_module_exists = db_query("SELECT 1 FROM {system} WHERE type = 'module' AND name = :name", array(
+      ':name' => $this->profile))
+      ->fetchField();
+    if ($install_profile_module_exists) {
+      module_enable(array($this->profile), FALSE);
+    }

Looks like the comment needs to be updated? It is not necessarily the default profile.

just a quick note there will be a conflict between this patch and #545452: Store install profile in the generated settings.php file which should be fairly easy to resolve, but that is to keep in mind anyways.

Committed to CVS HEAD. Thanks.

Tiny follow-up. We should also fix that list of "absolutely required modules" in a separate issue, because we're treating many totally not required modules as absolutely required currently.

Agreed with #116.

Committed minor follow-up.

FYI, I've opened #913086: Allow modules to provide default configuration for running tests to fix the last gaps