Problem

  • Tests that require other, optional modules cannot rely on any kind of "default" configuration for executing a test, and thus are forced to implement lots and complex code to create some.

Goal

  • Allow tests to rely on any kind of default configuration provided by other optional modules they depend on.

Details

  • #1541298: Remove Node module dependency from Testing profile and many other issues run into the same problem; e.g., in that case:
    1. Lots of tests rely on Node module
    2. But Node module is no longer installed by default.
    3. Node module's default user role permissions for "access content" no longer exist by default.
    4. By merely installing Node module, users are not able to access or create content.
    5. Lots of test failures.
  • Modules need to be able to provide default configuration for tests.
  • The default configuration for tests may be entirely different to the regular default configuration of modules being setup via config and hook_install(). (not to be confused)
  • The testing configuration rather maps to our previous, unintended abuse of the Standard profile - whatever was defined there, all tests that did not care simply used it.
  • The problem space exponentially increases when considering contributed modules that were never able to rely on the Standard profile in the first place. Module Z that relies on module Y which relies on module X needs to create testing configuration for both Y and Z in order to test X. (both Y and Z are otherwise irrelevant for the test, just need to exist to make X work)
  • Any default testing configuration, regardless of how it is implemented, will be set in stone once after it has been committed, because it might be used by tests in other projects.
  • An ideal world solution would therefore account for testing configuration "versions" and/or variants. But we possibly want to defer that complexity to later.
  • Any default testing configuration is never supposed to serve as basis for testing exactly that actual configuration. It is only ever used for (other) tests that "just need something" to run. If the test result can vary on that other configuration, then DrupalTestCase::generatePermutations() has to be used to verify the expected behavior for all possible variations instead.
  • The ultimate goal is to reduce required dependencies of Drupal core to an absolute minimum, in order to (drastically) speed up the Testing profile. In a sense, the (utopian) goal is achieved when a functional/integration test completes as fast as a unit test (e.g., a test for System module that doesn't require any other dependencies than System module itself).
  • Pretty much all tests will be asking for default testing configuration for all other modules that they need to install.
  • In turn, it would make most sense to install the default testing configuration for all modules being installed for a test by default, except if a certain module was asked to be skipped. 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. The simplicity in this sounds very appealing.

Notes

  • Recent PSR-0 seem unrelated, since all of this affects functional/integration tests (and not unit tests), which fully rely on the module system in the first place, so an API hook based solution sounds most appropriate.
  • The new configuration system for D8 does not seem to help either, because the default testing configuration may only be conditional, depending on other modules being installed for a test. Additionally, it is unclear whether the new configuration system will support the required granularity in the first place (e.g., granting "access content" for two specific roles, as in above mentioned example).

Proposed solution #105

  1. Introduce hook_testing_install() to allow modules to provide default testing configuration.
  2. Make setUp() invoke hook_testing_install() for all modules being installed automatically.
  3. Allow the test case to skip the default testing configuration for certain modules (so it can be set up manually).

Our tests take longer and longer to run (~30 minutes on highly optimized testbots recently), so we really should make the new testing profile work properly.

Since my local box is quite slow, I've tried and tried and tried to use the testing profile all over again, but in almost most all cases, I was hitting annoying bugs caused by code that still relies on the standard profile.

This is just a start, the most obvious bug. We should try to convert a couple of core tests here to make sure that the profile really works like it should.

Files: 
CommentFileSizeAuthor
#112 test.fixtures.112.patch19.38 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch test.fixtures.112.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#108 drupal8.testing-install.108.patch12.46 KBsun
FAILED: [[SimpleTest]]: [MySQL] 36,202 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#105 drupal8.testing-config.105.patch4.09 KBsun
PASSED: [[SimpleTest]]: [MySQL] 35,299 pass(es).
[ View ]
#98 fasttestsplz.patch20.78 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fasttestsplz.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#68 drupal.testing-profile.68.patch21.2 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.testing-profile.68.patch. See the log in the details link for more information.
[ View ]
#66 trim.test.bloat_.patch21.42 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 28,652 pass(es).
[ View ]
#65 drupal.testing-profile.65.patch21.14 KBsun
PASSED: [[SimpleTest]]: [MySQL] 28,603 pass(es).
[ View ]
#61 drupal.testing-profile.61.patch21.36 KBsun
FAILED: [[SimpleTest]]: [MySQL] 28,369 pass(es), 2 fail(s), and 1 exception(es).
[ View ]
#50 simpletest.patch20.83 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 25,321 pass(es).
[ View ]
#47 drupal.testing-profile.47.patch20.83 KBsun
FAILED: [[SimpleTest]]: [MySQL] 25,048 pass(es), 1 fail(s), and 1 exception(es).
[ View ]
#46 drupal.testing-profile.46.patch26.86 KBsun
FAILED: [[SimpleTest]]: [MySQL] 25,062 pass(es), 1 fail(s), and 1 exception(es).
[ View ]
#39 drupal.testing-profile.39.patch18.06 KBsun
PASSED: [[SimpleTest]]: [MySQL] 25,214 pass(es).
[ View ]
#20 drupal.testing-profile.20.patch24.08 KBsun
PASSED: [[SimpleTest]]: [MySQL] 25,229 pass(es).
[ View ]
#17 drupal.testing-profile.17.patch24.01 KBsun
PASSED: [[SimpleTest]]: [MySQL] 25,231 pass(es).
[ View ]
#15 drupal.testing-profile.15.patch23.57 KBsun
PASSED: [[SimpleTest]]: [MySQL] 24,809 pass(es).
[ View ]
#10 drupal.testing-profile.10.patch13.11 KBsun
PASSED: [[SimpleTest]]: [MySQL] 24,813 pass(es).
[ View ]
#7 drupal.testing-profile.7.patch1.39 KBsun
PASSED: [[SimpleTest]]: [MySQL] 24,780 pass(es).
[ View ]
drupal.testing-profile.0.patch1.62 KBsun
PASSED: [[SimpleTest]]: [MySQL] 24,796 pass(es).
[ View ]

Comments

sun’s picture

Issue tags:+Performance, +Needs tests

.

sun’s picture

Priority:Major» Normal
moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

yeah, i agree that we need to keep working on testing profile compatibility. easy rtbc here.

rfay’s picture

subscribe

Dries’s picture

Status:Reviewed & tested by the community» Needs work

Personally, I think this is a pretty ugly approach. Hard-coding knowledge about specific modules is not going to buy us a lot of credit. It makes the behavior hard to predict, the API harder to understand, and it makes the function harder to use in certain cases. This is quite a hack, IMO.

moshe weitzman’s picture

All I can think to do then is to remove comment and 'access content' perms from drupalCreateUser() and then add those perms manually to any tests that break.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new1.39 KB
PASSED: [[SimpleTest]]: [MySQL] 24,780 pass(es).
[ View ]

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.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

apparently nothing broke.

Dries’s picture

Interesting ... sounds like a good clean-up in that case.

sun’s picture

StatusFileSize
new13.11 KB
PASSED: [[SimpleTest]]: [MySQL] 24,813 pass(es).
[ View ]
moshe weitzman’s picture

Lets see how bot likes that. Meanwhile, I'm not so sure about name changes like:


'name' => 'Form triggering element determination',
+ 'name' => 'Triggering element',

Our code gets less grokkable without reading a bunch of surrounding code. Linus explains it pretty well at http://www.realworldtech.com/forums/index.cfm?action=detail&id=110618&threadid=110549&roomid=2

sun’s picture

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.

moshe weitzman’s picture

That’s all true. But I was referring to context for the developer during grep or when exchanging patch files and so on. It isn't a big issue in this case.

sun’s picture

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.

sun’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new23.57 KB
PASSED: [[SimpleTest]]: [MySQL] 24,809 pass(es).
[ View ]

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.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

Looks reasonable to me. We're on a path for faster tests which is critical to the success of unit testing in contrib

sun’s picture

Issue tags:-Needs tests
StatusFileSize
new24.01 KB
PASSED: [[SimpleTest]]: [MySQL] 25,231 pass(es).
[ View ]

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

sun’s picture

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

Dries’s picture

+++ modules/simpletest/drupal_web_test_case.php 21 Sep 2010 22:48:45 -0000
@@ -908,6 +908,31 @@ class DrupalWebTestCase extends DrupalTe
+   * Creates a default configuration from the Standard installation profile.

I don't think we need to capitalize 'standard'. Appears in a couple places.

+++ modules/simpletest/drupal_web_test_case.php 21 Sep 2010 22:48:45 -0000
@@ -908,6 +908,31 @@ class DrupalWebTestCase extends DrupalTe
+   *   The module name to create the default configuration of the Standard
+   *   installation profile for; e.g., "node" to invoke _standad_install_node().

I don't understand this sentence.

I'm not yet convinced about this approach. Will think about it more.

sun’s picture

StatusFileSize
new24.08 KB
PASSED: [[SimpleTest]]: [MySQL] 25,229 pass(es).
[ View ]

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.

Status:Reviewed & tested by the community» Needs work
Issue tags:-Performance

The last submitted patch, drupal.testing-profile.20.patch, failed testing.

sun’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+Performance

The last submitted patch, drupal.testing-profile.20.patch, failed testing.

sun’s picture

Status:Needs work» Reviewed & tested by the community
webchick’s picture

Status:Reviewed & tested by the community» Needs work

Dear sweet lord. :( What's with all the kitten killing in this patch?!

Can you please sheer this down to only the parts that are actually fixing the bug, and move all of these test/testcase renames/comment culling to some other patch? It's impossible to review atm.

webchick’s picture

Ok, after sifting through this patch for 30 minutes trying to figure out what was actually important, I think what you're doing here is taking an Install Profile API approach to standard.install, both to split up the code into smaller chunks and also to make them into API functions that can be called selectively by individual tests to bootstrap parts of the system relevant to their tests.

Like Dries, I'm not sure I like this approach, either. standard.install has a very specific target audience: people who are either brand new to Drupal, or who want a quick bootstrap start to their Drupal sites with some fairly sane defaults that they can use to branch off from there.

Tests also have a very specific target audience, and it is the exact opposite audience of the standard profile.

We need the flexibility to adjust the standard profile and what types of features it offers out of the box, what blocks it positions around the page, and so on, without having to worry about the implications this has on the testing side. Coupling these two things together seems like it'll result in fragile tests, or enormous test re-work required anytime anything at all changes on the default profile side, or both.

I'm curious why this approach to creating one or more "starter" functions wasn't done on testing profile instead, since that's what this profile is there for? Is it to reduce duplication of code, or..?

sun’s picture

Status:Needs work» Needs review

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.

beejeebus’s picture

@sun - awesome work! i've been meaning to get back to this after the testing profile patch landed. this looks a lot nicer than the "build some basic configuration" stuff we started on and ripped out of the testing profile patch.

i just wanted to weigh in with a big +1 on #27. our tests right now are just total, utter, bloat. what we're doing here is just cutting that down. we'll still be in the same position re. unintended coupling etc, but we'll have faster tests.

fixing the general approach to writing tests is for D8 material, but making things faster is for D7.

rfay’s picture

Just a note to remember that there are some other very significant approaches to improving test performance.

See #449186: Implement DatabaseConnection::duplicate().

It's rebuilding the entire testing environment on innodb for every test that kills us. @justinrandell explored some additional paths here, including consolidating tests (there's no need in many cases for tests to be in separate methods). I think the big win will be when we can replicate a standard database (probably one per class, or perhaps child classes could inherit the parent's db and specialize it.)

It is definitely true that our current situation does not scale. It will probably get us through the release, but it does not scale from here.

webchick’s picture

Oh, sorry. That bit about install profile "API" wasn't my normal grumbly core committer use of the word "API". ;) Install Profile API is a module that lets you selectively 'include' helper functions, like "user_add_permissions" and "cck_import_type_from_text" (I forget the exact names) to aid in the development of installation profiles and/or update hooks. So if you want to import 'node' helper functions you do install_include('node') and for CCK ones you do install_include('cck'). The comparison seems fairly apt here, since tests would be "including" the portion of the standard install that's relevant to what their test does.

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.

Well, that is true. We still have to change 50,000 tests when we change "Page" to "Basic page", as we did earlier this release before we had this mechanism.

I still am cagey about the mis-matched coupling here, and the change to essentially recommend it as a best practice in D7, but you are correct that it's not wildly different than what exists now.

Do we any have stats by chance on what the test run time is before/after this patch? Does testbot report that? Sorry if it's above and I missed it; I have to run catch a taxi. :\

sun’s picture

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

sun’s picture

@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’s picture

@sun, I know that's D8, of course. I just wanted to mention when we're talking about testbot performance that it's hugely important as we're on an unsustainable path, and try to put some context in here.

I am astonished that you got 3x performance with this patch. I'll try to make some time for it today.

rfay’s picture

Status:Needs review» Needs work

Perhaps I don't understand the situation, or there's something wrong with my tests, but I both get a regression in Simpletest and *poorer* performance with this patch.

The setup: My development machine using the web interface (so concurrency is 1).

1. If I try to run the two Actions tests, it runs them both if the patch is not applied, but only runs the second one with the patch applied. That, of course, invalidates the performance timing I was trying to run on that one.

2. The hoped-for performance doesn't show in these arbitrarily selected examples:

Test description Running time/Assertions, CVS HEAD Running time/Assertions, with #20
AJAX Commands + AJAX Command Form Values 2:20/91 2:29/91
Actions Configuration + Actions executing in a potentially infinite loop 1:34/63 0:46/38 (INVALID)
Trigger Example + Simpletest Example in Examples project 2:59/56 3:06/56
sun’s picture

Status:Needs work» Needs review

@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.

rfay’s picture

Version:7.x-dev» 8.x-dev
Status:Needs review» Needs work

@sun, you're absolutely right that flushing the cache brings that back, so that regression was bogus.

I did not know that this was just a demonstration of a couple of tests. In that case, I think it should be deferred to D8, as there's no reason to destabilize our testing framework.

I'm baffled why you made all kinds of editorial and class naming changes in a patch that has a specific purpose. I know that getting D7 out is near and dear to your heart. D7 *and* testing performance are near and dear to mine. Why sneak your editorial opinions into a patch like this? (I don't disagree with any of them. I just would prefer this patch to succeed and I want D7 to get out, and that means focus.)

I am -1 on this now.

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

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?

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

I suspect this should go into D8, sadly.

+++ 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 {
...
-      'name' => 'Actions executing in a potentially infinite loop',
-      'description' => 'Tests actions executing in a loop, and makes sure they abort properly.',
+      'name' => 'Actions infinite loop',
+      'description' => 'Tests actions executing in an infinite loop, and makes sure they abort properly.',

+++ modules/simpletest/tests/form.test 25 Sep 2010 17:58:58 -0000
@@ -7,17 +7,19 @@
-      'name' => 'Form element validation',
+      'name' => 'Element validation',
...
-    parent::setUp('form_test');
+    parent::setUp('form_test', 'file');
+    $this->drupalCreateStandardConfiguration('filter');

@@ -370,16 +372,18 @@ class FormsTestCase extends DrupalWebTes
-      'name' => 'Form alter hooks',
+      'name' => 'Alter hooks',

@@ -484,10 +490,11 @@ class FormValidationTestCase extends Dru
-      'name' => 'Form element and label output test',
+      'name' => 'Labels and markers',

@@ -557,10 +564,11 @@ class FormsElementsLabelsTestCase extend
-      'name' => 'Tableselect form element type test',
+      'name' => 'Tableselect',

@@ -764,11 +755,12 @@ class FormsElementsTableSelectFunctional
-      'name'  => 'Multistep form using form storage',
-      'description'  => 'Tests a multistep form using form storage and makes sure validation and caching works right.',
+      'name' => 'Multistep form storage',
+      'description' => 'Tests a multistep form using form storage and makes sure validation and caching works right.',

@@ -914,9 +906,11 @@ class FormsFormStorageTestCase extends D
-      'name' => 'Form wrapper callback',
+      'name' => 'Wrapper callback',

@@ -992,7 +988,7 @@ class FormStateValuesCleanTestCase exten
-      'name' => 'Form rebuilding',
+      'name' => 'Rebuild',

@@ -1082,10 +1078,11 @@ class FormsRebuildTestCase extends Drupa
-      'name' => 'Programmatic form submissions',
+      'name' => 'Programmed submissions',

Powered by Dreditor.

sun’s picture

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review

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.

rfay’s picture

OK, I'd love to have a great performance improvement go in as well. Probably not in D7 unfortunately. IMO this is distracting from the goal of a release and potentially destabilizing. (Note that it would not have appeared so destabilizing to me if you'd kept the patch to the specific issue at hand rather than doing editorial work in the same patch. Please do editorial work in an editorial patch instead of sneaking it into a performance patch.)

We need a bit more formal benchmarks (and perhaps an explanation of why it makes unconverted changes a bit slower).

With greatest respect,
-Randy

sun’s picture

StatusFileSize
new18.06 KB
PASSED: [[SimpleTest]]: [MySQL] 25,214 pass(es).
[ View ]

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.

webchick’s picture

Sure, let me be clear. I'm all about speeding up test runs. We committed the testing profile patch at Drupalcon CPH 12+ months after API freeze for the very reason that the velocity of core development increases when tests run faster.

My concern though is that the way this patch goes about it is taking our worst practice (because we didn't really know better back then) -- writing tests that rely on what's in standard.profile in order to work -- and effectively encoding it as a recommended performance improvement mechanism. This is going to naturally spin out into contrib tests, which will look to core for guidance on what to do. This is what I take issue with.

Because, in Drupal 8, I think we will want tests that do not have gazillions of dependencies on stuff in standard.profile, since that necessitates crap like a 28K patch to do minor stuff like rename "Page" to "Basic page". So if we're going to be making changes to how tests define their requirements in order to speed up testing, encoding calls to _standard_* as a best practice seems like a step backwards to me.

We should take a moment and think about how we want this to work in the future. A couple of scenarios I can think of:

1. Each test instantiates its own test data, independently of all other tests, and we retain testing profile as "barebones"; just enough to bootstrap Drupal. This results in lots of duplicated code, but OTOH lots of self-contained code that isn't impacted by arbitrary changes in standard.profile or in other tests, leading to easier maintenance.

2. Testing profile, not Standard profile, exposes API functions for creating default test data. This forms the relationship between Testing profile, rather than Standard profile, eliminating the ooky dependency there. OTOH, changes to standard profile that may affect tests need to be made in two places.

3. The proposed approach, which likely results in fewer net changes in terms of lines of code, but couples tests and standard profile together for the lifetime of the Drupal 7 release, and codifies this as a best practice for all Drupal 7 contributed modules. Not that much different to what we do now, but huge amounts of clean-up work in D8, assuming we don't want to move forward with that coupling as a general rule.

I'm not sure what exactly Dries found troubling about the patch and if this angle was it. Obviously, there are pros/cons with any approach, but I'd love to discuss this a bit. My understanding when we committed the testing profile back in Copenhagen was that we were going for approach #1. I'm not sure how we ended up at approach #3, which seems the least desirable in terms of where we ideally want to go.

Thoughts?

webchick’s picture

And the more I look at it, the more I really don't like the way this is split up in standard.profile. Standard profile is the "product" of Drupal core; if split, it would be split up into functions like _standard_install_content() which included content type set up (image/taxonomy included), any default nodes, etc. and _standard_install_people() which included stuff like default roles/permissions, and default users (if any), or whatever. Basically, functions grouped by logical actions/portions of the profile.

Naming these by _MODULE() in something that is not remotely geared toward module developers sounds like a needless hurdle for the non-technical core contributors (e.g. UX team) who need to be focused on the utility of standard.profile.

So I'll re-iterate once more, why can't we move these helper functions into testing profile instead, and have them blurt out stuff like "Test Node 1/2/3" and "Test Text Format 1/2/3"? The testing profile makes total sense to split up by module name, and makes total sense to couple to .test files.

webchick’s picture

Title:Testing profile does not work» Allow testing profile to speed up test runs
Priority:Normal» Major

Also, adjusting issue title and increasing priority so it stays on the radar.

catch’s picture

Status:Needs review» Needs work

I also like adding these to the testing profile, even if they end up being exact copies of the functions in standard profile initially. Marking needs work for that change, don't think this is going in as is, not a big change to update the patch.

sun’s picture

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.

Dries’s picture

My concerns with this patch where exactly the same as webchick's -- sorry for not articulating that in more detail.

The approach in #41 sounds a lot better to me, and does not paint ourselves in a corner that might be expensive to get out of.

I'm all for improving the performance of the tests so let's explore the solution suggested in #41 and see if it makes a material difference.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new26.86 KB
FAILED: [[SimpleTest]]: [MySQL] 25,062 pass(es), 1 fail(s), and 1 exception(es).
[ View ]

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

sun’s picture

StatusFileSize
new20.83 KB
FAILED: [[SimpleTest]]: [MySQL] 25,048 pass(es), 1 fail(s), and 1 exception(es).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, drupal.testing-profile.47.patch, failed testing.

beejeebus’s picture

investigationing.

<?php
mysql
> select filename from system where name = 'testing' and type = 'profile';
Empty
set (0.00 sec)
?>

would seem to the issue.

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new20.83 KB
PASSED: [[SimpleTest]]: [MySQL] 25,321 pass(es).
[ View ]

updated patch attached, only changed this line:

<?php
   
require_once DRUPAL_ROOT . '/' . drupal_get_path('profile', 'testing') . '/testing.install';
?>

to

<?php
   
require_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'testing') . '/testing.install';
?>

any other changes are just moves to update with HEAD.

beejeebus’s picture

woo, all green. i think this is RTBC, but i'll wait for others to way in.

moshe weitzman’s picture

Sorry, I don't really get this approach.

+++ modules/simpletest/drupal_web_test_case.php 30 Sep 2010 09:28:30 -0000
@@ -912,6 +912,32 @@ class DrupalWebTestCase extends DrupalTe
+   *   The name of a module, for which a corresponding _testing_install_MODULE
+   *   function exists in the testing installation profile. For example, pass
+   *   "node" to invoke the corresponding function _testing_install_node().
+   *   The corresponding function must exist in testing.install.

Is it a good idea to require functions to be in testing.install? Aren't we then asking sites to hack testing.install? I am thinking of sites doing setup() for their custom code. Perhaps we just remove talk of testing.install.

Powered by Dreditor.

sun’s picture

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).

beejeebus’s picture

i don't think we need to worry about providing a way for contrib modules to setup default configuration in the testing profile. i think that's way beyond the scope of this patch, so i'm ok with it how it stands now.

the most important thing is that we're moving towards starting with (almost) nothing, and explicitly creating what is needed for a test.

this is still RTBC from my point of view.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

OK, good enough.

webchick’s picture

Status:Reviewed & tested by the community» Needs review

Since this has the potential to break a lot of things, this isn't going to go in before beta 1, although I'd like to commit it basically immediately after. So since we have at least a few hours ;) let's discuss this a bit more.

Moshe's feedback is definitely valid. drupalCreateTestConfiguration() is basically completely useless for instantiating data outside of core (or really, worse than that... outside of what testing.install provides for helper functions). Since contrib modules can't hack testing.install to pop in _testing_install_flag(), for example, they'll need to set up their own wrapper functions or find alternative workarounds. This introduces a DX WTF since the mantra is always "When in doubt, do what core does." And you can't. :\

I get that now that we're 7 alphas into release cycle we don't want to refactor the entire damn testing system (or well. we do, but we can't ;)), but I wonder if there's a way to make that function a more general wrapper that could work generically for any $profile.install, perhaps? Dunno. Just throwing that out there.

If we don't come up with any better alternatives, I would be ok committing #50. It addresses all of my previous concerns about coupling tests to standard.profile, and it seems it will certainly speed things up a lot.

sun’s picture

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}).

beejeebus’s picture

ok, cool, thanks sun and webchick.

unless sun beats me to it, i'll have a go at the list in #57 after beta.

sun’s picture

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

webchick’s picture

I don't really understand why this needs committer feedback. The community is perfectly able to respond to those suggestions.

So the goal is to make contrib be able to do what core does to instantiate test data.

That basically rules out 1, because contrib modules can't include their own profiles. I suppose they could reference a sister contrib profile, but then our installation profiles list gets bloated with all of these "Flag Tester profile" type profiles instead of actually useful bases for distributions.

2. is a significant API change. We'd have to update documentation in a lot of places that talks about how to write SimpleTests, what hooks that are enclosed in .install files, etc.

3. I don't really get. Where would this $module configuration come from? But it smells like it'd be yet another API change.

Given that we're almost at RC now, we probably just need to go with #50 at this point and make some @todos for Drupal 8 unless someone has better ideas.

sun’s picture

StatusFileSize
new21.36 KB
FAILED: [[SimpleTest]]: [MySQL] 28,369 pass(es), 2 fail(s), and 1 exception(es).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal.testing-profile.61.patch, failed testing.

beejeebus’s picture

blurgh, fails to apply. will attempt a reroll. sun, thanks for keeping this alive.

beejeebus’s picture

or not. /me runs off to get a coffee...

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new21.14 KB
PASSED: [[SimpleTest]]: [MySQL] 28,603 pass(es).
[ View ]

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

beejeebus’s picture

StatusFileSize
new21.42 KB
PASSED: [[SimpleTest]]: [MySQL] 28,652 pass(es).
[ View ]

fix the format creation fail.

EDIT: crosspost, #65 is what we should work off. i'll hold off until we get green.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

We have green for the last two posts. Here is an RTBC for #65. justinrandell just agreed to this over IRC.

sun’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new21.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.testing-profile.68.patch. See the log in the details link for more information.
[ View ]

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.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

Sounds good. Back to RTBC.

sun’s picture

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...

sun’s picture

Issue tags:-Performance

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

sun’s picture

Issue tags:+Performance

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

moshe weitzman’s picture

Still green, still important. Testing with contrib modules will be even slower than core is now, which is saying something.

rfay’s picture

@moshe, could you expand on what you're saying? Currently contrib modules test pretty fast because they don't run the entire core suite, just their own, which makes it a lot smaller test.

webchick’s picture

Status:Reviewed & tested by the community» Needs review

I'm never going to be anti-speeding up test runs. However, there are significant architectural limitations of this approach that make it feel non-core-worthy to me as they currently sit. See #56. And it's a big deal to commit this because at the point we're currently at (and were at when the patch was originally RTBC) we don't have time to go back for do-overs.

I don't see representation in this issue from most of the people who worked on the original testing framework. That worries me. I don't see thorough reviews. That worries me. It also worries me that we're going at this in a "just stop the bleeding" sort of way rather than a "how would we ideally fix this" kind of way.

My gut feeling is we're better off widening this discussion and thinking, fixing this performance issue "for real" in D8, and then determining if the approach is back-portable or not.

I guess that makes this "needs review", or something.

sun’s picture

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.

webchick’s picture

boombatower, chx, damz, rfay, and beejeebus are people i generally associate as being knowledgeable about the testing framework. I haven't seen detailed reviews of this approach from any of them (or anyone else, for that matter).

Edit: Except for rfay, who was -1 and subsequently bumped it to D8 earlier.

beejeebus’s picture

hmmm. i like the idea of fixing our testing approach for D8 in a more elegant, powerful way. i like the idea of a better fix being back-ported when D8 starts.

i'm ok with this patch not going in on the basis that this is one of the things we can backport, but we really, really need to fix our approach to testing, because the current bloat is both ridiculously slow and really, really bad testing practice.

sun’s picture

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.

Damien Tournoud’s picture

So, if I read the logs correctly, the test run of #68 by testbot #559 took 31 minutes 30 seconds, while the base line of #559 is 32 minutes (ie. a 1.5% decrease). This patch converted 13 new test classes to the testing profile (ie. 3% of the 417 classes we have in core).

Sounds like a good deal overall. That said, there is still 97% of classes to convert, and I wonder would will do the crazy work required to come up with stuff like this:

<?php
+++ modules/simpletest/tests/form.test    30 Nov 2010 22:55:03 -0000
@@ -7,6 +7,7 @@
  */
 
class
FormsTestCase extends DrupalWebTestCase {
+  protected
$profile = 'testing';
 
   public static function
getInfo() {
     return array(
@@ -
17,7 +18,8 @@ class FormsTestCase extends DrupalWebTes
  
}
 
   function
setUp() {
-   
parent::setUp('form_test');
+   
parent::setUp('form_test', 'file');
+   
$this->drupalCreateTestConfiguration(array('filter'));
   }
 
  
/**
?>
sun’s picture

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

sun’s picture

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.

sun’s picture

Version:7.x-dev» 8.x-dev

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

rfay’s picture

OK, now is the time to get this in!

BTW: Since the testbot packages are now packaged as debian debs, the highly-optimized db-in-memory testbot is something everybody can have quite easily, even as a VM. I haven't done anything about packaging it for that or promoting it, but it would work just fine.

sun’s picture

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

RobLoach’s picture

Test driven development is impossible with Drupal when running a single test takes this long.... #34496: [meta] Add Flag module to allow users to subscribe/unsubscribe without posting a comment

rfay’s picture

@Rob - this took 30 minutes, which is about what the core suite takes now. Were you saying it takes longer than that or were you saying that that's way too long. (Lots of people would agree with you).

If I may, I should add that test-driven development does not mean "throw a patch at the testbot and see if it passes". It actually means build the tests before you build the code, if I understand right.

RobLoach’s picture

@rfay: Running the tests locally before this patch is really slow. Even just a single test took a ridiculous amount of time. For my last project, I wrote a bunch of tests before doing development, and then during development, continuously ran the tests, and it just took way too long... I'll hit up this patch and see it go in comparison.

RobLoach’s picture

[EDIT] .... Yay for comment.module not protecting against duplicate comments.

jbrown’s picture

Ideally we would have

protected $profile = 'testing';

in DrupalWebTestCase

It's inappropriate and really slow to be using the 'Standard' installation profile for testing.

sun’s picture

Category:bug» task
catch’s picture

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

moonray’s picture

following...

xjm’s picture

Tagging issues not yet using summary template.

sun’s picture

Issue tags:+Testing system
RobLoach’s picture

Status:Needs review» Needs work
Issue tags:+Performance, +Needs issue summary update, +Testing system

The last submitted patch, drupal.testing-profile.68.patch, failed testing.

RobLoach’s picture

Status:Needs work» Needs review
StatusFileSize
new20.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fasttestsplz.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Re-roll.

RobLoach’s picture

Status:Needs review» Needs work

Umm, do we still need this?

rfay’s picture

I think this is one of a couple of initiatives that are critical to our testing resources.

I encourage it to move forward at this point. This is a great time in the dev cycle to make it happen.

sun’s picture

sun’s picture

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.

jbrown’s picture

Status:Needs work» Needs review
Issue tags:-Performance, -Needs issue summary update, -Testing system

#98: fasttestsplz.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Performance, +Needs issue summary update, +Testing system

The last submitted patch, fasttestsplz.patch, failed testing.

sun’s picture

Title:Allow testing profile to speed up test runs» Allow modules to provide default configuration for running tests
Status:Needs work» Needs review
StatusFileSize
new4.09 KB
PASSED: [[SimpleTest]]: [MySQL] 35,299 pass(es).
[ View ]

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.

sun’s picture

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.

sun’s picture

Issue summary:View changes

Updated issue summary.

sun’s picture

Issue summary:View changes

Updated issue summary.

beejeebus’s picture

i like hook_testing_install(). +1 to the concept.

sun’s picture

StatusFileSize
new12.46 KB
FAILED: [[SimpleTest]]: [MySQL] 36,202 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal8.testing-install.108.patch, failed testing.

Sylvain Lecoy’s picture

I'm jumping directly to the last comment, but this issue relates to PHPUnit fixtures ?

In #1801176: Deploy a PHPUnit system with a bottom-up approach I'm about to convert a UserCreateTest to the new architecture, it has been stated (I don't remember where) that we should have only database setUp by test case (e.g. in setUpBeforeClass()) instead of by method. So I think this is precisely the place to load-up a fixture, which relate in my opinion to a default configuration.

Because I want to take a bottom-up approach for the targeted architecture, I am trying to move out of Drupal vocabulary as much as possible. Legacy code will provide adapter classes to help the transition from SimpleTest to PHPUnit concepts. The legacy code will be created with a top-down approach.

tim.plunkett’s picture

Version:8.x-dev» 9.x-dev
tim.plunkett’s picture

Issue summary:View changes

Updated issue summary.

sun’s picture

Version:9.x-dev» 8.x-dev
Status:Needs work» Needs review
StatusFileSize
new19.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch test.fixtures.112.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Recent work on the child issues of #1595028: [meta] 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:

<?php
    $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:

<?php
    $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. :-)

Status:Needs review» Needs work

The last submitted patch, 112: test.fixtures.112.patch, failed testing.

sun’s picture

Status:Needs work» Needs review

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’s picture

I'm not sure I fully grok the proposal, but I would vastly prefer to solve this with #2096899: Add $EntityType::create() to simplify creating new entities (which could be called from test code, distro setup code, or anywhere else) than a test-specific proposal that requires duplicating (and maintaining that duplicated) logic found elsewhere.

sun’s picture

@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:

<?php
    $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:

<?php
    $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.

Status:Needs review» Needs work

The last submitted patch, 112: test.fixtures.112.patch, failed testing.