Goal

  • Remove this compatibility layer:
    <?php
       
    // @todo Remove this compatibility layer in Drupal 8, and only accept
        // $modules as a single array argument.
       
    $modules = func_get_args();
        if (isset(
    $modules[0]) && is_array($modules[0])) {
         
    $modules = $modules[0];
        }
    ?>

Proposed solution

  • Use a static $modules array class property to define modules to install.
  • Collect all $modules values of all intermediate classes up to DrupalWebTestCase.
  • Benefits:
    1. Eliminates most setUp() method overrides.
    2. Allows to check module dependencies of a test case without instantiating the class.

Possible solutions

  1. Do not change the signature, but require to always pass an array.
    <?php
    function setUp() {
     
    $modules = func_get_args();
     
    $modules = $modules[0];
    }
    ?>
  2. Change the signature to enforce an array.
    <?php
    function setUp(array $modules = array()) {
    }
    ?>
  3. Ditch the argument approach, and use a class property instead. Benefit: Eliminates most setUp() method overrides.
    <?php
    class DrupalWebTestCase {
      function
    setUp() {
       
    // Collect modules to install.
       
    $modules = array();
       
    $class = $this;
        while (
    $class) {
          if (
    property_exists($class, 'modules')) {
           
    $modules = array_merge($modules, $class::$modules);
          }
         
    $class = get_parent_class($class);
        }
      }
    }

    class
    FooWebTestCase extends DrupalWebTestCase {
      public static
    $modules = array('foo');
    }
    class
    BarWebTestCase extends FooWebTestCase {
      public static
    $modules = array('bar');
    }
    ?>
Files: 
CommentFileSizeAuthor
#24 drupal8.test-setup-modules.24.patch2.26 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,295 pass(es).
[ View ]
#9 drupal8.test-setup-modules.9.patch2.06 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.test-setup-modules.9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 drupal8.setup-modules-property.2.patch1.91 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,366 pass(es).
[ View ]
#1 drupal8.setup-modules-property.1.patch3.55 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,213 pass(es).
[ View ]

Comments

sun’s picture

Issue summary:View changes

Updated issue summary.

sun’s picture

Status:Active» Needs review
StatusFileSize
new3.55 KB
PASSED: [[SimpleTest]]: [MySQL] 34,213 pass(es).
[ View ]

Proposal 3) in code. I like.

sun’s picture

Issue summary:View changes

Updated issue summary.

sun’s picture

StatusFileSize
new1.91 KB
PASSED: [[SimpleTest]]: [MySQL] 34,366 pass(es).
[ View ]

Without the debugging madness. ;)

That's my favorite, really. Eliminates the entire argument and manual processing.

Especially beneficial for cross-project tests in contrib.

Tests can be mixed and merged and sub-classed in any way, without having to care for proper processing of a $modules argument in any way.

sun’s picture

Assigned:Unassigned» sun
Dave Reid’s picture

Using a static property makes it hard to extend a base class and not have to manually specific the base class's modules. This is especially useful in contrib, not core.

sun’s picture

@Dave Reid: The $modules of any parent class in between are explicitly taken into account. That's why I really like this.

sun’s picture

Issue summary:View changes

Updated issue summary.

sun’s picture

Additional benefit of the static $modules property:

Allows to check module dependencies of a test case without instantiating the class / invoking setUp().

Updated the issue summary accordingly.

sun’s picture

Title:Require modules array for test setUp() method» Change how $modules list to install is passed to DrupalWebTestCase::setUp() method
Crell’s picture

Status:Needs review» Needs work
+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -1387,6 +1387,14 @@ class DrupalWebTestCase extends DrupalTestCase {
+    $class = $this;
+    while ($class) {
+      if (property_exists($class, 'modules')) {
+        $modules = array_merge($modules, $class::$modules);
+      }
+      $class = get_parent_class($class);
+    }

If what we actually want here is a class name, from $class, then we should probably do $class = get_class($this); instead, to be more precise.

Static anything tends to make me queasy, but in this case we're already using a static getInfo() method as a declaration mechanism so it's no worse than that, and the advantages sun lists are valid. For completeness I would rather a static getModules() method to a public property, but the concept I'm OK with.

Failing that, option 2 from the OP would be my next choice. (You want an array, pass an array, fatal if not.)

-27 days to next Drupal core point release.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new2.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.test-setup-modules.9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

1) Fixed the get_class($this).

2) Not sure/convinced about $modules vs. getModules() yet. I'd prefer the simplicity of $modules, TBH, and don't see any serious problems with it being directly accessible (sounds rather like a hidden feature to me).

3) Re-rolled against HEAD. Sadly, FieldUITestCase is no longer a prime example for multiple class inheritance and proper accounting for additional $modules defined in classes in between, but I hope that the fact that previous patches passed are sufficient to clarify that this actually works. ;)

Crell’s picture

I don't get the hidden-feature comment. It's good OO principle to default to an accessor unless there's a solid reason not to; in this case, I don't see a compelling argument to make it a public property rather than a method.

A method would also allow us to skip the if(property_exists()) check, and just call getModules() in every case. The base class implementation returns an empty array, so nothing happens. Child classes can then override it to return something as needed. Also, that means the method can be defined and documented as a method, and included in an interface, whereas a member variable cannot.

tstoeckler’s picture

If I'm not mistaken, because we traverse the class tree like that, calling $modules += parent::getModules() from your getModules() implementation would add the modules twice. Now, I imagine that wouldn't be *terrible*, but with a simple property, you really can't do anything wrong. Of course we could document not to call the parent implementation manually, but, again, what's the point?

Crell’s picture

Eh? How would it add elements twice?

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -1401,12 +1401,19 @@ class DrupalWebTestCase extends DrupalTestCase {
+    $class = get_class($this);
+    while ($class) {
+      if (property_exists($class, 'modules')) {
+        $modules = array_merge($modules, $class::$modules);
+      }
+      $class = get_parent_class($class);
+    }

This turns into...

$class = get_class($this);
while ($class) {
  $modules = array_merge($modules, $class::getModules());
  $class = get_parent_class($class);
}

The only reason that would return duplicates is if you call parent::getModules() in your getModules() method, which is by no means a requirement. We'd just document "well don't do that."

Or, now that you mention it, that would be even better. Replace that entire code block with:

$modules = static::getModules();

(static being the late-static-binding version of self, introduced in PHP 5.3, that makes static methods work intelligently when doing inheritance.)

And the if you specify your own getModules() method, you DO call parent::getModules() yourself:

public static function getModules() {
  return array_merge(parent::getModules(), array('views'));
}

OO working as it should. :-) (I haven't tried running the above code, but give or take a syntax error it should all work, and work the way the language wants to work.)

tstoeckler’s picture

Yeah, that's kind of what I said...
Anyhow, I guess we have to decide if not having to call parent::getModules() is worth the traversal in DrupalWebTestCase or not. I personally would say yes, but either way is better than now, I guess.

sun’s picture

Alright, bigger picture:

  1. I do not see a point or use-case for allowing tests to skip the defined modules of a parent test class it inherits from. Thus, I don't see why every single test case should have to perform an array_merge() and whatnot manually. Freeing test authors from the entire setUp() and very similar $modules = func_get_args() drama, which requires to copy the same code all over again, and making sure it is correct, is one of the key reasons for the proposal 3).
  2. I wouldn't want to see test cases that define the module list dynamically. I'm aware that test cases would still be able to do so in the setUp() method, and it's probably OK to leave the option for extreme edge-cases, but it should be discouraged.
  3. LSB doesn't help here, because its resolution stops at the first fully resolved static call with no fallback. Contrary to that, we want to resolve all static calls (that exist) in the full class hierarchy.
  4. If it absolutely have to be a method (though I still don't see why), then we'd have to find a proper method name. getModules() sounds wonky to me, because the definition actually sets modules. But setModules() would be wonky too, because the method doesn't actually set a property or similar. Thus, it would have to be installModules() or similar... (which is again not correct, 'cos the method does not actually perform the installation of modules...) And when comparing all that with a simple $modules property, then I feel lost in a fruitless OO purity discussion. (KISS)
  5. $modules could be renamed to $dependencies, but unfortunately, that would clash with getInfo()'s 'dependencies' key - which is used to determine whether all dependent modules required by a test (but not necessarily directly installed) exist, before trying to execute the test (and AFAIK, is also used by the d.o testing infrastructure to provide the proper projects on testbot checkouts when running tests). Also, the specified $modules are not necessarily all required dependencies, so renaming $modules to $dependencies could give the wrong impression.
  6. Speaking of getInfo(), I'd like to get rid of it and replace most of it with phpDoc parsed via ReflectionMethod instead; see #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)
  7. Next to the proposed $modules, there is also the $profile property, the installation profile to use for running tests. $profile is not static but protected property, so the last override in the hierarchy takes effect. Unlike $modules, $profile has a default value of 'testing'.
  8. Every test case will define $modules in D8.
  9. Taking the proposal in #913086: Allow modules to provide default configuration for running tests into account, the default testing configuration being installed for a test will be based on $modules, too. There will be another test case specific property that allows the test to omit/skip the testing configuration for specified modules. There's no proposal for that property name over there yet, but let's simply consider $skipConfigurations for now.

So in an ideal world, with all aspects combined, a test case that specifies/overrides everything would look along the lines of:

namespace Drupal\Comment\Tests;

use Drupal\Simpletest;

/**
* Comment threading.
*
* Tests proper incrementing of comment numbers in comment threads.
*/
class ThreadingTest extends Simpletest\WebTest {
  // Testing is the default, but let's pretend it wasn't.
  protected $profile = 'testing';

  // Comment is sufficient, installs full dependency stack of Entity, Text,
  // Field, Node, including their default testing configuration.
  public static $modules = array('comment');

  // ThreadingTest actually uses the default config for Comment module, but
  // let's pretend it wasn't.
  protected $skipConfigurations = array('comment');

  /**
   * Tests comment threading.
   */
  function testThreading() {
    // ...
  }
}

If you take away the inline comments, then we have a very simple test case definition that is easily understood, even by novice test authors.

Crell’s picture

sun, let's back up a moment. Isn't there ongoing discussion of switching out Simpletest in favor of PHPUnit anyway? Shouldn't we resolve that question first before we spend time on heavy architectural changes of our custom testing framework?

If we're investing that much time, I think it would be better spent in migrating to a more standard and capable system than continuing to evolve our own, knowing that there's a good chance we'll replace it.

catch’s picture

Currently Upal extends PHPUnit classes so that DrupalWebTestCase can be used as is - afaik the only thing that's incompatible with current tests at the moment is that PHPUnit doesn't return from assertions, and we have a few core tests that rely on that behaviour. So at least in the first instance, even with a switch to PHPUnit, all of this stuff likely wouldn't change that much.

sun’s picture

Righto - as the name implies, PHPUnit is a framework for unit tests, which doesn't help with or resolve any of the challenges we're facing with our functional/integration tests, especially not on the scale of our modular aspects. I'm very open to look into standardization of our testing system, but that's a separate issue, and regardless of its outcome, we'll have to find a solution for this issue (and the others).

Crell’s picture

catch: Yes, but isn't the intent to then start removing our extra intermediary layers where what's underneath is good enough? Ideally we would be able to just use vanilla PHPUnit directly in time. (At least that's my ideal.)

sun’s picture

Alright, can we go ahead with the patch in #9?

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

I'm going to be bold and mark this RTBC.
If we decide to change this to a getModules() or whatever later on, this patch will actually make that easier (because it separates the modules from the rest of the code). More importantly, this allows to remove the hackish func_get_args() dance from the dozen or so helper test classes in core (see the op).

Since we're only at #20 comments, I think it's OK to handle the conversion to the static $modules property in this issue, but in a follow-up patch.

catch’s picture

catch: Yes, but isn't the intent to then start removing our extra intermediary layers where what's underneath is good enough? Ideally we would be able to just use vanilla PHPUnit directly in time. (At least that's my ideal.)

Well we might get there eventually, but that's got absolutely nothing to do with switching to PHPUnit.

For example to remove DrupalWebTestCase we'd need Drupal to be at the point where you never actually install it - rather you'd simply start with a blank slate (apart from config directory and database connection), then start enabling modules from there. That's a very long way off (optional system and user modules will not be easy), so it's very likely improvements to DrupalWebTestCase will last at least a full release cycle.

sun’s picture

Issue tags:-Testing system

#9: drupal8.test-setup-modules.9.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Testing system

The last submitted patch, drupal8.test-setup-modules.9.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new2.26 KB
PASSED: [[SimpleTest]]: [MySQL] 37,295 pass(es).
[ View ]

Re-rolled against HEAD.

sun’s picture

Title:Change how $modules list to install is passed to DrupalWebTestCase::setUp() method» Replace $modules list for WebTestBase::setUp() with ::$modules class properties
Issue tags:+API clean-up

Clarifying issue title.

Can we move forward here?

Please also note that I'm actively looking into ways for replacing Simpletest with PHPUnit and/or alternatives. This issue here, however, needs to happen independently from that, since swapping out the base testing framework does not imply that we won't have any web tests anymore. (And fwiw, I just recently talked to a Behat developer, who was quite impressed by our WebTestBase infrastructure [not it's code, but what it allows us to do].)

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Today was my 5th time explaining simpletest to those new to core at a sprint, and far and away the biggest WTF was how setUp() was used to enable modules.

I think this is great.

catch’s picture

Title:Replace $modules list for WebTestBase::setUp() with ::$modules class properties» Change notification for: Replace $modules list for WebTestBase::setUp() with ::$modules class properties
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active

Thanks. Committed/pushed to 8.x, this will need a change notification.

tim.plunkett’s picture

In addition to writing the change notice, do we want to repurpose this as a meta for tracking conversions, or should that be a new issue?
I opened #1710300: Convert node tests to use ::$modules property instead of parent::setUp($modules) just to see how easy/hard it was.

grep -nr "parent::setUp([^)]" * | wc -l shows 279 places this will need changing, node module was 19 tests worth. That's about a 250K patch if it's done all at once.

sun’s picture

One or more separate issues make more sense. Given how large the patch in #1710300: Convert node tests to use ::$modules property instead of parent::setUp($modules) is, I guess that any larger patch would have to be constantly re-rolled due to other changes... So perhaps one issue per module would be better? Ideally, we'd bundle all of them in a nice issue tag.

tim.plunkett’s picture

I've added a change record: http://drupal.org/node/1710766

sun’s picture

Title:Change notification for: Replace $modules list for WebTestBase::setUp() with ::$modules class properties» Replace $modules list for WebTestBase::setUp() with ::$modules class properties
Priority:Critical» Normal
Status:Active» Fixed

Thank you!

tim.plunkett’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

Updated issue summary.