Allow modules to provide default configuration for running tests

.

Alright, let's see what breaks.

Additionally, I really thought of converting a couple of existing test cases in this issue/patch, so as to really make sure it works as expected.

Context is retained. The test cases are in the file form.test, each test case additionally specifies "Form API" as group, SimpleTest outputs all form test cases in that group, and testing results always contain the test case title, the test case class, the test file, and originating function/method. There's really no need to repeat it all over again.

Working some more with the testing profile, it would really help if there was a DrupalWebTestCase::drupalCreateStandardNodeTypes(), which effectively would configure the the identical node types as in standard.install.

The originally proposed patch for the testing profile included an installation profile API change/addition, extracting the node type creation into a dedicated hook_node_type_install() (or similar), which would have allowed to do this in a similar way (though you'd have to manually load standard.install to be able to invoke it). I don't think it's worth to change the installation profile API, so I'd opt for a simple helper method/function that duplicates the node type info and installation from standard.install.

So this is the simple proposal I have in mind. Yes, it hard-codes some stuff, but after all, all we care for is to be able to use the testing profile for more tests.

Slightly tweaked the method name + phpDoc. I'd consider this ready to fly now.

#17: drupal.testing-profile.17.patch queued for re-testing.

Thanks for reviewing! Fixed the wording.

I think that the proposed change is the maximum we can do at this point in time. Because otherwise, we'd most probably introduce an installation profile API change, which would potentially break lots of installation profiles. But effectively, all I want and need is to be able to write and run tests faster.

#20: drupal.testing-profile.20.patch queued for re-testing.

I wouldn't call it an API. You are basically right with your analysis, statement, and goals.

However, facing reality, as of now, almost all of the current tests entirely depend on the configuration that is done in the standard installation profile. To take a simple and straight-forward example, comment tests expect an "Article" node content type, along with the "default" node body field, and comments being enabled for that content type. And so do most other tests.

But let me repeat: I do not want to invent something entirely new here. That is not my intention. The installation profile API should not change. And I also have no clue what Installation Profile API module does, because I never used it.

Since most tests depend on the standard installation profile configuration already, I don't see anything wrong in providing tests a way to load a certain "base" configuration. Note that most of the tests that will use the testing profile along with this base configuration just need *something*. It does not matter at all how the content types are configured or whatever, it only matters that there is some configuration that can be used. For example, the Form API tests that are changed in this patch just need any remotely valid text format and filter configuration. In current HEAD, the tests use the standard installation profile, and therefore, simply use what's there. That's nothing new, most other tests do the same.

So what this patch effectively tries to do is to allow tests to use the testing profile and selectively create the required module configuration only. So instead of installing 28 modules for a single test that has nothing to do at all with Dashboard, Toolbar, and whatnot, we install the baseline of required core modules, possibly create 1-x configurations, and are done with $this->setUp.

Cutting down a test case's runtime to 50% or even less of the current time.

Effectively, this approach does not have any impact on the maintenance burden we already have. For example, if we ever decide to change the "Article" or "Basic page" node types, then all I'm going to say is "Good luck in updating all tests." It sucks, but it's merely the reality.

For D8, we can rethink this from scratch. But for D7, I desperately need faster tests. The testbot has to come back in less than 10 minutes. And that is possible, by making most tests use the testing profile with this approach. Likewise, I have to be able to run tests on my local box again, which is currently impossible, as they take a totally insane time of hours to complete.

@rfay: That idea/patch is D8 material (and was already bumped accordingly).

@webchick: Running one of the Form API test cases (IIRC, FormsTestCase) took 3 minutes on my local box. With this patch, that identical test case takes ~53 seconds.

@rfay: The only test that is actively making use of the new facility is FormsTestCase:

+++ modules/simpletest/tests/form.test	25 Sep 2010 17:58:58 -0000
@@ -7,17 +7,19 @@
 class FormsTestCase extends DrupalWebTestCase {
+  protected $profile = 'testing';
...
-      'name' => 'Form element validation',
+      'name' => 'Element validation',
...
   function setUp() {
-    parent::setUp('form_test');
+    parent::setUp('form_test', 'file');
+    $this->drupalCreateStandardConfiguration('filter');
   }

A couple of other Form API test cases have been converted to use the testing profile, too, but they do not depend on any kind of configuration other than menu callbacks in form_test.module.

+++ modules/simpletest/tests/actions.test	25 Sep 2010 17:58:58 -0000
@@ -67,11 +67,13 @@ class ActionsConfigurationTestCase exten
-class ActionLoopTestCase extends DrupalWebTestCase {
+class ActionsLoopTestCase extends DrupalWebTestCase {
+  protected $profile = 'testing';

Did you flush all caches after applying the patch and before trying to re-run the ActionsLoopTestCase? The class name was renamed for consistency with all other Actions test cases.

Powered by Dreditor.

1. This patch is not done. Lots more has to be done.

The entire testing profile was committed without making tests use it. We agreed that we need to convert tests to use it in follow-up issues. I have tried hard to convert more tests, but ran into limitations. This patch fixes the currently existing limitations, so we can finally move ahead and convert more tests to use the testing profile.

2. To be effective, work will have to be done throughout our tests, potentially destabilizing our testing infrastructure. Why would we do that right now?

I'm not sure how you come to that conclusion. It does not make sense.

3. We don't have numbers yet on how this affects converted and unconverted tests.

I already provided approximate numbers in #32.

The entire point is that most work on D7 critical bugs is delayed by having to wait for test results. I'm sick of having to comment out entire tests in test cases to only run the test I'm interested in, and I only have to do that, because tests are using the standard profile, and solely installing that adds >1 minute of runtime for every single test that is run. Multiply that with the number of tests in a test case, and you constantly sit and wait, for nothing. I've to do that multiple times on every single day currently. And of course, whenever you have to wait for something to complete, you start to switch contexts, to work on other patches "in the meantime", further delaying the progress we badly need to do currently.

More numbers:

Test case: ActionsLoopTestCase ("Actions executing in a potentially infinite loop")
HEAD: 1 min 16 sec -- 27 passes, 0 fails, 0 exceptions
Patch: 46 sec -- 24 passes, 0 fails, 0 exceptions (Note that this test generates a random amount of 3-12 values.)

Test case: FormsTestCase ("Form element validation")
HEAD: 6 min 50 sec -- 267 passes, 0 fails, 0 exceptions
Patch: 4 min 13 sec -- 273 passes, 0 fails, 0 exceptions

That's almost 3 minutes, for a single test case. On average, the current time for running a test case is decreased by ~33%.

I still have no clue how you come to think that this patch would destabilize anything. Without a concrete bug report, no one is going to be able to follow that consideration. This patch is technically not able to destabilize tests, since all tests have clear expectations, and hence, if there would be a missing standard profile configuration, then tests would blatantly fail.

I'm not able to cleanly reproduce a slowdown of tests using the standard profile. However, even if this adds 3-5 seconds to those tests, it doesn't matter at all, as we are going to convert most tests to use the testing profile.

Removed the additional clean-ups in attached patch. Sigh.

OK, if that is what is needed to get this patch in (i.e., apply the patch, copy standard.install into testing.install, revert standard.install, change the method accordingly), and Dries is on board with that, then I'll happily re-roll.

Alright! Attached patch moves that entire modularized configuration into testing.install, not touching standard.install at all.

Sorry, used the wrong patch as base. Attached is a in-patch adjustment. I hope that works.

My stance on the last comment is: It was never the intention of this issue to invent something perfect. We are merely allowing tests to use some helper functions to quickly install/configure "some" kind of default configuration, tests that use those helpers actually do not care for what exactly is there, it just has to be something, and ideally should have a touch of reliability, so that tests don't need to be adapted too often. The situation is not different to almost all of the current tests, with the exception that those rely on the standard installation profile, and that profile can basically change at any time for any kind of reason. Unit tests that actually want to test a particular functionality will not and should not use any kind of default configuration either way (and I seriously hope that we don't have any unit tests relying on a default configuration for the actual functionality anywhere in core).

Improvement options, in the order they cross my mind:

1. Add a $profile function argument. Make it optional, make it default to 'testing'.
2. Move the "fake-hooks" out of testing.install and into $module.install files. In other words, introduce a hook_test_install(), to be placed into $module.install.
3. Change the $module function argument to $modules (plural). Invoke all testing installation configurations subsequently, and do a $this->resetAll() afterwards, so we properly take all possible caches into account.

btw, did you know that we no longer are able to rename simpletest.module to testing.module? It would have to be test.module now, because there's a name clash with the testing profile.

Also, what justinrandell found out in #50 smells like a major bug to me. Only ignored it, as it's totally irrelevant for this issue, but we should open a major issue for that bug (profiles are recorded/saved as modules in {system}).

I'd like to get core maintainer feedback on the possible options for improvements that I listed in #57.

Re-rolled #50 against HEAD.

If a hook_testing_install() in $module.install files is really no longer an option, then I guess we can only go with the current approach of putting individual default module configurations for core modules into testing.install.

Outdated installation functions; didn't contain recent changes for text formats and vocabulary machine names.

Given recent headaches I ran into when changing the site configuration from within the parent site (test code), I've changed the $module argument into a list of $modules, which allows us to invoke $this->resetAll() afterwards, so all configuration changes are guaranteed to be taken over.

This is still one of the most important patches on my list. I think it's crucial for the Drupal product and project at glance.

I'm struggling hard with our current testing performance. From all things that I do, I can say that more than 50% of my time is spent with waiting for testing results. The bottleneck is so huge that I even started to do ROI calculations when considering to write tests for something or not. The evaluation and math behind that is actually simple: Will it take me more time to write and run tests, or will it take me more time to fix bugs along the way as they arise? Needless to say that this is not our intention and everyone should write tests. However, given our current testing performance, it is a perfectly valid math to do.

It is also substantially wrong to tell people to develop on faster machines instead. That is, because the underlying problem of our tests is that they no longer have anything to do with "unit testing". If anything, then we are testing a fully-fledged and bloated Drupal default installation profile and configuration. Why on earth should any of my little contributed modules should have to install and configure Color, Comment, Taxonomy, Menu, Help, Path, Search, Image, File, Contextual, Dashboard, Shortcut, Overlay, Toolbar, RDF, and whatnot modules in order to test the module's own functionality? No one is interested in that BS, my machine is fast enough, thanks. Leaving out that entire needless bloat restores sanity.

The most annoying factor probably is that I'm actually here to help and contribute, and to resolve any kind of (core) issue, it takes me 10 minutes to fix the bug -- but two or more *hours* to wait for tests.

We've clearly reached a ratio that is absolutely unacceptable.

P.S.: Waiting for local test results of #552604: Adding new fields leads to a confusing "Field settings" form while writing this...

#68: drupal.testing-profile.68.patch queued for re-testing.

#68: drupal.testing-profile.68.patch queued for re-testing.

Again, this issue/patch does not try to invent a picture-perfect solution for the problem, and that is especially because we are in the stage that we are in.

This patch also does not remotely intend to tackle support for contributed installation profiles in any way. That's out of scope for this issue.

There's not much code to review, as

1. the patch actually adds one new method only
2. several Form API tests are merely changed to use the testing profile
3. existing lines in standard.install are merely copied over into testing.install in a "modularized" fashion.

Effectively, I'm not sure what to do. FWIW, I've pinged @boombatower via IRC as I have no idea who else I could ping.

All I know is that I desperately need this for my current and future work in Drupal core and contrib.

The problem I have with waiting even longer is that I'm not able to work around this problem in any sane way. AFAIK, I cannot move the problem to Edge, because even though our testing framework supports the notion of 'dependencies', those are not applied by the testbot; which means I cannot have contributed modules rely on an EgdeWebTestCase instead of DrupalWebTestCase. Therefore, the only way to actually use the testing profile would be to duplicate all of the required default module configuration code contained in standard.install into countless of module tests, and just hope that those lines never ever have to be touched again.

Further tests will be converted along the way, no question. But to be able to do that, we need this patch.

Sorry, but we really need this patch. The most simple ways of trying to use the testing profile are blatantly failing:

  protected $profile = 'testing';
...
  function setUp() {
    parent::setUp(array('block'));
...
  function testSaveBlocksForm() {
    $this->drupalPost('admin/structure/block', array(), 'Save blocks');

throws a form error:

Region for Main page content block field is required.

Had to buy new hardware just to develop for Drupal. But tests still take ages. Sigh, kthxbye.

#68: drupal.testing-profile.68.patch queued for re-testing.

Extracted the most annoying WTF, bug, and quickfix into #1366232: drupalCreateUser() breaks if testing site/profile does not install Comment module

I wonder whether we couldn't simply wrap all module-specific configuration actions in standard_install() into module_exists() conditions, so tests can simply load that file and call standard_install()?

  protected $profile = 'testing';

  function setUp() {
    parent::setUp();
    module_load_include('install', 'standard');
    standard_install();
  }

It's not pretty, it's ugly, but it gets the job done. Way simpler than earlier stop-gap proposals.

That would be the most simple solution for this problem space right now, and would allow us to actively use the Testing profile for many more tests instead of resorting to the Standard profile.

I want to make progress here, and at this point, I'm comfortable with any kind of half-baked solution that gets the job done.

#1373142: Use the Testing profile. Speed up testbot by 50% shows that we can effectively cut the time for running all tests by half. And adjusting/fixing only a few tests for the testing profile revealed some serious bugs in functional module code.

We can still design a proper and "perfect" system later.

Better issue title. We're using the Testing profile already, so the previous title was misleading now.

In the follow-up issue #1541298: Remove Node module dependency from Testing profile, I basically stumbled over this very issue again: Lots of tests rely on Node module, but Node module is no longer installed by default. This also means that Node module's default user role permissions for "access content" no longer exist by default. In turn, by merely installing Node module, users are not able to access or create content. In turn, tests are failing.

In light of the recent and upcoming PSR-0 changes for tests, I considered whether there would be any new possible approaches to resolve this issue. However, since all of this only affects functional/integration tests (and not unit tests), I believe that an API hook based on the module/extension system still is the appropriate answer.

The solution should not only work for Drupal core, but also for contributed modules, which can equally have optional dependencies on other modules that do not supply any default configuration for testing. As such, the previous proposals in this issue involving special/conditional code within the Testing profile (which is core-only) do not look acceptable.

Any solution for this problem space will involve fairly special module implementations - regardless of how the default configuration is provided, it is pretty much set in stone, once after it has been defined/committed, since you do not know which other tests depend on it. In light of that, an ideal solution should perhaps involve some kind of versioning mechanism, similar to module schemata: New tests can rely on new default config, whereas old tests can still rely on the old.

Similar to module updates, it might be required to additionally have the actual major version number in the implementation names. Hence, if your module integrates with Views, your tests may rely on default config of Views 2.x or 3.x. -- Scratch that, major version dependencies between tests need to be resolved at a different level of the testing framework/infrastructure. If your module depends on Views 3.x, then there will only be Views 3.x, and so you get its default config.

We never really ran into the issue of default configuration changes in the past, since the Standard profile's configuration was only marginally changed over time. If all default configuration for tests would always be the same, I think that would limit us in the long-term. I consider that especially to be an issue for contributed modules, which don't have any reason for dead-locking their config forever.

One could not only think of versioned configurations, but additionally of default configuration variants. Variants sound a bit too far-stretched though, since the default configuration is merely supposed to act as shortcut/help for test authors. The default configuration is never supposed to be used for testing exactly that actual configuration, but instead, only to deliver some sane defaults to ease in testing of functionality that happens to rely on other functionality that needs configuration. If the test result can vary on that other configuration, then you should use DrupalTestCase::generatePermutations() to verify the expected behavior for all possible variations instead.

Since the goal is to cut down the Testing profile (and Drupal core) more and more to its bare essentials, I can also see a desire of test authors to setup the default configuration for "all modules being installed for this test, except 'these'." In other words, given a testing environment that contains initially nothing, and into which modules are installed that are required for a particular test, I want to setup the default testing config for all of those (few) modules, except the module(s) that I'm actually testing. This turns the definition around; instead of asking for default config for particular modules, you're asking to skip the default config for certain modules. In turn, this would come closest to our current/previous abuse of the Standard profile - by merely asking for modules to install for your test, you also get the default config for each, unless explicitly skipped. This sounds very appealing to me.

Speaking about configuration, the new configuration system obviously comes into mind. However, the default configuration for tests must not be mistaken with the default configuration that can be supplied by modules. And I'm not sure whether testing configuration isn't too special in many cases; e.g., given the above "access content" user role permission example, there'd have to be a config file that only adds that permission to specified user roles - at this point, it's unclear to me whether the config system, API, and also file format will even support that granularity (and if so, when ;)). However, it could definitely be considered for atomic/self-contained default configuration objects, such as text formats and node types. Thus, a new optional ./config_test per module? Sounds appealing, but configuration objects might depend on other configuration objects, which might not be installed or configured, so the default test config would possibly have to be imported conditionally - which ruins the idea.

Sorry for being elaborate. Trying to make sense of this.

Attached patch contains a first prototype.

I've rewritten the summary based on #105 (but shortened).

Please consider May 7th as deadline for reviewing this proposal.

#105 contains a fairly simple patch that demonstrates the proposed API for providing testing configuration.

Note that all testing system performance improvements directly tie into the goals of the Framework initiative (which in turn, ties into parts of former WSCCI). Resolving these kind of issues has much more global effects than on the testing framework alone and each of these issues unblocks a range of dependent issues that further decouple, demystify, and streamline Drupal core to large extents.

Updated issue summary.

Updated issue summary.

So let's see how it would look and work like.

The patch mainly demonstrates default testing configuration for filter formats. Only a few tests are affected, but you will immediately notice how all of that code to setup (completely irrelevant) filter formats is duplicated currently.

It also moves the default permissions for Node module, but that's really the uninteresting and simple part for Node module only. The tough conversion will happen in #1541298: Remove Node module dependency from Testing profile.

Recent work on the child issues of #1595028: Convert tests using Standard profile to use Testing profile instead revealed that it takes ~40 lines of very verbose boilerplate code to e.g. create stupidly simple "Tags" taxonomy field: #2254211: Fix test performance of Drupal\taxonomy\Tests\LegacyTest — That is extraordinarily cumbersome and the internals of quite some of these APIs are really hard to figure out. It's also a maintenance nightmare, since all of that boilerplate code is scattered across hundreds of test classes.

Therefore, I restarted from scratch. The proposed solution I'm presenting here definitely violates all textbook standards. (It's beyond me why PHP 5.4 added support for doing such unholy things in the first place...) But it gets the job done, it works very nicely and cleanly, and the concept and implementation is limited to tests.

To recap: The fundamental situation and requirement is that integration tests are operating with a custom list of modules specific to a test. The functionality of other modules is peripheral, they are needed to create test fixtures only. However, (core and contrib) modules are optional, and additional (contrib) dependencies may not necessarily exist (in the filesystem). Furthermore, test fixture helper classes need to be able to perform assertions on their own (but all test assertion methods are protected).

I started by specifying how I want the API + DX for test fixtures to look like. I want clean and sexy:

    $role   = $this->fixtures->user->createRole();
    $format = $this->fixtures->filter->createFormat(array(), array($role));
    $type   = $this->fixtures->node->createType();
    $node   = $this->fixtures->node->createNode(array(
      'type' => $type->id(),
    ));

As part of the big puzzle of this issue, I also want shortcuts for the most common test fixtures:

    $this->fixtures->user->createRequiredRoles();
    $this->fixtures->filter->createFormatRestrictedHtml();
    $this->fixtures->node->createTypeArticle();
    $this->fixtures->taxonomy->createVocabularyTags();

...whereas (1) it's up to each module to decide what test fixtures are generally available and (2) whether to provide dedicated helpers for more common fixtures.

As in HEAD, test fixture methods need full access to the test class instance. Partly, because they need to rebuild data structures and/or flush caches (which is intentionally not done by the underlying API functions), and second, because they need to report the performed operations in test results.

Traits are not an option, because a missing trait causes PHP to blow up at parse/compile time. The adapter pattern is not an option, because it would require all methods and properties on test base classes to be public instead of protected.

Now comes PHP 5.4: Closures can be re-bound to a different object and scope. And ReflectionMethod allows to create a Closure from any class method.

Combine the two aspects and it is possible to dynamically compose pluggable class methods out of various independent classes, which are operating in the scope of $this.

TestBase $this → FixtureManager $fixtures → node ← NodeFixtures
           ↑                                            ↓
            ————————————————————————————————————————————

This patch is a working prototype and proof of concept, including a test. I mainly copied the existing drupalCreate*() methods from WebTestBase into corresponding module-specific *Fixtures classes. Only DUTB is implemented/supported right now, but the idea is that both DrupalUnitTestBase and WebTestBase will use the identical mechanism and only one fixture class per module.

I am fully aware that this is a bit crazy, so there's no need to point that out. However, the driving code is surprisingly simple (in essence, ~10 lines of code), the public/developer-facing concept and construct is very simple to understand and explain, it resolves a long-standing problem, and again, it is limited to tests. :-)

Looks like some tests are double-enabling / double-disabling some modules. Not going to look into that until there's buy-in/agreement on the proposed solution.

@webchick:

The API of entities is not a problem. We already have (entity) APIs. Tests and test fixture helper methods are using the APIs already.

Tests need a vastly simplified way to quickly create fixtures ad-hoc, using randomString()s and randomName()s for many properties. To reduce verbosity, test fixture helper methods are using random values all over the place to cover the >90% use-case for tests. For example, drupalCreateNodeType() does not require any arguments; it will simply create a random new node type.

Some test fixture helpers are combining multiple APIs in a sensible way to further reduce verbosity. For example, drupalCreateUser() does not only create a user account, it also creates a new user role for the specified user permissions.

And that's an aspect that should not be solved at the (entity) API level, as it would introduce hard interdependencies between various APIs that are independent otherwise. For example, just with pure APIs, creating a simple taxonomy "Tags" field requires to call into many different APIs:

    $vocabulary = entity_create('taxonomy_vocabulary', array(...))->save();
    entity_create('field_config', array(...))->save();
    entity_create('field_instance_config', array(...))->save();
    entity_get_form_display('node', 'article', 'default')
      ->setComponent($field_name, array('type' => 'taxonomy_autocomplete'))
      ->save();

For the sake of a sane test authoring DX, that should be shortened to something along the following lines:

    $field = $this->fixtures->taxonomy->createTagsField('node', 'article');

In addition, after creating a test fixture, the helper method typically has to reset/clear a bunch of related caches (or rebuild the router, etc.pp.), so as to make the new fixture immediately available to the system under test. Such operations are intentionally not performed by API methods, because they are also used in e.g. batched operations — in many cases, it's the responsibility of the caller to clear caches and rebuild data structures.

There's nothing wrong with improving the DX of the underlying (entity) APIs, but test fixtures are a layer that sits above and on top of those APIs; test fixture helper methods are simply using whichever APIs are made available to them.