Reestablish test coverage of the toolbar module that was disrupted by mobile update work

I am assigning this to myself in the hope that we do not duplicate our efforts. I plan to spend today working on the issue.

In #1137920-261: Fix toolbar on small screen sizes and redesign toolbar for desktop, @jthorson pointed out that ModuleApiTest passes if breakpoint is added to the list of dependencies in standard.info. I think this is fixing the symptom. If a profile depends on module1 and module1 depends on module2, does the profile have to declare module2 as a dependency? I do not think it should, but I will check the docs to see. If not, then I will change the test.

I checked the docs, specifically How to Write a Drupal 7 Installation Profile:

The dependencies list includes all modules that will be enabled when this profile is installed

I take that to mean that the answer to my question in #1 is yes: the profile does have to declare module2 as a dependency. Instead of rewriting the test, I will add to standard.info.

The attached patch addsdependencies[] = breakpoint
to standard.info, and then sorts the list of dependencies. The second part should make no difference to the code, but it makes it easier to see whether a given module is on the list.

In case someone disagrees about sorting the list, I have made separate commits in the sandbox: 9f64aa6 and cb8f6e4.

I am starting to look at BareStandardUpgradePathTest.

Debugging can be a humbling process. Also time-consuming.

I think I understand the problem. In BareStandardUpgradePathTest, we start with a D7 site and upgrade it. The toolbar module is enabled in the D7 site, so it is enabled after the upgrade, but its dependencies are never checked (!). Also, the test never installs the standard profile (nor any other) so its dependencies are not checked (see #3 above). Then toolbar_view() calls entity_load('breakpoint_group', 'module.toolbar.toolbar') which ends up calling entity_get_info('breakpoint_group') (which answers #1137920-263: Fix toolbar on small screen sizes and redesign toolbar for desktop).

At this point, I think that the upgrade script should be fixed so that it checks dependencies before enabling modules. I also think that entity_get_info() should not lead to a WSOD if it is passed a bogus parameter.

@jeesebeach:

I already did, and it did not help. Please re-read #4.

If we want a quick fix to pass the test, we can bail out if the breakpoint module is not enabled. Kind of silly, since toolbar depends on breakpoint. Come to think of it, that might fix the rest of the failing tests all at once.

#6 You have to write an update function (in system.install) that enables the breakpoint module if toolbar is enabled while doing the update. The profile never gets executed/processed.

@attiks:

Thanks, that should be pretty easy. This looks like a good model to follow:

/**
 * Enable the Actions module.
 */
function system_update_8021() {
  // Enable the module without re-installing the schema.
  update_module_enable(array('action'));

Meanwhile, I ran all the failing tests locally, replacing

  $breakpoints = entity_load('breakpoint_group', 'module.toolbar.toolbar');

with

  $breakpoints = array();

and everything looks green.

I was a little worried that update_module_enable() might cause problems if it is called on a module already enabled. So I tested a couple of times with the line

  update_module_enable(array('breakpoint', 'config'));

doubled. No problem (at least in the five tests that I was running).

Committed to the sandbox: 38a20bb.

I think we are done fixing broken tests, so we can start thinking about adding new ones.

This screenshot shows the toolbar at the top and the five tests that I ran locally. :)

@benjifisher can you write a patch to fix this?

@attiks:

The new .install file is already in Jesse's sandbox, and I expect she will incorporate it into the next patch on #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop. (The goal of this issue is to get a patch there that will keep the test bot happy!) But since you ask, I have attached a patch and set the status to NR so that the test bot can have its say. It includes all the changes from #3 and #9 above.

nice one

@attiks:

I think I owe you additional thanks for the advice in #7. I do not know how much time you saved me! I am glad I decided to post a progress report in #4 instead of working in isolation.

@benjifisher you're welcome ;-)