Allow tests to require and test existance of contrib modules [#343502]

As I proposed in http://groups.drupal.org/node/17325:

I am writing a test for one of my contrib modules that has a feature that is enabled when another contrib module (we'll call it OtherModule) is enabled. I want other people other than myself to be able to run the entire test suite, but what if they didn't have OtherModule installed or even present in their Drupal installation? The test is going to fail spectacularly in that case. So I ended up wrapping that test case class in an if statement:

if (($file = db_result(db_query("SELECT filename FROM {system} WHERE type = 'module' AND name = 'othermodule'"))) && file_exists($file)) {
  class MyModuleOtherModuleIntegrationTestCase extends DrupalWebTestCase {
     ...
  }
}

I re-used the idea from module_load_include and drupal_get_filename to check if the current module is present in the {system} table and that the file also exists. Keep in mind the check for file_exists is necessary since Drupal doesn't remove records from {system} of deleted files/modules.

Going through this got me thinking...what if I could just specify that this test requires a module using getInfo()? This will admittedly not be very useful for any of the core tests, since we are assured they will always exist. I think this might be useful for contrib module tests that need to test integration with another (optional) contrib module. The implementation in getInfo() would look like the following:

class MyModuleOtherModuleIntegrationTestCase extends DrupalWebTestCase {
  function getInfo() {
    return array(
      'name' => t('OtherModule integration'),
      'description' => t('Test MyModule integration with OtherModule.'),
      'group' => t('MyModule'),
      'requires' => array('othermodule'),
  }
  ...
}

Comment	File	Size	Author
#34	drupal.testing-dependencies.patch	3.21 KB	sun
#29	343502-simpletest-required-modules-D7.patch	1.97 KB	dave reid
#23	343502-simpletest-required-modules-D7.patch	2.2 KB	dave reid
#13	343502-simpletest-required-modules-D7.patch	2.3 KB	dave reid
#11	343502-simpletest-required-modules-D7.patch	1.21 KB	dave reid
#8	343502-simpletest-required-modules-D7.patch	1.21 KB	dave reid
#8	343502-example.test.txt	1 KB	dave reid
#8	343502-example.test	1 KB	dave reid
#3	343502.test.txt	1017 bytes	dave reid
#1	343502-simpletest-required-modules-D7.patch	1.28 KB	dave reid
#1	343502.test	1017 bytes	dave reid

Comments

Comment #1

dave reid

he/him

English

Nebraska USA

commented 6 December 2008 at 03:08

Status:

Active

» Needs review

Status	File	Size
new	343502.test	1017 bytes
new	343502-simpletest-required-modules-D7.patch	1.28 KB

Here's the patch to simpletest.module along with a 'dummy' .test file to place in modules/simpletest/tests so you can see how a test that requires another [missing] contrib module will fail with the patch, and how it will not show/run the test with the patch.

I must reiterate that this will not be useful for core tests, but we need to start thinking about contrib tests and sub-module contrib tests.

Comment #2

dmitrig01 commented 14 December 2008 at 16:20

Can you post the test as a .txt file? it's not letting me access it as a .test.

Comment #3

dave reid

he/him

English

Nebraska USA

commented 14 December 2008 at 18:03

Status	File	Size
new	343502.test.txt	1017 bytes

Oh sorry, that's weird. Here you go dmitrig01.

Comment #4

dave reid

he/him

English

Nebraska USA

commented 6 January 2009 at 21:39

Comment #5

dries commented 7 January 2009 at 12:25

This looks like a valid use-case to me, and system tests (tests between modules) is something we want to enable and encourage.

Should we use the same terminology used in .info files? That is 'dependencies' rather than 'requires'?

Comment #6

dries commented 7 January 2009 at 12:25

Issue tags:

+Favorite-of-Dries

This looks like a valid use-case to me, and system tests (tests between modules) is something we want to enable and encourage.

Should we use the same terminology used in .info files? That is 'dependencies' rather than 'requires'?

Comment #7

dave reid

he/him

English

Nebraska USA

commented 7 January 2009 at 18:35

Yeah, that would probably be best, I'll re-roll shortly.

Comment #8

dave reid

he/him

English

Nebraska USA

commented 11 January 2009 at 19:31

Status	File	Size
new	343502-example.test	1 KB
new	343502-example.test.txt	1 KB
new	343502-simpletest-required-modules-D7.patch	1.21 KB

Changed 'requires' to 'dependencies'.

Comment #9

dries commented 12 January 2009 at 08:32

#320451: Clean up some modules code is related -- might be worth a look. I'd prefer to get #320451 in first.

Comment #10

29 January 2009 at 05:15

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #11

dave reid

he/him

English

Nebraska USA

commented 29 January 2009 at 14:21

Status:

Needs work

» Needs review

Status	File	Size
new	343502-simpletest-required-modules-D7.patch	1.21 KB

Not sure why the 'File naming' test has a fail, it's completely unrelated and no test code is actually touched by this patch. *shrugs* The same test passes on my local install.

Anyway, I re-rolled to fix a 1 line offset and to give the testing bot a chance to redeem itself.

Comment #12

alexanderpas commented 12 February 2009 at 03:41

can we have a test for this testclause???

something like dependent on non-existing-module and then $this->assertTrue(FALSE, 'dependent code failure');

otherwise, nice work

Comment #13

dave reid

he/him

English

Nebraska USA

commented 12 February 2009 at 18:36

Status	File	Size
new	343502-simpletest-required-modules-D7.patch	2.3 KB

Sure thing! Easy enough to do. :)

Comment #14

sun

German

Karlsruhe

commented 13 February 2009 at 01:42

subscribing

Comment #15

babbage commented 13 February 2009 at 04:14

Great idea, very useful. I have two suggestions, one which I'll post now, the other one once I've tested what I'm saying to be sure it is right!

Suggestion 1: Rather than just excluding the test, I think it would be useful to have some sort of feedback to the user that optional tests have not run due to XX optional module not being installed. Otherwise, the person running the tests may be unaware they have broken a piece of functionality that cannot be tested with the currently suitable tests. Does not look like the current patch provides for this...

Comment #16

dave reid

he/him

English

Nebraska USA

commented 13 February 2009 at 04:16

I think the main idea behind this was to allow for integration tests that you only want to run if the module is installed, in which case if it's not, should not be displayed or even run. I'll look into that, but I fear it will make this simple path a little less simple.

Comment #17

babbage commented 13 February 2009 at 05:07

OK.
I believe the query needs to be:

<?php
($file = db_result(db_query("SELECT filename FROM {system} WHERE type = 'module' AND name = 'othermodule' AND status ='1'")))
?>

Because currently, the query will return modules that are installed, whether or not they are enabled.

My other suggestion above does complicate the patch, and perhaps it would be better as a separate issue. I like small patches.

Comment #18

babbage commented 13 February 2009 at 05:05

Oh, and just noticed you will also need to change the namespace in the test, because this conflicts with my new Drupal stealth module I am about to upload to CVS, which is already using the name module_that_does_not_exist.

;)

Comment #19

dave reid

he/him

English

Nebraska USA

commented 13 February 2009 at 05:56

dbabbage, We don't need to check status = 1 because the module doesn't need to be enabled in the system that runs the tests. We just need to check that the module actually exists because the test itself needs to run parent::setUp('othermodule') (inside it's own setUp()) to install and enable it. For example,

class OtherModuleIntegrationUnitTest extends DrupalWebTestCase {
  function getInfo() {
    return array(
      'title' => t('othermodule test'),
      'description' => t('Tests integration with other contrib modules.'),
      'group' => t('othermodule'),
      'dependencies' => array('required_contrib_module', 'another_required_contrib_module'),
    );
  }

  function setUp() {
    parent::setUp('required_contrib_module', 'another_required_contrib_module');
  }

  function testSomething() {
    ...
  }
}

Comment #20

babbage commented 13 February 2009 at 06:38

Ah ok, I am showing my ignorance regarding writing tests then. Move on, nothing to see here. :)

Comment #21

boombatower commented 16 February 2009 at 07:27

So then use module_exists() ?

Comment #22

dave reid

he/him

English

Nebraska USA

commented 16 February 2009 at 07:46

Status:

Needs review

» Needs work

Can't use module_exists because of the scenario:

A test requires module_a that you have available in your Drupal install, but not installed or enabled. You still should be able to run the test that requires module_a.
But using module_exists will fail during the SimpleTest selection form since the module is not enabled.

I could probably help reduce the code using drupal_get_filename() instead of manually checking the system table.

Comment #23

dave reid

he/him

English

Nebraska USA

commented 16 February 2009 at 07:52

Status:

Needs work

» Needs review

Status	File	Size
new	343502-simpletest-required-modules-D7.patch	2.2 KB

Revised patch using drupal_get_filename().

Comment #24

16 February 2009 at 08:20

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #25

dave reid

he/him

English

Nebraska USA

commented 16 February 2009 at 08:26

Status:

Needs work

» Needs review

Testbot failure on #4 (user picture tests). Requesting retest.

Comment #26

13 March 2009 at 08:40

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #27

dave reid

he/him

English

Nebraska USA

commented 14 March 2009 at 03:16

Status:

Needs work

» Needs review

Errant GD testbot error...resetting status.

Comment #28

deviantintegral commented 16 May 2009 at 20:49

Status:

Needs review

» Needs work

This no longer applies as instantiating each class was removed in #376129: Change getInfo() to a static method for performance. Do we want to re-implement loading the classes so we can call getInfo(), or find some other way of describing test dependancies?

Comment #29

dave reid

he/him

English

Nebraska USA

commented 26 May 2009 at 01:38

Status:

Needs work

» Needs review

Status	File	Size
new	343502-simpletest-required-modules-D7.patch	1.97 KB

Rerolled using the new way $info = call_user_func(array($class, 'getInfo'));.

Comment #30

mr.baileys

Dutch

🇧🇪 (Ghent)

commented 26 May 2009 at 10:10

Status:

Needs review

» Needs work

Some comments after reviewing the patch, applying it and playing around with it:

+ 'title' => t('Testing dependent module test'),
should be 'name' instead of 'title'
I like dbabbage suggestion of having visual feedback when tests are skipped, but I understand this might complicate this patch. Would it be easier to tinker with simpletest_get_all_tests and have it return an additional field called "disabled" or something, and then grey out the checkbox on the SimpleTest submit form (so users are aware of the test and see that it is disabled/skipped)? Or would that be equally complex? Personally, I just hate the fact that there is no trace of the test when the dependencies are missing...

I tried all kinds of dependency combinations and it consistently worked as expected, and the logic looks sound to me...

Comment #31

boombatower commented 26 May 2009 at 15:06

If you want to mess with simpletest_get_all_tests() please wait for #449198: SimpleTest: Clean up test loading and related API. From the looks of it that might be a good idea.