Fixed: Functional tests: LayoutUpgradePathTest fails too often in GHA.

[indigoxela]

Description of the bug

Yet another failure of LayoutUpgradePathTest. In two different PHP versions. In a PR, that doesn't even touch PHP-code.

This tests fails too often, to call it "occasional". Not always the same PHP version, not all PHP versions at once, but way too often to ignore.

The test chokes with: Call to undefined function layout_load_multiple_by_path() (ships with layout.module).

Steps To Reproduce

No idea, the test passes locally, of course.

Some race condition of update hooks, when upgrading? Doesn't seem reasonable, as the Layout module gets enabled in system_update_1000().

Additional information

• Backdrop CMS version: latest dev,
• PHP version: random

[quicksketch]

Elaborating on where the error occurs, test failures look like this:

With the first error being:

 Exception Error      dashboard.module  14                                                                              
    Call to undefined function layout_load_multiple_by_path()

Line 14 of dashboard.module is an implementation of hook_menu():

function dashboard_menu() {
  $items = array();

  // Check if any dashboard is present and is not disabled.
  $dashboard_layouts = layout_load_multiple_by_path('admin/dashboard'); // <-- Line 14
  ...
}

A guess as to what might be happening here: The old Drupal 7 dashboard.module exists in the drupal-7.filled.database.php.gz database backup. Since the module was enabled in D7, the hook dashboard_menu() might be getting called before the update hooks are fired (which enables Layout module).

Now why dashboard_menu() would be getting called, I'm not sure. Possibly a call to menu_rebuild() from an update hook? Or something that needs to load a menu entry might trigger a first-time build of the menu system.

[avpaderno]

menu_rebuild() is called from 34 functions/methods, including BackdropWebTestCase::backdropCreateContentType() which is called from 126 tests. None of those tests are in the LayoutUpgradePathTest class, though.

[avpaderno]

Maybe, the easier way to fix this is for dashboard_menu() to first check the Layout module is enabled. The Dashboard and the Layout modules are only installed when Backdrop is installed with the Standard profile; in a Drupal 7 migration, only the Drupal 7 Dashboard module is eventually installed.

[avpaderno]

(Notice that the Dashboard module does not list the Layout module as its dependency, but that is understandable: Differently, upgrading from Drupal 7 would probably be not possible, as the Drupal 7 Dashboard module does not depend on the Layout module, but the Block module.)

[indigoxela]

the Dashboard module does not list the Layout module as its dependency

At least, that's easy to explain: the Layout module in B is required, it can't be off. So there's no need to declare a dependency.

[avpaderno]

It is required from the Standard profile, but other profiles could not require it.

[klonos]

I've had instances of this failure where dashboard.module is not mentioned anywhere in the log, and where there are no exception errors - just exception warnings and exception notices:

[indigoxela]

@klonos this does shed a light on it, though. Hopefully...

Your test fails with:

require_once(...path...to...backdrop/modules/user/user.pages.inc)

^^ And that's a Drupal path. The Backdrop path would be ...core/modules/user/user.pages.inc (note the "core/").

So function system_block_view() tries to include a Drupal file. 🤔

[indigoxela]

It is required from the Standard profile...

@kiamlaluno it's not the profile, but the module is defined as required by its info file. You don't even see the Layout module on /admin/modules/list, which is the case for all required modules.

[klonos]

@indigoxela I believe that the .../backdrop/backdrop/... path is something that tugboat does when copying the codebase as part of provisioning the instance (because it is cloning from https://github.com/backdrop/backdrop) 👀

[indigoxela]

I believe that the .../backdrop/backdrop/... path is something that tugboat does

It's actually the GitHub Action runner, the dir is always /home/runner/work/backdrop/backdrop. (Tugboat is unrelated here.)

But it's still the wrong path - no "**/core/**modules..." but "/modules..." as in Drupal 7.

[klonos]

But it's still the wron path - no "**/core/**modules..." but "/modules..." as in Drupal 7.

Yeah, I don't know why that happens 🤷🏼

[avpaderno]

It is also an explicit dependency of the Standard profile and the Testing profile.

Anyway, what I was trying to say is that, during a migration from Drupal 7 (which is what the LayoutUpgradePathTest class seems to test), the Dashboard module cannot expect the Layout module to be installed.

[indigoxela]

A little more brainstorming... we're supposed to be on core/update.php at that point. And in fact, there's a module_exists('dashboard') with a permission check in that file.

It's there since 2019 though, and it's not clear why this suddenly would triggers random errors on different PHP versions.

And most of all: I ran this test locally in several combinations and it always passes.

[argiepiano]

Wondering if there is a way to see the verbose output of the automated tests in Github? When I run tests locally, I get lots of output that includes the pages that include the data sent to the server via POST, and all server responses to requests. This has helped me a great deal in the past. But I don't seem to be able to see that output in GHA.

[avpaderno]

The code in core/update.php that verifies the Dashboard is enabled is the following one.

function update_helpful_links() {
  $links['front'] = array(
    'title' => t('Home page'),
    'href' => '<front>',
  );
  if (module_exists('dashboard') && user_access('access dashboard')) {
    $links['dashboard'] = array(
      'title' => t('Dashboard'),
      'href' => 'admin/dashboard',
    );
  }
  elseif (user_access('access administration pages')) {
    $links['admin-pages'] = array(
      'title' => t('Administration pages'),
      'href' => 'admin',
    );
  }
  if (user_access('administer site configuration')) {
    $links['status-report'] = array(
      'title' => t('Status report'),
      'href' => 'admin/reports/status',
    );
  }
  return $links;
}

I guess that, at some point, for some reason, there is code that invokes hook_menu() for those links.

[indigoxela]

Just tried a totally different approach: run an upgrade from a D7 site that had the dashboard module enabled (never did that before, as that module was always off).

Hm, doesn't throw an error, but doesn't actually work, either.

After upgrading, the module shows as enabled, but neither the dashboard layout exists, nor does the functionality work after importing a vanilla dashboard layout.
Uninstalling/installing the module fixes that.

Why I tried that?

Eventually the test fails, because something's actually broken in that specific part of the upgrade. 😉 Just in case...

[klonos]

For those that cannot reproduce this on their local: crazy thought, but are you using apache (which is what our tests are using) or nginx?

[klonos]

...also, more recently I've been having both LayoutUpgradePathTest and BareUpgradePathTestCase fail with Call to undefined function layout_load_multiple_by_path() in pairs (in the same php version I mean), both pointing to dashboard.module, line 14:

---- Upgrade path: Bare upgrade test (BareUpgradePathTestCase) ----


Status    Group      Filename          Line    Function                            
------------------------------------------------------------------------------------------------------------------------
Exception Error      dashboard.module  14                                                                              
    Call to undefined function layout_load_multiple_by_path()
Fail      Other      upgrade.test      187     performUpgrade()                                                        
    "Updates were attempted" found
Fail      Other      upgrade_bare.test 23      testBareUpgrade()                                                       
    The upgrade was completed successfully.
Error: Process completed with exit code 1.

Screenshot:

[indigoxela]

For those that cannot reproduce this on their local: crazy thought, but are you using apache (which is what our tests are using) or nginx?

@klonos good idea, but unfortunately my dev setup is comparable to the GHA setup. Apache + php-fpm. The problem never shows on my dev, but I never run all the tests, as my dev box just isn't powerful enough for that.

I'm afraid it's a problem caused by some race condition. We added some functional tests recently, so the order of tests run in batches changed.

[indigoxela]

FTR: the order of calling functions is:

1. simpletest_script_run_one_test
2. run
3. setUp
4. resetAll
5. backdrop_flush_all_caches
6. menu_rebuild
7. menu_router_build
8. dashboard_menu

So flushing the cashes triggers menu_rebuild, which triggers hook_menu.

Fancy, that this doesn't actually always fail, as the upgrade tests try to get rid of all caches (also static caches). But they never fail on my local dev. So, no update here, still unsolvable.

[quicksketch]

This is going to be a real forehead smacker if my wild guess at backdrop/backdrop#4669 actually fixes this problem.

While working on #2557, I was moving config functions around to be super-early bootstrap, and I got a clear set of errors that dashboard.module couldn't find the Layout class during upgrade tests. Obviously Dashboard module needs Layout module, but since Layout module is required, shouldn't it always be available? Maybe not! I think the D7 LayoutUpgradeTest might keep failing because the D7 Dashboard module was also on by default. So when upgrading, Dashboard module might actually be enabled before Layout module is, thus causing a big problem.

So the fix? Maybe we just explicitly declare Dashboard module to depend on Layout module? Could it be so easy?

I re-ran the tests 3 times on backdrop/backdrop#4669 with perfect 100% passing every time across all PHP versions. I also got 100% passing a few times on my PR for #2557, which also had the same fix.

I'm going to keep running it a few times more and see if continues passing every time. 🤞

[indigoxela]

This is going to be a real forehead smacker

What the... 🤯

@kiamlaluno already suggested it, but I've been too stubborn to seriously consider (shame on me!).

A one-liner fix. 🥇

[quicksketch]

It's now passed 100% 4 more times. There's zero-harm in trying out this fix. I think we should merge it and just see if it fixes the problem.

[laryn]

I think we should merge it and just see if it fixes the problem.

Agreed -- and merged into 1.x!

Thanks @quicksketch, @kiamlaluno, @indigoxela, @klonos, and @argiepiano for all the brainpower that went into this one liner! Fingers are now crossed.

[quicksketch]

Thanks folks! And sorry I didn't consider this sooner considering @kiamlaluno's suggestions earlier. Honestly, I'm surprised that just specifying the dependency would be sufficient, but it seems to have done the job.