When using an install profile that is packaged as a distribution (ie. its projects live in profiles/[name]/modules) tests in any modules therein cannot be run successfully. To reproduce:

  1. Install D7 using the standard install profile
  2. Create a modules directory at profiles/standard/modules
  3. Move one of the core modules (e.g. poll) from modules/poll to profiles/standard/modules/poll and then clear your caches.
  4. Run poll tests and watch #fail

While the scenario above is obviously unrealistic, downloading an install profile from Drupal.org that contains, say, views in profiles/[name]/modules is a very real case.

Problem

The problem is caused by the module whose test is to be run not being available in drupal_system_listing() given that it lives in a profile directory:

  1. Views is in profiles/[name]/modules/views,
  2. When Views' tests are run, the install profile used is standard, leading to the following paths scanned by drupal_system_listing():
    • profiles/standard/modules
    • sites/all/modules
    • sites/[x]/modules
  3. Views is obviously not in any of these directories and so the test fails as it cannot enable Views from the get go.

Approaches

Some possible approaches,

  1. Treat this as a UI problem. Don't show any tests for modules in profiles/[name]/modules. Install profiles and distros would not be able to run tests for any of their modules.
  2. Always have $test->profile use the current install profile if the installed profile is not one of the Drupal core install profiles. Tests will likely take much longer to run, especially when they could just be using the lighter testing profile.
  3. Adjust drupal_system_listing() to use not only drupal_get_profile() but the 'parent' install profile (the install profile of the site that the test is running on).
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alex_b’s picture

This is the culprit:

  /**
   * Preload the registry from the testing site.
   *
   * This method is called by DrupalWebTestCase::setUp(), and preloads the
   * registry from the testing site to cut down on the time it takes to
   * set up a clean environment for the current test run.
   */
  protected function preloadRegistry() {
    $original_connection = Database::getConnection('default', 'simpletest_original_default');
    db_query('INSERT INTO {registry} SELECT * FROM ' . $original_connection->prefixTables('{registry}'));
    db_query('INSERT INTO {registry_file} SELECT * FROM ' . $original_connection->prefixTables('{registry_file}'));
  }

This is plain broken. preloadRegistry() (or the code that calls preloadRegistry()) needs to check whether the search path of the host site and the test site are the same before it decides to copy the registry of the host site to the client site.

alex_b’s picture

i may have shot too fast here. preloadRegistry should actually make sure that the host site's registry is used, thus modules sitting in the host site's profiles directory should always be found...

boombatower’s picture

Assigned: Unassigned » boombatower
Status: Active » Needs review
FileSize
1.41 KB

After a discussion in IRC confirmed that the testing system cannot test modules in a profile since it cannot guarantee the modules tests will enable the profile and thus make the module available. Hacking the modules system could fix this, but does not seem appropriate. As such the only useful test is the profile's test which is assumed to enable itself and can test any of the modules included with the profile. Considering the only modules this will be an issue for are the modules written for the profile (aka not views or other contrib modules) which can (and make sense) be tested by the profile itself.

If you want to run tests for modules included in a profile move (or sym link) them into one of the sites module directories.

The patch ignores all modules in profiles (expect the profile itself) so that the UI and run-tests.sh script do not list module tests they cannot run.

boombatower’s picture

boombatower’s picture

My luck, commit as I roll.

yhahn’s picture

FileSize
2.83 KB

Discussed this issue further with alex_b and we agreed that an approach where hiding tests for modules in profiles/[x] would be a serious blow to Drupal distributions and promoting usage of Drupal as a platform for building products. For example, this would prevent any of the install profiles packaged by Drupal.org from being able to run the tests for their contrib modules. Symlinking from sites/all/modules is not a real solution to me and requires further explaining to users why such a hack is necessary.

The patch below implements option #3 listed in the initial post -- Simpletest sets the host profile to be used by drupal_system_listing().

adrian’s picture

Status: Needs review » Needs work

instead of hardcoding these paths into the drupal_system_listing, I think a simpler
and more elegant approach would be to add a drupal_add_search_path() function,
which will allow you to add the path to a control array.

So we would do :

  drupal_add_search_path('host_profile', "profiles/{$profile}");

Other useful places to configure additional paths include settings.php and sites.php.

adrian’s picture

FileSize
3.42 KB

here's a first roll of the patch.

This only handles the host_profile case for now, but we can add additional places to specify paths.

adrian’s picture

FileSize
3.42 KB

test bot hates me ?

renamed to .patch.

boombatower’s picture

We should not be running tests for the contrib modules in an install profile as those are run on the contrib project already...no need to waste the testbot's time with overly redundant testing. Hacking core to achieve this purely for testing isn't any better either.

The profile can run tests for any of it's own modules...and any glue code with contrib modules, configurations of contrib modules, or whatever, but running the contrib tests verbatim is just bad. Since they won't run with the install profile they are found in (ie they will use standard profile during tests) that is confusing for one and a waste for the testbot and doesn't make a lot of sense. Contrib projects should not be micromanaged in distributions (tests, issues, or anything). Distributions simply package contrib modules, configure them, and provide glue code. If anything we need to display the test results for contrib modules on the distribution project page.

alex_b’s picture

no need to waste the testbot's time with overly redundant testing

I'd like to find a different solution for that other than axing patch #9.

The point of #9 is strengthening distros. To that end we need to find a solution on how to run tests of arbitrary contrib modules shipped with a profile.

BTW, the real conceptual problem here is that module tests depend on profiles. This collides with the fact that profiles have their own search path.

I view the current behavior as a regression. It was possible to run tests of any contrib module residing in a profile in D6.

boombatower’s picture

Including all profiles in the search path seems like including all sites in the search path...how is that what we want? Or allowing one to run tests from siteb in sitea...sure it is possible, but really?

I dunno...this is obviously possible I'm just not sure it makes sense...and having to special case (like any special case) the testbot seems like another pointer that this isn't a clean solution.

So what is/are our usecase(es) here?
- developer has profile installed and wants to run tests on contrib module locally? if he is developing the contrib module (or finds bug) he will need a checkout to make patch or commit? which drush make won't give you so won't they need to checkout the module (which could be done in site folder) in order to work on it? (maybe not? obviously could make raw diffs with some hacking, but...)
- want to see summary of contrib modules for distro (d.o functionality)
- end users should never be running tests

I mean normally people shouldn't be running tests locally when testbot does it already and has optimized machines that do it faster (if anything we should improve it). So what exactly is our usecase here? I don't do distro work, but those in this issue I know have more experience in that area...so what does that look like?

My understanding is the distros have drush make scripts that grab the contrib modules...so unless there is an alternative running tests on those checkouts is fairly useless since development will need to be done against a checkout of dev not a stable dump, right?

adrian’s picture

developer has profile installed and wants to run tests on contrib module locally? if he is developing the contrib module (or finds bug) he will need a checkout to make patch or commit? which drush make won't give you so won't they need to checkout the module

Just add --working-copy to it.

(which could be done in site folder) in order to work on it? (maybe not? obviously could make raw diffs with some hacking, but...)

Every module would need to be symlinked to the sites/all folder, because if you write a test for a custom module that depends on a contrib module, those modules would not get enabled at all.

What happens is that the testing interface shows you a bunch of things to be tested, but then fails when you try to test to run those tests because they are no longer found.

adrian’s picture

Including all profiles in the search path seems like including all sites in the search path...how is that what we want? Or allowing one to run tests from siteb in sitea...sure it is possible, but really?

We are not including all profiles, only the host site's profile is getting included.

For example, if modules are present in sites/sitename.com/modules you will be able to test them because the 'test site' is installed as 'sitename.com' too and therefor sites/sitename.com/modules is in your search path.

If the site directories had the same behavior as profiles have currently, the testing interface would show you the modules in your interface, but when you run them the site will be installed explicitly as 'testing.com' (or whatever the test required) and only sites/testing.com/modules is added to the search path. Hence all the tests fail because the 'test site' cant actually find the modules it needs to test.

adrian’s picture

Status: Needs work » Needs review
FileSize
7.89 KB

here's a new version of this patch. I moved more of the paths to the control array.

I also moved some functions to bootstrap.inc, because they get called during bootstrap.

This would also help #545452: Store install profile in the generated settings.php file , as the drupal_add_system_directory could be called after including the settings.php.

adrian’s picture

Title: Tests in profiles/[name]/modules cannot be run » Make system directories configurable to allow tests in profiles/[name]/modules to be run.

Improve the title to illustrate the approach.

Related issues :
#295434: Support contrib being placed in profiles/all - Allows additional paths to be added by the profile maintainer.
#545452: Store install profile in the generated settings.php file - Simplify profile selection so we can do it earlier in the bootstrap process.
#25720: Add additional system directories to search within settings.php - We could finally do this, very easily, and without globals.

adrian’s picture

FileSize
9.03 KB

Here is a more complete version of the patch that handles priorities a lot more cleanly.

I ironed out the concept of 'scope' so that there are now 3 primary levels of precedence. They are system-wide, profile-wide, site-wide.
site wide takes precedence over profile wide takes precedence over system wide.

Each of these 3 levels also have 'before' or 'after' levels too. For instance, the sites/all search path is actually added "before:site", meaning it has precedence over anything in the profiles, but not in sites/$sitename.

This was necessary so that you can target what priority the additional search paths should have. A good example here is related to #295434: Support contrib being placed in profiles/all
To add an additional profiles/all search path, the profile developer simply needs to add the following to his .profile file :

  function standard_init() {
    drupal_add_system_directory('before:profile', 'profiles/all');
  }

The other reason I did this, is because we no longer need to have a use case specific override for the host site's profile.
We can accurately determine where we are adding the path without needing to modify core to make a provision for it.

Status: Needs review » Needs work

The last submitted patch, search_path_3.patch, failed testing.

adrian’s picture

Status: Needs work » Needs review
FileSize
9.05 KB

Fix tests. The file path had a '.' prefixed due to the handling of the system level modules/themes dir. I now have special handling for those paths.

febbraro’s picture

subscribe

arianek’s picture

subscribe + my 2 cents:

Thanks to those who are working on this patch - I would consider this extremely important as someone who bears a good amount of the overarching responsibility of whether the sites we launch are all up to snuff.

For anyone who is doing the majority of their development using install profiles, this is something that is absolutely necessary. We want to have full test coverage on any custom code/features within our profile that are dependent on contrib modules, just like for any part of core.

Unless I'm missing something, it sounds like we would have severely incomplete test coverage without fixing this? Never mind that it could help with some of the other (highly annoying) issues that we've come across working this method such as #545452: Store install profile in the generated settings.php file.

langworthy’s picture

I'm don't think symlinking should be considered a fix to this. As a developer who has benefited from the ability to test custom install profiles in D6, not fixing this for D7 seems like a step backwards.

Also, why is the priority of this normal when the problem removes testing abilities when upgrading D6 distributions to D7?

febbraro’s picture

My company build and supports 2 distros (with a 3rd on the way) and we consider test coverage paramount in the development of our install profiles. There are already 100s of things that make building and maintaing these distro difficult and very time consuming. Having something like #19 from adrian in place will make testing new releases a much cleaner and easier process w/o workarounds.

+1

jgraham’s picture

Huge thanks to all who are working on this;

1. Agreed with langworthy in #22, since this exists in D6 not fixing for D7 is certainly a step back.
2. As others have indicated symlinking is not a solution
3. Adrian's explanation in #14 is a good summary of the awkwardness of the current implementation
4. This will make it easier for shops to maintain distributions/install profiles which is a good thing for the Drupal community.

+1

jhedstrom’s picture

subscribing

eaton’s picture

I mean normally people shouldn't be running tests locally when testbot does it already and has optimized machines that do it faster (if anything we should improve it). So what exactly is our usecase here? I don't do distro work, but those in this issue I know have more experience in that area...so what does that look like?

In a nutshell: anyone developing an installation profile for use with Drupal who wants to use SimpleTests. There are theoretically ways around the problem, but they all involve a huge amount of wrangling to get around what was previously doable with out-of-box SimpleTest module.

We're building what amounts to a custom distro for a large client that will be relying on SimpleTest for regression testing: they're definitely concerned about D7 portability moving forward, and losing the ability to test their profile system without all sorts of symlinked singing and dancing would be a real disaster.

chrisshattuck’s picture

I'm curious if boombatower is still leaning against this one after hearing the testimonials from distro folks. I like the idea of supporting distros natively instead of requiring a symlink workaround. It seems broken to simply hide the tests in the UI.

Boombatower said one thing that maybe hasn't been fully addressed:

We should not be running tests for the contrib modules in an install profile as those are run on the contrib project already...no need to waste the testbot's time with overly redundant testing.

I have a couple of questions about this:

  1. Is it possibly a good thing to run tests on distros, so that end users can see that the distro is good to go, rather than the users having to cross-reference all the contrib modules too? Or is this info aggregated in in the distro info?
  2. Would it be possible to turn this off for the testbot but still allow devs and end-users to run the tests without the symlink workaround (which seems lame)
  3. If it can't be turned off, what kind of burden are we talking about here?

I'm also curious what changed from D6 to D7 that caused this to break. Is it any indicator of an important change that this patch would be un-doing?

dixon_’s picture

The move to test driven development in Drupal will (and have already) changed things to be much better. And the distro movement is also very very important for Drupal, as Dries keeps mentioning from time to time. We need to fix this or a smooth test driven development will never happen for distros in D7. As said, two very important things for Drupal.

As @langworthy says, symlinking is not a solution to the problem. Regardning @boombatower's concernc on this issue I think @adrian sums it up very well:

We are not including all profiles, only the host site's profile is getting included.

For example, if modules are present in sites/sitename.com/modules you will be able to test them because the 'test site' is installed as 'sitename.com' too and therefor sites/sitename.com/modules is in your search path.

If the site directories had the same behavior as profiles have currently, the testing interface would show you the modules in your interface, but when you run them the site will be installed explicitly as 'testing.com' (or whatever the test required) and only sites/testing.com/modules is added to the search path. Hence all the tests fail because the 'test site' cant actually find the modules it needs to test.

So huge +1 for this. Maybe bump the priority of this issue?

Jackinloadup’s picture

Does this need to be in before beta or 7.0 release or can it be implemented in a point release?

eaton’s picture

Does this need to be in before beta or 7.0 release or can it be implemented in a point release?

While it's not a major API change, I'd be very hesitant to change something about the way our testing system works after a major release. It seems like that is something we would want to stay consistent.

sun’s picture

This looks a bit over-engineered. Why is a simple drupal_alter() not sufficient?

FWIW, for http://drupal.org/project/edge, we are also in the need to be able to place themes for testing purposes into the module's directory, so I recalled this issue, but the functionality of this patch does not make it any simpler than achieving the same through a dirty hack.

Boris Mann’s picture

What is being perhaps missed here (although mentioned by some above), is that many are doing ALL site development for clients as install profiles -- i.e. common every day site building functionality. So, very important to have this work "out of the box" (man, I'm getting tired of writing that...)

bonobo’s picture

RE: "many are doing ALL site development for clients as install profiles" - yes. I suspect that as install profiles become easier to build, this will come to be best practice, and eventually something that everyone just does (remember building sites before CCK and Views?).

Losing the ability to test install profiles without hacks is a big loss, and it will create unnecessary barriers in the way of creating and maintaining install profiles throughout the D7 lifecycle.

Big +1 to addressing this now, and addressing this in core.

webchick’s picture

To me, this sounds like a pretty straight-forward regression from what was possible in D6 and should be fixed in D7. Symlinks are not an answer, and neither is the response that contrib can test its own modules. While true, the tests that contrib does will test each module individually. A profile will likely have one or more custom modules in it that will want to test the entire stack. Am I missing any other objections?

Obviously, though, what we do to fix here should be as non-invasive as possible, given that we're past beta now.

We could do with a hell of a lot less +1ing and a hell of a lot more actual code reviews in this issue, however.

chx’s picture

Status: Needs review » Needs work

We do not do anything like that. It's WAY overengineered. And, all this chatter did not even brush the real problem: tests will break if not ran in the profile they are designed for. For example, one presumes the existence of certain node types. Presumes the status of modules and as a consequence the DOM of the page when running XPath over it. So just making it possible (with a simple solution) to get a test to run on another profile, well it won't solve anything at all.

Also, any nontechnical chatter followup will be deleted without further warning.

yhahn’s picture

tests will break if not ran in the profile they are designed for

chx, please read the entire thread or at least the original post. The issue is not about running test A with a different profile than it was designed for. It is about the scope with which drupal_system_listing() makes code available and how this can cause problems for modules and their tests in profiles/[x].

Anonymous’s picture

subscribe.

chx’s picture

I read the issue and for example the opening post contradicts itself. How so? the first list is about "Move one of the core modules (e.g. poll) from modules/poll to profiles/standard/modules/poll and then clear your caches." but then the second list says "When Views' tests are run, the install profile used is standard, leading to the following paths scanned by drupal_system_listing(): profiles/standard/modules" erm, so profiles/standard/modules is good, after all? What is your problem, after all?

yhahn’s picture

We're discussing on irc now to clear this up.

Anonymous’s picture

which channel? or did i miss it in #drupal-contribute already?

chx’s picture

So the issue is superb convoluted. If your test specifies and the system contains profile X (say, standard) but you have installed the profile Y and the module is under it then profile X will be set up for the child Drupal and so the module will not be found under Y. The test needs to use X, yes, but the search needs to happen under Y. Note that if the profile X is such that it contains modules that are also under Y then a lot of of grief is going to happen. We can't help that for now. The hope is that X is indeed a core profile which usually does not contain modules. I proposed a superb small, insanely targeted fix (iterated with yhahn) and it's at http://paste.pocoo.org/show/272516/ , yhahn is rolling a patch with added comments, simpletest use it and a test. It's all good now.

webchick’s picture

There. That looks more like it. ;)

Thanks for working that out, guys!

yhahn’s picture

Status: Needs work » Needs review
FileSize
2.61 KB

As mentioned by chx, this patch takes a minimal approach to addressing this problem.

  • Introduces simpletest_parent_profile variable that is only meant to be used by simpletest to add one additional profile (the parent site's active profile) to the test site's search paths.
  • Adjust DrupalWebTestCase to set the parent profile during ->setUp() and delete it during ->tearDown().

Tested successfully with tests in profiles/[x] that use the testing and standard profiles.

dixon_’s picture

I successfully tested yhahn patch from #43.

chx’s picture

Status: Needs review » Needs work

It still needs comments in drupal_system_listing and I am wondering whether a test would be possible (without making the testing profile another test module dumping ground, webchick did not like that)

dixon_’s picture

Status: Needs work » Needs review
FileSize
2.76 KB

Here is yhahn's patch rerolled with some more comments in drupal_system_listing() as per chx's request. I can't figure out how to write a test for this, though.

yhahn’s picture

Thanks @dixon_ ... the only way I can think of to write a test for this is to actually create a test module in profiles/standard/modules that uses, say, the testing profile. Maybe there is another way that doesn't spam the profiles with test modules?

David_Rothstein’s picture

It looks like discussion of how (or if) to write a test for this is semi-postponed on #938600: Decide what to do with test modules under testing profile.

If I understand this issue correctly, though, then because the bug depends on the profile that the main site (the site which is running the tests, not the testing profile itself) is using, the only way to test this would be to do some kind of "meta-test"? Basically, put a module in profiles/testing/modules which contains a test which itself specifies use of the 'standard' profile. Then, write a test which uses the 'testing' profile, and within that test, have the Simpletest browser go through the admin UI and make sure that it can successfully run the module's test. It seems a little scary, but would probably work. Not sure if it's worth the effort... (It does look like within modules/simpletest/simpletest.test there are already some "meta-tests" that run tests from within the tests though.)

The patch itself looks fine to me. Ugly, but effective :)

EvanDonovan’s picture

Is this still under consideration for D7? This post on Development Seed's blog suggests that they need it for the work that they are doing with distributions in D7: http://developmentseed.org/blog/2010/sep/30/features-and-exportables-dru....

dasjo’s picture

subscribing

ademarco’s picture

subscribe

EvanDonovan’s picture

#46: 911354.46.patch queued for re-testing.

jason.fisher’s picture

Stalled?

sun’s picture

Issue tags: +Testing system
langworthy’s picture

The blocker issue mentioned in #48 is resolved and it seems David is correct that we can put a module in profiles/testing/modules.

Given this is a regression of D6 and that the patch seems to have the okay do we require a test for this to be committed?

jhedstrom’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
FileSize
2.46 KB

This would presumably have to be applied to 8.x first. Here's the same patch from #48 re-rolled against 8.x. I'm currently researching whether tests are possible for this patch.

Status: Needs review » Needs work

The last submitted patch, 911354-56-install-profile-tests.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
5.37 KB

Here's the same patch as from #56, but I also added a test. This test first bundles a test case under the 'testing' installation profile. There is then a new test case in simpletest.test that enables the testing profile, and verifies that the test bundled with that profile appears on the test admin form.

langworthy’s picture

The patch in #58 works for me.

I used Drupal 8.x HEAD and copied the standard install profile to another directory and installed to that profile. I grabbed the 7.x-dev versions of views and ctools, placed them in my new profile directory, and changed the core version to 8.x. I ran a single views test and it failed completely. When I applied the patch in #58 and ran the test again it passed.

langworthy’s picture

re-roll of #58

Status: Needs review » Needs work

The last submitted patch, 911354-60-install-profile-tests.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
5.37 KB

Looks like the testbot couldn't apply the patch in #60 for some reason. Here's another reroll.

langworthy’s picture

My bad. It looks to me now that #58 doesn't need a re-roll when applying the patch using git apply with drupal.git.

For some reason the patch won't apply for me when using drush make. But #60 does.

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/includes/common.inc
@@ -5151,8 +5149,14 @@ function drupal_system_listing($mask, $directory, $key = 'name', $min_depth = 1)
+  $profiles = array(drupal_get_profile(), variable_get('simpletest_parent_profile', ''));
...
+    if ($profile && file_exists("profiles/$profile/$directory")) {

Minor: if we do if($profile) , we might as well return FALSE as default value in variable_get()

+++ b/modules/simpletest/simpletest.test
@@ -503,3 +503,31 @@ class SimpleTestMissingDependentModuleUnitTest extends DrupalUnitTestCase {
+    // There is a test bundled with the drupal_system_listing_compatible_test module.

That's just a liiiittle bit too long. :)

+++ b/profiles/testing/modules/drupal_system_listing_compatible_test/drupal_system_listing_compatible_test.info
@@ -4,3 +4,4 @@ package = Testing
\ No newline at end of file

+++ b/profiles/testing/modules/drupal_system_listing_compatible_test/drupal_system_listing_compatible_test.test
@@ -0,0 +1,17 @@
\ No newline at end of file

:(.... :)

+++ b/profiles/testing/modules/drupal_system_listing_compatible_test/drupal_system_listing_compatible_test.test
@@ -0,0 +1,17 @@
+ *   Include a test to make sure tests withing installation profiles
+ *   can be run by simpletest.

Is that actually a standard or are there just a lot too many spaces there?

moshe weitzman’s picture

Could we just call this new variable install_profile? Drush already honors that variable when set. This variable name comes from #545452: Store install profile in the generated settings.php file which tried valiantly but failed to get into D7.

jrbeeman’s picture

@moshe, I was confused about it too, but upon further inspection, it looks like the simpletest_parent_profile variable is set using $this->originalProfile.

I've backported the patch in #62 to apply against Drupal 7.x head and attached it here. The patch appears to be working great for our D7 install profile.

moshe weitzman’s picture

Status: Needs work » Needs review
Issue tags: -Needs backport to D7, -Testing system

Status: Needs review » Needs work
Issue tags: +Needs backport to D7, +Testing system

The last submitted patch, 911354-62-simpletest-profiles.patch, failed testing.

sun’s picture

Title: Make system directories configurable to allow tests in profiles/[name]/modules to be run. » Tests in profiles/[name]/modules cannot be run and cannot use a different profile for running tests
Assigned: boombatower » sun
Status: Needs work » Needs review
FileSize
7.72 KB

As I'm neck-deep into the testing system currently, let me drive this home.

Re: #65: Nope, 'install_profile' would be a bogus name, as the variable does not hold the name of the actual installation profile, but instead, the profile of the parent site that is executing a test. The 'install_profile' variable is already in use.

Also note that SimpleTest did not properly install the installation profile of a test at all until very recently, if the profile was different to the profile of the parent site triggering the test run. #1373634: Installation profile is not installed and not registered as module, unless identical to parent site contained the essential fix to prime the 'install_profile' variable, so that the correct profile is installed in test runs.

Attached patch

  1. verifies that tests in installation profile modules are found.
  2. goes one step further and verifies that tests in an installation profile module can use a different installation profile for running tests (e.g., the testing profile), and modules within the installation profile are still found.

    E.g., a contrib module that happens to be packaged with a non-core installation profile may use the Testing profile for running tests, but still wants to install itself (and is located in the non-core profile, not the testing profile).

  3. goes another step further and verifies that tests of another/different installation profile than the parent site do not appear.

    I.e., modules and tests in profiles/whatever/modules are not supposed to appear when the site is installed with the profiles/awesome profile.

  4. clarifies the code comments for why this special casing is needed.
  5. cleans up class and test names.
  6. fixes various coding style issues.
  7. removes the variable_del() from tearDown(), as it's unnecessary, since the variable is only set in the child site.
sun’s picture

oopsie, fixed the stale comment in SimpleTestOtherInstallationProfileModuleTestsTestCase.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Nice tests! I raised an eyebrow at the name SimpleTestInstallationProfileModuleTestsTestCase but if thats how we roll, then OK with me.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/common.incundefined
@@ -5314,9 +5314,22 @@ function drupal_system_listing($mask, $directory, $key = 'name', $min_depth = 1)
+  // For SimpleTest to be able to test modules packaged together with a
+  // distribution we need to include the profile of the parent site (in which
+  // test runs are triggered).
+  $testing_profile = variable_get('simpletest_parent_profile', FALSE);
+  if ($testing_profile && $testing_profile != $profile) {
+    $profiles[] = $testing_profile;
+  }
+  // In case both profile directories contain the same extension, the actual
+  // profile always has precedence.
+  $profiles[] = $profile;
+  foreach ($profiles as $profile) {
+    if (file_exists("profiles/$profile/$directory")) {
+      $searchdir[] = "profiles/$profile/$directory";
+    }

Any code that only runs inside a test environment should be inside a check for drupal_valid_test_ua().

sun’s picture

Status: Needs work » Needs review
FileSize
7.54 KB
amateescu’s picture

Status: Needs review » Reviewed & tested by the community

#72 is fixed, bot approves, I'd call this RTBC again.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x, back to 7.x (I think I added the right files, we'll soon find out if not).

langworthy’s picture

Status: Patch (to be ported) » Needs review
FileSize
7.71 KB
sun’s picture

Status: Needs review » Reviewed & tested by the community

That should pass. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal7.simpletest-profiles.76.patch, failed testing.

langworthy’s picture

Strange, the failing File API test passes for me fine.

langworthy’s picture

Status: Needs work » Needs review
langworthy’s picture

Status: Needs review » Reviewed & tested by the community

I know we don't RTBC our own patches but sun originally set that status on the patch before the test bot messed up.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

webchick’s picture

Issue tags: +7.13 release notes

x

hefox’s picture

Status: Closed (fixed) » Needs review
FileSize
1.89 KB

Sorry for re-opening, but this appears to not work. Of course could be a regression or I'm doing something wrong with what I'm doing and thus should file/look for a new issue, but has anyone tested this with the 7.x version? No one mentions what they did to test the patch and from what I understand, simpletest in 8.x has diverged (though whether it was that different when this was committed, no idea). The original 7.x patches (pre issue going to 8.x) did not include drupal_valid_test_ua part.

As far as I can tell, during web test case's setUp, drupal_valid_test_ua returns false because while setup is running, a test isn't (the browser agent is still mozilla). So when it goes to enable modules, it can't.

(hm, It'd be possible to create a test for this by adding new install profile with a module with the test in it, and test that module can be enabled... overkill?).

favrik’s picture

After reading the thread (though only once), is the goal being able to run tests from modules that exist in a custom installation profile? If so, that is currently not possible. But as soon as I move the modules to `sites/all/`, tests are runnable.

s_leu’s picture

I was not able to run tests of my custom module that resided in the sites/all/modules/custom folder on an installation that uses the custom installation profile "recruiter". This profile installs Drupal 7.19 by default.

After applying the patch posted in #85 and testing again, i can confirm that the patch is solving the problem.

mpotter’s picture

Status: Needs review » Reviewed & tested by the community

We are using this (#85) in Open Atrium 2 to support our automated testing on Drupal 7.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

I think if we can we should provide tests for this, and #85 seems to suggest that would be possible. Also I think instead of introducing a new function I would rather fix drupal_valid_test_ua() if possible in WebTestCase::setUp(). I'm not sure, though, if that is possible.

Berdir’s picture

Hint. If you, for some strange reason, want to run tests for modules in a profile that is not actually your parent profile, you can manually set the originalProfile property in a test like this:

+  /**
+   * {@inheritdoc}
+   */
+  protected function prepareEnvironment() {
+    parent::prepareEnvironment();
+    // Make it possible to find modules in the profile.
+    $this->originalProfile = 'your_profile_name';
+  }
sun’s picture

Assigned: sun » Unassigned
Issue summary: View changes
mitokens’s picture

wizonesolutions’s picture

Found the same thing as @hefox: drupal_valid_test_ua() is always FALSE during DrupalWebTestCase::setUp(). Maybe a simple $_SESSION['simpletest_setup_active'] being set to TRUE and checked for in drupal_valid_test_ua() would suffice?

This must be broken in Drupal 8, too?

MiSc’s picture

Version: 7.x-dev » 8.2.x-dev

I think this is still an issue in Drupal 8 - if you test a module inside your own profile, you must add:

protected $profile = 'my-profile';

If you don't do that, tests will fail. Tests should be able to run, without adding that.

Or am I doing it the wrong way?

  • catch committed a8edf85 on 8.3.x
    Issue #911354 by adrian, boombatower, jhedstrom, sun, yhahn, dixon_, et...

  • catch committed a8edf85 on 8.3.x
    Issue #911354 by adrian, boombatower, jhedstrom, sun, yhahn, dixon_, et...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • catch committed a8edf85 on 8.4.x
    Issue #911354 by adrian, boombatower, jhedstrom, sun, yhahn, dixon_, et...

  • catch committed a8edf85 on 8.4.x
    Issue #911354 by adrian, boombatower, jhedstrom, sun, yhahn, dixon_, et...

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andrewbelcher’s picture

Priority: Normal » Major
Issue tags: +Contributed project blocker

I've just hit this in 8.4.6. Adding $this->originalProfile solved the module dependency issue, but it looks like container.namespaces is still not being set correctly, causing discovery to fail (in my case an entity type not being picked up)...

Is there any suggestion on a solution to this, if someone can suggest a direction I can have a quick go at fixing it. Using the install profile for the test adds a huge amount of complexity to the form I'm trying to test (via alters in other modules in the profile) that I really don't think that's very workable for me...

andrewbelcher’s picture

Ok, with a bit of debugging, it looks like while $this->originalProfile adds a line to the test's settings.php which solves the module dependency on install, when it then rebuilds the data (\Drupal\Core\DrupalKernel::moduleData), it uses $this->getConfigStorage(), which loads the active config excluding overrides, which means the setting in settings.php is ignored and it can no longer find the module, so the module is then excluded from the list (and so not included in container.namespaces on rebuild).

So I think the fix is that \Drupal\Core\DrupalKernel::moduleData needs to read from the config factory (to include overrides), unless there is a specific reason to use the active un-overridden config here?

As for a work around, tests can do the following:


  /**
   * {@inheritdoc}
   *
   * Ignore the simpletest schema so we can set the profile.
   */
  public static $configSchemaCheckerExclusions = ['simpletest.settings'];

  /**
   * {@inheritdoc}
   * 
   * Set the parent profile for config that reads directly from the database.
   */
  protected function initConfig(ContainerInterface $container) {
    parent::initConfig($container);

    $container->get('config.factory')
      ->getEditable('simpletest.settings')
      ->set('parent_profile', 'delay_repay')
      ->save();
  }

  /**
   * {@inheritdoc}
   * 
   * Set the original profile for writing to settings.php config overrides.
   */
  protected function prepareEnvironment() {
    parent::prepareEnvironment();
    // Make it possible to find modules in the profile.
    $this->originalProfile = 'delay_repay';
  }

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

  • catch committed a8edf85 on 9.1.x
    Issue #911354 by adrian, boombatower, jhedstrom, sun, yhahn, dixon_, et...

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

Project: Drupal core » SimpleTest
Version: 9.1.x-dev » 8.x-3.x-dev
Component: simpletest.module » Code

Triaging issues in simpletest.module as part of the Bug Smash Initiative.

This looks like it belongs in the Simpletest project.