Problem/Motivation

I can manually verify a bug by installing Drupal with the Minimal profile and then visiting the modules page, but doing exactly the same thing via Simpletest does not show the error. See http://drupal.org/node/1170362#comment-5794850 for code and screenshots.

Proposed resolution

Remaining tasks

Figure out what Simpletest is doing differently and/or how to mimic the manual steps via Simpletest.

User interface changes

API changes

Original report by pwolanin

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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Assigned: Unassigned » pwolanin
Status: Active » Needs review
FileSize
1.78 KB

Here's essentially the same patch for 7.x as #2 on that issue.

pwolanin’s picture

So, I think this would now allow you to test different profiles using code like:

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() {

  }
}

boombatower’s picture

I think it should be simpler and more documented.

Possible

$this->setInstallProfile('web_test_case_profile');
$this->setUp();

....

and on tearDown the class variable $install_profile could be reset.

pwolanin’s picture

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.

catch’s picture

I like the function name. Useful functionality to have. Not tested yet.

boombatower’s picture

Status: Needs review » Needs work

I like this implementation much better although I think in needs to be reset in tearDown so that the next test will always use the default profile.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.27 KB

patch with bonus test.

I still don't think the addition to tearDown is really needed, but it doesn't hurt.

Dries’s picture

Personally, I think setInstallProfile is 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?

pwolanin’s picture

right, we could also set a class variable rather than the internal static - cleaned up version attached.

cwgordon7’s picture

Why the line:

"+ $this->test_profile = variable_get('web_test_case_profile', 'default');"

?

pwolanin’s picture

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.

pwolanin’s picture

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.

dropcube’s picture

Currently, there are a lot of assertions relying on the HTML response. For example, drupalLogout checks 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 uses drupalLogout will fail due to the absence of the name field in which drupalLogout bases 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

pwolanin’s picture

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

Damien Tournoud’s picture

Title: Allow the profile used for tests to be set via a variable » Allow the profile used for tests to be set via a variable

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

pwolanin’s picture

@Damien Tournoud - one motivation is to allow you to test profiles - but this patch enables a number of possibilities

dropcube’s picture

FileSize
2.52 KB

The .test file attached uses a more generic approach by implementing a class InstallProfileTestCase which other profile tests may extend. This class contains common functions, for example, _testInstalledModules(). Also a test for the default profile has been added.

dropcube’s picture

Here is the patch based on pwolanin's #12 with the approach described at #17

pwolanin’s picture

Status: Needs review » Needs work

If InstallProfileTestCase is supposed to be a generic base class to be extended, it should not have any tests in it.

dropcube’s picture

Here is the updated patch. The base class does not contain test functions now, but assertions.

dropcube’s picture

Status: Needs work » Needs review

Install profiles Tests: 25 passes, 0 fails, 0 exceptions

pwolanin’s picture

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.

dropcube’s picture

FileSize
4.55 KB

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.

dropcube’s picture

Here is an updated patch against HEAD.

pwolanin’s picture

This looks pretty good - made a few tweaks. For example, not every test needs to check that the invalid profile name is rejected.

moshe weitzman’s picture

subscribe. a worthwhile patch. not time to test now though.

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

I'd still really like to see this functionality added. I'm trying to write tests for Project Issue module, and it'd be much, much easier if I could re-image durpalorg_testing profile on each testX function, rather than default.

SimpleTest module improvements are still allowed as long as they don't break APIs.

dww’s picture

webchick: +1 to that. ;) We're working on Project* tests for d.o unit testing (d.o upgrades, redesign deployment, etc). We need the same thing. Maybe one of us will get this working today.

hunmonk’s picture

Status: Needs work » Needs review
FileSize
4.73 KB

updated patch. same functionality as the last patch, but a lot has changed in the code since then -- hopefully i got things right. let's see what the test bot says...

hunmonk’s picture

bah, forgot to include the install.test file. reroll with that added.

cwgordon7’s picture

This looks fundamentally good. There's some minor coding style stuff (trailing whitespace, simpletest stuff, etc.) that I'd like to fix up later tonight, but other than that this should be just about rtbc.

boombatower’s picture

Assigned: pwolanin » boombatower
Status: Needs review » Reviewed & tested by the community
FileSize
10.46 KB

Tests for includes are placed in simpletest/tests since all test must be in a module. I moved the install.test test cases to the profiles they relate to and put the base test case in standard.test. This was a lot of fun since I needed to get the registry to parse all install profiles instead of just the active one.

I do not think we want to revert to the standard profile without at least adding a failed assertion or some such, although I am not sure if we want to check it directly or not. With all the other class variables we stay away form get'ers and set'ers so probably keeep with that pattern when working with install profiles.

Since the tests will be closely related to the install profile used a general suite wide variable seems like a bad idea. Tests either want a custom install profile or they don't.

Missing public static on getInfo() and scope modifiers (protected) on other methods. We also no longer put the Implements docblock on setUp() and getInfo(). Few other minor things like t()'s and what not (try to move away form them).

- Test case names should match profile namespace.
- Number of documentation issues.
- Tests don't run since functions are called that do not exists such as hook_profile_modules().
- A number of areas seem to be using outdated API style.
- A bunch of notices in standard.test.

It would appear this was never tested since you cannot run test tests when placed in includes/ and even when placed somewhere will they will run they blow up. After about 3 hours I got everything working, but it was far from done.

boombatower’s picture

Status: Reviewed & tested by the community » Needs review

Actually so many changes this could use another review.

boombatower’s picture

The way I wrote the patch it will allow the registry to find profiles in places outside of ./profiles which to me seems like something the system should do. So if I get agreement I can file a followup patch that finds the other places in core that hard-code a reference to it in ./profiles.

boombatower’s picture

Laptop isn't configured for spaces properly, fixed patch to remove tabs.

boombatower’s picture

This should fix the number of tests run.

dww’s picture

Status: Needs review » Needs work

This patch is looking very close. I only saw two minor problems:

A)

+class MinimalProfileTestCase extends InstallProfileTestCase {
...
+  protected function testExpertProfile() {

B)

+class StandardProfileTestCase extends InstallProfileTestCase {
...
+  function testDefaultProfile() {

The test function names don't match the classes. Trivial re-roll, 1 sec.

dww’s picture

Status: Needs work » Needs review
FileSize
1000 bytes
9.75 KB

Like so. I just edited the patch file directly in this case. ;)

boombatower’s picture

Yep, looks good. I changed the class names to match, but forgot the test methods. Do we need another review or should we RTBC this?

dmitrig01’s picture

Status: Needs review » Reviewed & tested by the community

Read the patch, tried it. Looks good.

This has the side affect that if you're really evil you can enable two install profiles. Muahahaha. But this is awesome. Something that should be exposed via UI in Drupal 8...

boombatower’s picture

UI? The tests are designed to work with one profile, not multiple.

catch’s picture

It would be useful for some modules, for example entitycache, to be able to run all core tests as if they were part of the default profile, since the objective there is to not break core, and core (still) doesn't reliably use it's own API functions for updating entities in all cases. However that's a different issue to this patch.

jhedstrom’s picture

Subscribing. @boombatower, have you already started a back-port of this feature to 6.x? If not, I can probably throw one together.

boombatower’s picture

Not at the moment...life has been ultra crazy lately.

catch’s picture

#39: 279515-39.variable-profile.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 279515-39.variable-profile.patch, failed testing.

rfay’s picture

subscribe

Anonymous’s picture

me too.

not sure about variable_set/variable_get here, looking into it.

Anonymous’s picture

being able to set the install profile at the class level is too coarse a scope for this issue: #323477: Increase simpletest speed by running on a simplified profile.

for that, we need to be able to set the profile per test, so we'd need something like:

  protected $testProfiles = array(
    'testMethodName' => 'profile',
  );

with a default, so only tests that don't want to use the default need define it.

sun’s picture

Title: Allow the profile used for tests to be set via a variable » Allow to test installation profiles

Better title. #323477: Increase simpletest speed by running on a simplified profile already focuses on allowing a certain install profile for running tests. This issue seems to be about testing install profile expectations itself.

sun’s picture

Version: 7.x-dev » 8.x-dev
sun’s picture

Assigned: boombatower » Unassigned
Issue tags: +Testing system
pillarsdotnet’s picture

sun’s picture

Status: Needs work » Postponed (maintainer needs more info)

I've reread this entire issue once more and looked at the most recent but dated patches. I do not see any new or changed functionality that would not be possible already.

#1373142: Use the Testing profile. Speed up testbot by 50% changed the default profile for running tests to the Testing profile. Individual test cases can still override that decision to use the Standard, Minimal, or whatever else profile being available for running tests by assigning protected $profile = 'standard';

Additionally, #911354: Tests in profiles/[name]/modules cannot be run and cannot use a different profile for running tests fixed the discovery of test cases provided by the installation profile or modules in an installation profile when the installation profile of the test is different to the installation profile of the site that executes the test (i.e., the parent site).

Thus, I don't understand what this issue is about. If I'm mistaken, then please do not clarify in a comment, but revise/revamp the issue summary instead, as it doesn't hold any information at all currently.

pillarsdotnet’s picture

Status: Postponed (maintainer needs more info) » Active

Updated summary as requested.

sun’s picture

Status: Active » Closed (duplicate)

There's no relationship between that issue and this issue, other than that you're dealing with a strange, installation profile related testing system oddity and this issue happens to have that term in its title.

#1215104: Use the non-interactive installer in WebTestBase::setUp() is way more related, as you seem to experience a difference in installation profile behavior between manual testing and automated testing.

Looked once more at the patch of this issue today, and confirming once again, all of the changes exist in the current codebase already - either literally or through similar means. So let's close this issue to prevent further confusion.

pwolanin’s picture

Status: Closed (duplicate) » Active

I don't see how the linked issue duplicates this? Certainly for 7 I cannot yet set an install profile to use instead of standard?

sun’s picture

Version: 8.x-dev » 7.x-dev
Status: Active » Postponed (maintainer needs more info)

D7 also supports protected $profile = 'standard'; already.

Can you clarify what is not working?

pfrenssen’s picture

Sure this is working in D7, for an example, see SimpleTestOtherInstallationProfileModuleTestsTestCase().

joachim’s picture

Status: Postponed (maintainer needs more info) » Active

> D7 also supports protected $profile = 'standard'; already.

It does.

But that does not cause the install profile to be used. It causes the modules listed in that install profile's .info file to be loaded.

The install code in the profile is not run. See #2038823: DrupalWebTestCase::setUp() does not run hook_install_tasks() for the selected profile.

pfrenssen’s picture

Are these issues duplicating eachother? If so, can you mark one of them as a duplicate?

joachim’s picture

Title: Allow to test installation profiles » setting an installation profile for a test doesn't run hook_install_tasks()
Category: feature » bug

Closed the other one as a duplicate, and changing this to a bug, making title more specific.

joachim’s picture

Issue summary: View changes

Updated summary per sun's request in #55.