modules_installed is broken during testing

I don't get it. Why is "modules_installed" broken?

$disable_modules_installed_hook = FALSE. This is ugly enough, without a double negative. Please use $modules_installed_hook = TRUE instead

So the issue is that the module hook cache is not flushed after we call drupal_install_system(). Would the attached patch work?

More precisely, the issue is we preload the registry of the child site with information from the parent registry, making module_implements() believe that it should run those hooks.

The solution is to preload the registry way later, just before rebuilding it, not before enabling modules.

A static modifier was missing in currentlyRunning().

@Damien: your patch avoids any problems during install, but not if a test enables a module post-install. in that case, we have the same problem, as we can get bogus calls to hook_modules_installed, which is bound to bite us somewhere.

Oh, true, but that's easy to fix: simply $this->canPreloadRegistry = FALSE; after the registry is rebuilt.

I still like my approach better.

@justinrandell: I see no reason why an install function couldn't (at least indirectly) call a hook itself. So this is very fragile.

The way to go is what I implemented in #6, with the addition of a $this->canPreloadRegistry = FALSE; after module installation, so that we don't preload the registry at latter calls (it would be inefficient).

Note that you were wrong in #7:

@Damien: your patch avoids any problems during install, but not if a test enables a module post-install. in that case, we have the same problem, as we can get bogus calls to hook_modules_installed, which is bound to bite us somewhere.

FALSE: in my approach, we only preload the registry at the very last moment: just before rebuilding it. It's 100% safe, in all cases.

Here is a reroll.

Should I add that my patch also has the added benefit of implementing DrupalWebTestCase::currentlyRunning() which will prove useful (in a later issue) to cleanup some of the the crappy "simpletest" regexps that pollute core.

subscribe. :)

*sob* this is still an issue.

Rerolled.

The Field tests are already defining a $instance member variable. Moving to $this_instance.

I'd still like to see an answer to #14, and ideally a final review from @justinrandell.

Investigating the different approaches, I thought justin's approach was less of a hack.

I wonder if we truly need the theme registry. I'd love to rip it out and just do real time template discovery - then benchmark. Don't think I will get to that soon, but someone should.

Subscribe. I'm wondering if it's the same bug we're hitting in #372743-18: Body and teaser as fields (second paragraph) :

The fails are caused by the body_field instance being marked as widget_active = FALSE, because of module install workflow at setUp:
The 'book' content type (and thus the body instance for 'book') is created in book_install(). By this time text.module is installed, but not 'active', because drupal_install_modules($module_list) installs all modules, *then* marks them as active and refreshes module_list().
So $widget_active = module_exists($widget_module); in _field_write_instance() returns FALSE.
All tests pass if I replace this line with $widget_active = drupal_function_exists($widget_module . '_field_widget');

The 'body as field' patch is getting stale, so I'd need to reroll it first and test.

1.

+  // When running in SimpleTest mode, we preload the child registry using what
+  // we already know from the parent registry.

I'd like to see us add a sentence that explains _why_ we do this.

2. The name "$this_instance" is rather poor. Can we think of a more descriptive name like $current_test or something?

3.

+    // It is now useful to preload the child registry site with information
+    // from the parent registry.

I'd like to see us add a sentence that explains _why_ it is now useful and/or why it wasn't useful before.

Re: #23: we need to rebuild the registry as soon as possible, for two reasons: (1) static caches are full of information from the parent site and (2) the registry tables are empty, so hooks implemented by modules installed by drupal_install_system() will not be recognized.

This operation should be relatively cheap, especially if we decide to pre-copy include/* files.

Bump..

#515040: Simpletest throws exception on a fresh install that I filed a few days ago may stem from this problem. In short, simpletest doesn't work with a fresh install of head if the profile module is enabled.

A call to menu_rebuild() is made when simpletest is installing the toolbar module for the child site. Even though the profile module is not installed (and will not be installed), menu_rebuild() somehow knows that the profile module is enabled on the parent site, something that shouldn't happen.

This patch does not apply. Some hunks have been applied to HEAD already. What's the status of this issue?

I've created #594234: hook_modules_installed() not invoked during tests which I believe is a related problem.

$disable_modules_installed_hook argument was introduced in the monster Field API patch, but the registry is gone now, and disabling this hook for modules that are installed in SimpleTest totally breaks my tests in a contributed module.

This is a straight re-roll against HEAD. If this passes, then we don't really need to preload the class registry, and only do it for performance reasons. Which in turn would mean that we can simply remove the argument.

Continuing that conclusion, I can run tests just fine with a total revert of the $disable_modules_installed_hook argument.

I have no idea.

This patch also breaks #569326: Add hook_taxonomy_vocabulary_info()

still no luck

Seems like http://api.drupal.org/api/function/rdf_install/7 had the same problem...

More clean-ups, but Drupal render test is still failing for me (as for the bot).... chx mentions it passes for him... (?)

I'm really running out of ideas now... additionally synced with install_finished() now.

We're calling $this->refreshVariables(); too late?

subscribing

Let's see whether #399642: Replace drupal_install_modules() with an improved module_enable() solved the problems.

Thus, re-rolled, no further changes. So this patch still updates setUp() to do exactly the same as http://api.drupal.org/api/function/install_finished/7

Mixing #685004: Cannot run tests under non-standard profile into the game.

subscribing

.

#48: drupal.modules-installed.48.patch queued for re-testing.

Re-rolled against HEAD and removed the change for unit tests (no modules installed here).

Obviously, the theme system information is not reset at all for any test that is run.

This works locally, let's see what the testbot has to say.

The test bot doesn't like you anymore, sun. ;)

No surprise... who doesn't hate me?

Actually, those entire RDF tests contain plenty of workarounds for the bug that this issue is fixing. Removed all of those. After doing so, it seems that the RDF functionality seems to work properly, but the tests are trying to verify totally weird stuff, which was true with previous workarounds, but not with a proper module installation behavior.

well, so why don't you eat this?

or rather this? :P

oh my, it's getting late here, sorry.

@sun, yes, we really had a hard time getting the rdf tests to work and found out hook_modules_installed() was not invoked during testing (plus other caching issues). It obviously came form simpletest since everything worked when installing manually. These workarounds can be removed now the underlying issue is being fixed. The patch attached fixes the rdf test which failed in #66 (similar to patch in #65).

+++ modules/rdf/rdf.test
@@ -196,13 +192,14 @@ class RdfCrudTestCase extends DrupalWebTestCase {
-    $this->assertTrue(rdf_mapping_save($mapping[1]) === SAVED_NEW, t('Second mapping was inserted.'));
+    rdf_mapping_save($mapping[1]);

#66 failed, because I thought I could remove this rdf_mapping_save().

Why is it needed? Shouldn't rdf_entity_info_alter() store the mapping already?

EDIT: The result of that save operation is 0 (zero), returned by db_merge(), which (I think) means that nothing was inserted and nothing was updated.

The second mapping is for the blog bundle (see in rdf_test.module), and because the blog module is not enabled in this test there is no 'blog' bundle in the entity info cache to hold that mapping. That's why we need to have the explicit save operation (which is fine since that's the operation we're testing here).

EDIT: the above is not a good explanation since we're not using the entity info cache in this particular test. I'll rewrite the save operation test tomorrow morning.

the variable names in RdfCrudTestCase were confusing so I've refactored it. all rdf tests are now passing on my machine.

Awesome! Thanks for helping out, scor!

Committed to CVS HEAD. Thanks.