I have removed the reset functionality from module_load_all as "useless" which was reverted before the {system} table removal patch went in, I already complained in another issue and was told that it needs to stay in.

However, it now causes random failures all over the place. This is hardly surprising: I have not removed it because I have a penchant for removing random things but because it was broken.

This was first fixed in #140 in that issue as seen in http://drupal.org/files/interdiff-139-140.txt. That, in itself, is not enough now because theme() throws an exception with that fix. #1331486: Move module_invoke_*() and friends to an Extensions class has the current fix, no time to roll it now.

CommentFileSizeAuthor
#29 drupal8.test-module-load-all.28.patch5.26 KBsun
#27 drupal8.test-module-load-all.26.patch4.87 KBsun
#25 drupal8.test-module-load-all.25.patch5.17 KBsun
#22 switch-teardown-order.patch6.04 KBBerdir
#19 drupal8.test-module-load-all.19.patch1.83 KBsun
#9 drupal8.test-module-load-all.9.patch1.55 KBsun
#8 drupal8.test-module-load-all.8.patch991 bytessun
#6 meh.patch1.91 KBchx
#4 meh.patch1.9 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

chx CreditAttribution: chx commented

You hit the following when the cache happens not to be warm when your test runs its tearDown (hence the random aspect):

exception 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest764204key_value' doesn't exist' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php:58
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php(58): PDOStatement->execute(Array)
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php(506): Drupal\Core\Database\Statement->execute(Array, Array)
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Query/Select.php(420): Drupal\Core\Database\Connection->query('SELECT 1 AS exp...', Array, Array)
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Query/Merge.php(417): Drupal\Core\Database\Query\Select->execute()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.php(98): Drupal\Core\Database\Query\Merge->execute()
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/system.module(3006): Drupal\Core\KeyValueStore\DatabaseStorage->set('system.theme.da...', Array)
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/ExtensionHandler.php(193): system_rebuild_theme_data()
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/ExtensionHandler.php(117): Drupal\Core\ExtensionHandler->systemList('module_enabled')
#8 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/ExtensionHandler.php(83): Drupal\Core\ExtensionHandler->moduleList('module_enabled')
#9 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc(2567): Drupal\Core\ExtensionHandler->loadAll(false, true)
#10 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(874): module_load_all(false, true)
#11 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php(824): Drupal\simpletest\TestBase->tearDown()
#12 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(637): Drupal\simpletest\WebTestBase->tearDown()
#13 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(381): Drupal\simpletest\TestBase->run()
#14 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('15', 'Drupal\system\T...')
#15 {main}

I have seen this in totally unrelated patches as well, it's not because the extensions patch happens to move things.

chx’s picture

chx CreditAttribution: chx commented 
function module_load_all($bootstrap = FALSE, $reset = FALSE, $loaded = FALSE) {
  static $has_run = FALSE;

  if ($reset) {
    $has_run = $loaded;
  }

and change the reset calls in tearDowns from FALSE, TRUE to FALSE, TRUE, TRUE.

chx’s picture

chx CreditAttribution: chx commented

Finally, I assign these issues (there are so far six at least) to sun not because I do not appreciate his work on the {system} removal -- I admit the code became cleaner -- but because he did not talk to anyone about what the possible consequences are about reverting parts of a patch this complicated. And, I was too tired of fighting to throughly review a 80KB interdiff...

chx’s picture

chx CreditAttribution: chx commented
Status: Active » Needs review
FileSize
meh.patch1.9 KB

Status: Needs review » Needs work

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

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Needs review
FileSize
meh.patch1.91 KB

Status: Needs review » Needs work

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

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Priority: Critical » Major
Status: Needs work » Needs review
FileSize
drupal8.test-module-load-all.8.patch991 bytes

Any call to system_list() causes this chain of errors. The patch here attempts to touch the symptom but not the cause.

The real question you need to ask is this:

Table 'drupaltestbotmysql.simpletest764204key_value' doesn't exist

Why is system_list() still querying simpletest764204key_value instead of key_value when being invoked from tearDown(), after the environment has been reset to the parent site/test runner?

Clearly, system_list() is operating on the wrong container.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal8.test-module-load-all.9.patch1.55 KB

More context.

Status: Needs review » Needs work

The last submitted patch, drupal8.test-module-load-all.9.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review

#9: drupal8.test-module-load-all.9.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

#9 failed with "Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]." - not related to the actual patch.

Also, I apologize for my snarky reply. It's really hard to resist that when you're continuously accused to have removed a "proper bug fix" that gets repeated in "meh.patch" files by people who obviously know it better.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

That's another reason I am assigning to you -- because you do know better. Thanks for the clearer fix.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal8.test-module-load-all.9.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Needs review

#9: drupal8.test-module-load-all.9.patch queued for re-testing.

Edit: oh, the irony: this failed because of another random testbot failure.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Following. #1764474: Make Cache interface and backends use the DIC exposed this problem in a consistent way and I had to fix it there too. Did fix it in a slightly different way, though by moving the drupal_container() restore up, not the module reset calls down, because we also need the original container back before drupal_static_reset() is called.

The reason for that is that drupal_static_reset() can trigger a __destruct() of ThemeRegistry() which in turn ends up calling system_list().

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

The reason for that is that drupal_static_reset() can trigger a __destruct() of ThemeRegistry() which in turn ends up calling system_list().

If that happens, then it must happen in the test environment (not in the parent site/test runner environment).

I quickly checked your patch over there, and it breaks exactly that.

TBH, those magic destructors are a PITA to deal with though... I can perfectly see how a drupal_static_reset() will trigger __destruct() methods that will immediately call into other functions and re-prime statics and caches. :(

I'm not sure how to combat that in a reliable, once-for-all/-eternity way.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I'm not sure how to combat that in a reliable, once-for-all/-eternity way.

Hrm. Speaking of, a pretty straightforward solution might be this insanity:

drupal_static_reset();
drupal_static_reset();

Inline comments would have to clarify, of course. ;)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Priority: Major » Critical
Issue tags: +Testing system
FileSize
drupal8.test-module-load-all.19.patch1.83 KB

Implements #18.

Status: Needs review » Needs work

The last submitted patch, drupal8.test-module-load-all.19.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Ok, as I figured out while working on #1764474: Make Cache interface and backends use the DIC, I think I know what the actual problem is here.

Right now, this is the order of things:

1. Remove test tables.
2. Restore original database connection
3. Clear caches.

I *think* we need to change it to this.

1. Clear caches
2. Remove test tables.
3. Restore original database connection.

This allows to execute the cache clears within the original environment. They can do whatever they please to the test database, we don't care. The static caches should be cleared anyway. It might be possible that we need to do certain clears again in the original environment, we'll see. The more stuff we move into the container and can just get back, the fewer of that should be necessary...

Am I missing something? ;)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
switch-teardown-order.patch6.04 KB

Hm. Tried it. For web tests, it seems to work nicely, but unit tests don't like it and seem to get stuck?

Anyway, for testing purposes, here is a patch that tries this and for now duplicates the tearDown() method of unit tests to keep the old behavior there.

Status: Needs review » Needs work

The last submitted patch, switch-teardown-order.patch, failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

We're over thresholds. Can I commit #9 so we can get this out of the criticals queue and work on a more optimal fix in a normal task?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.test-module-load-all.25.patch5.17 KB

@Berdir: Isn't #19 implementing exactly what you descibe in #21?

1) Clears caches
2) Removes database and files and restores database connection
3) Clears caches again
4) Restores container

The only remaining problem with #19 is that WebTestBase::tearDown() drops all tables before calling into TestBase::tearDown().

Thus, the only thing we would need to "repeat" is the first drupal_static_reset().

However, since I'm moving the removal of tables in #1774388: Unit Tests Remastered™ anyway into TestBase::tearDown() already, I'm going to merge that change into this patch instead.

Status: Needs review » Needs work

The last submitted patch, drupal8.test-module-load-all.25.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.test-module-load-all.26.patch4.87 KB

Sorry, that was merged too quickly -- forgot to move the drupal_static_reset() above the removal of db tables.

Also realized that language() is not properly reset.

Status: Needs review » Needs work

The last submitted patch, drupal8.test-module-load-all.26.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.test-module-load-all.28.patch5.26 KB

Odd, I don't really see why the patch shouldn't apply against latest HEAD -- re-rolled.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I think this is ready to fly.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Random patch failures due to module_load_all reset » Teared down test environment leaks into parent site/test runner
Status: Needs review » Reviewed & tested by the community

Clarifying issue title. I'm also marking this RTBC, even though the patch to commit is my own, but this is a critical issue that is blocking other patches, and part of the changes were rtbc'ed earlier already.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Component: base system » simpletest.module
chx’s picture

chx CreditAttribution: chx commented

I think this is good to go, yes. It'd be superb hard to test this and having unit tests clean up their tables is good.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Cool, thanks for the verification, chx.

Committed and pushed to 8.x. Bomp. Bomp. Bomp. A critical bites the dust-AH!

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