Allow tests to require and test existance of contrib modules
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | simpletest.module |
| Category: | task |
| Priority: | critical |
| Assigned: | Dave Reid |
| Status: | closed |
| Issue tags: | API clean-up, Favorite-of-Dries |
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:
<?php
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:
<?php
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'),
}
...
}
?>
#1
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.
#2
Can you post the test as a .txt file? it's not letting me access it as a .test.
#3
Oh sorry, that's weird. Here you go dmitrig01.
#4
#5
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'?
#6
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'?
#7
Yeah, that would probably be best, I'll re-roll shortly.
#8
Changed 'requires' to 'dependencies'.
#9
#320451: Clean up some modules code is related -- might be worth a look. I'd prefer to get #320451 in first.
#10
The last submitted patch failed testing.
#11
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.
#12
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
#13
Sure thing! Easy enough to do. :)
#14
subscribing
#15
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...
#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.
#17
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.
#18
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.
;)
#19
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,
<?php
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() {
...
}
}
?>
#20
Ah ok, I am showing my ignorance regarding writing tests then. Move on, nothing to see here. :)
#21
So then use module_exists() ?
#22
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.
#23
Revised patch using drupal_get_filename().
#24
The last submitted patch failed testing.
#25
Testbot failure on #4 (user picture tests). Requesting retest.
#26
The last submitted patch failed testing.
#27
Errant GD testbot error...resetting status.
#28
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?
#29
Rerolled using the new way
$info = call_user_func(array($class, 'getInfo'));.#30
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 tried all kinds of dependency combinations and it consistently worked as expected, and the logic looks sound to me...
#31
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.
#32
Will reroll shortly.
#33
This is required to make any sense for unit tests in D7 contrib.
#34
Crazy re-ro-re-roll.
#35
Looks good and tested. Crazy that I did not know about continue statement accepting a the number of levels to skip! Learn something new every day.
#36
Committed to CVS HEAD. Thanks!
#37
Automatically closed -- issue fixed for 2 weeks with no activity.