Allow the profile used for tests to be set via a variable
pwolanin - July 7, 2008 - 18:18
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | simpletest.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | pwolanin |
| Status: | patch (code needs review) |
Description
Per http://drupal.org/node/279455
Right now drupal_web_test_case always uses the default profile when installing. This is problematic for a couple reasons - for example, I can't test an alternate profile to make sure it runs correctly.
#1
Here's essentially the same patch for 7.x as #2 on that issue.
#2
So, I think this would now allow you to test different profiles using code like:
<?php
class BasicInstallProfileTestCase extends DrupalWebTestCase {
protected $original_profile;
/**
* Implementation of getInfo().
*/
function getInfo() {
return array(
'name' => t('Basic Profile functionality'),
'description' => t('Test profiles'),
'group' => t('System')
);
}
/**
* Implementation of setUp().
*/
function setUp() {
$this->original_profile = variable_get('web_test_case_profile', NULL);
variable_set('web_test_case_profile', 'basic');
parent::setUp();
}
function tearDown() {
parent::tearDown();
if (!empty($this->original_profile)) {
variable_set('web_test_case_profile', $this->original_profile);
}
}
/**
* Test that profile features are enabled.
*/
function testBasicProfileFeatures() {
}
}
class BlogInstallProfileTestCase extends DrupalWebTestCase {
protected $original_profile;
/**
* Implementation of getInfo().
*/
function getInfo() {
return array(
'name' => t('Blog Profile functionality'),
'description' => t('Test profiles'),
'group' => t('System')
);
}
/**
* Implementation of setUp().
*/
function setUp() {
$this->original_profile = variable_get('web_test_case_profile', NULL);
variable_set('web_test_case_profile', 'blog');
parent::setUp();
}
function tearDown() {
parent::tearDown();
if (!empty($this->original_profile)) {
variable_set('web_test_case_profile', $this->original_profile);
}
}
/**
* Test that profile features are enabled.
*/
function testBlogProfileFeatures() {
}
}
?>
#3
I think it should be simpler and more documented.
Possible
<?php
$this->setInstallProfile('web_test_case_profile');
$this->setUp();
....
?>
and on tearDown the class variable
$install_profilecould be reset.#4
how about the attached?
One doesn't even need to reset the variable, I think, since this will be fresh for each class that's instantiated.
#5
I like the function name. Useful functionality to have. Not tested yet.
#6
I like this implementation much better although I think in needs to be reset in
tearDownso that the next test will always use the default profile.#7
patch with bonus test.
I still don't think the addition to tearDown is really needed, but it doesn't hurt.
#8
Personally, I think
setInstallProfileis behaving a little funny.1. The return value can be 3 different things. Because this is a class, we don't necessarily have to do the funny return value thing for
setInstallProfile.2. To reset the install profile, why don't we set it to 'default' using
$this->setInstallProfile('default')instead of$this->setInstallProfile(NULL, TRUE)? It seems like reset is not needed?#9
right, we could also set a class variable rather than the internal static - cleaned up version attached.
#10
Why the line:
"+ $this->test_profile = variable_get('web_test_case_profile', 'default');"
?
#11
Because those building a new or complicated profile might want to run ALL tests with that profile as the install and/or a packaged distribution of Drupal might not include the default profile.
#12
from IRC conversion, I misunderstood the cwgordon's concern - the code was unclear (and only worked by luck) - it's clearer and more reliable to move the determination of the profile to prior to where the DB prefix is altered.
#13
Currently, there are a lot of assertions relying on the HTML response. For example,
drupalLogoutchecks if the user name field is found when requesting the /user page. If we are going to test alternate profiles that may install multiple modules we should have in mind that such modules may alter the output in any way. For example, consider a module that alters or replace the user login form, then every test that usesdrupalLogoutwill fail due to the absence of the name field in whichdrupalLogoutbases it's assertions.So, here I see again the real need of having data available to make assertions, and not basing assertions 100% on the HTML response. See #266220: Exposing metadata to tests
#14
@dropcube - yes, that's certainly true - but I'm not sure there is any ready way around that. This patch just enables the use of other profiles - so if you do, obviously you'll have to take that into consideration when interpreting test results.
#15
@dropcube: I already expressed my concerns about #266220: Exposing metadata to tests. If I understand pwolanin approach here, we want to test profiles themselves (ie. have a default.test and a expert.test), not use existing tests on different profile configurations.
Again: we are testing Drupal in its standard configuration. We cannot even imagine testing every possible configuration out there.
#16
@Damien Tournoud - one motivation is to allow you to test profiles - but this patch enables a number of possibilities
#17
The .test file attached uses a more generic approach by implementing a class
InstallProfileTestCasewhich other profile tests may extend. This class contains common functions, for example,_testInstalledModules(). Also a test for the default profile has been added.#18
Here is the patch based on pwolanin's #12 with the approach described at #17
#19
If InstallProfileTestCase is supposed to be a generic base class to be extended, it should not have any tests in it.
#20
Here is the updated patch. The base class does not contain test functions now, but assertions.
#21
Install profiles Tests: 25 passes, 0 fails, 0 exceptions
#22
since drupal_verify_profile() does some extra checking and might eliminate some modules from the list in the profile, I'm not sure we want to use it for the test. I'd suggest reproducing the first part of the code from that function.
#23
I am attaching an updated version of the file install.test for review and not a patch because seems to be a conflict in drupal_web_test_case.php due to #231190: Page Cache doesn't work with HEAD requests
In the test now are implemented #22 pwolanin's suggestions and added other tests for the default profile. Will submit a patch later.
#24
Here is an updated patch against HEAD.
#25
This looks pretty good - made a few tweaks. For example, not every test needs to check that the invalid profile name is rejected.
#26
subscribe. a worthwhile patch. not time to test now though.