Prevent tests from deleting main installation's tables when parent::setUp() is not called

To reproduce, simply comment the parent::setUp() line of any module's test file and run its tests.

Just determined that (thankfully) this is not universally the case, so at least someone can't easily take down a testbot by embedding one such comment in a patch and uploading it to the issue queue. For example, when I commented the first setUp() I found in system.module, it did not result in the loss of all the tables when I ran all of system.module's tests. However, the second example (with the new module) is always reproducible. I've not figured out yet what the critical difference is.

An aside, I see a distinction between "supporting broken code" and "preventing broken code from dropping all database tables." ;)

The function to clean broken installations does just such a check to only delete tables with the simpletest prefix:

/**                                                                             
 * Removed prefixed tables from the database that are left over from crashed tests.                                                                            
 */
function simpletest_clean_database() {
  $tables = db_find_tables(Database::getConnection()->prefixTables('{simpletest}') . '%');
  $schema = drupal_get_schema_unprocessed('simpletest');
  $count = 0;
  foreach (array_diff_key($tables, $schema) as $table) {
    // Strip the prefix and skip tables without digits following "simpletest",  
    // e.g. {simpletest_test_id}.                                               
    if (preg_match('/simpletest\d+.*/', $table, $matches)) {
      db_drop_table($matches[0]);
      $count++;
    }
  }

  // ...snip....

So, we would just need to do something similar in DrupalWebTestCase::tearDown.

I'll try my hand at rolling and testing such a patch.

Patch attached. It simply prevents the tables from being deleted when the database prefix is not a valid simpletest prefix.

I confirmed this prevents the issue for mymodule above. The test instead returns an error message that the database connection is invalid.

Tagging this for backport to D7. We might also want to check if the issue exists in the D6 contrib module.

The attached module should be the simplest case that produces the original bug (for testing purposes). Don't run its tests on a non-sandbox!

Edit: note that there's a typo in the commented line, so if you wish to uncomment it to test the case where the paren::setUp() is called, remove the underscore from my_module.

Ah, that is a good idea. code reuse++. Edit: I did not find any existing issue open regarding disabled tables not being cleaned.

I confirmed that a test that disables modules leaves leftover tables. (An example is Field API's "Field Info Tests.").

Unfortunately, it seems none of the database tables are properly cleaned with #13 applied. Looking into why this might be now.

It seems we simply need to move the call to simpletest_clean_database() down a couple lines to after the original DB connection is restored.

I confirmed that with that modification, the concept in #13 cleans the database properly and prevents the original table-dropping bug. There still seem to be two leftover tables after the Field API test I mention above, but this can probably be addressed separately.

I'll reroll against 8.x shortly.

Hmm, lot of failing tests. This is confusing because (some of) the tests ran fine locally. The only difference is that I was testing in 7.x.

I'll requeue just to double-check it's not a fluke.

#16: simpletest-prevent-dropped-tables-1212992-16.patch queued for re-testing.

Remove inaccurate information

I'd rather replace the current schema-based dropping of tables by calling

array_map('db_drop_table', db_find_tables($this->databasePrefix . '%'));

That's guaranteed to drop only the tables of the test run.

Hm, would it make sense to factor this into a helper, something like simpletest_clean_database_by_prefix($prefix)?

Here's a patch using sun's suggestion. Seems to work locally: tables for disabled modules are properly cleaned, the host environment's tables don't get nuked, and the Clean database button behaves as expected.

Oopsie, extra line of whitespace. Here's a fix for that.

Updated issue summary.

Updated issue summary.

Clarifications.

Updated issue summary.

Well, the broken test will still fail after the patch. It will just fail without dropping the parent tables. Hmmm. Edit: also, as fun as it might be, I'm not sure I should try to take down a testbot. ;)

A safer (and easier) thing to test would be the other issue (whether tables are properly removed when a module is disabled).

Edit: Actually, we'd need a test environment within a test environment to test that as well? Hmmm.

Tagging for tests based on feedback from catch and chx.

Updated issue summary.

#53 fails locally for me as well. Looking into it.

Alright, when I print debug for the tables matched, I've several prefixes
Table from child simpletest installation was not cleaned up: simpletest370751simpletest167342comment
Table from child simpletest installation was not cleaned up: simpletest370751simpletest240042comment
Table from child simpletest installation was not cleaned up: simpletest370751simpletest573371comment

I've an idea on how to fix this; testing a different solution now.

Edit: Mmm, just realized they have the same child prefix and it's just the grandchild prefix that's different. It also occurs after I clean environment.

So, it appears to me that none of the grandchild sites' tables are deleted by the time our check in testWebTestRunner() gets run. There are tables left from six separate grandchildren at that point--hundreds of them, not just the comment tables.

I used the pattern '/' . $this->databasePrefix . 'simpletest\d+/' to only check for tables from grandchildren from the current test run.

Here's just the fix by itself, rerolled for core/. I want to make sure it passes testbot.

Also unassigning cause I have no idea how to make the test work.

Back to NW for the demon tests.

Right, we still want to allow it to delete mysite_simpletest2342*. The function's a safety net to ensure we don't ever delete mysite*. Part of the bug in this issue is mis-identifying what the "original" prefix is.

I'm confused; manual testing has confirmed that the patch does resolve the two issues originally described, including when the parent site is not prefixed... how do you reproduce the problem described in #67?

With #70 and a non-prefixed parent site database:

• The "Clean enviroment" button works properly.
• The Field API Field info tests do not leave leftover tables.
• The test for the module in #9 fails without nuking the entire database.

Yay!

I had another idea on how to write a test for this, but it may turn out to be another fantasy. We'll see.

:( Well, it was almost awesome.

The attached test fails regardless of whether either #70 or #60 is applied with it. If you check the verbose messages, somehow the semaphore table still gets deleted in the grandchild site, even when the patch is applied. Maybe someone else can figure out why.

And yeah, it's missing docs atm, but that doesn't matter 'cause it doesn't work.

Scenarios this wouldn't cover even if it did work would be the enable/disable case catch was trying to test for earlier (maybe we could do something in tearDown() for that?) and the prefixed db bug with the clean environment button that beejeebus mentions.

It gets set before running the grandchild, here:

+++ b/core/modules/simpletest/simpletest.testundefined
@@ -503,3 +503,59 @@ class SimpleTestMissingDependentModuleUnitTest extends DrupalUnitTestCase {
+    // If this is the child request, run a grandchild test without calling
+    // the parent::setUp() method.
+    else {
+      $this->break_setup = TRUE;
+      // Run this test from web interface.
+      $this->drupalGet('admin/config/development/testing');
+      $edit = array();
+      $edit['SimpleTestBrokenSetUp'] = TRUE;
+      $this->drupalPost(NULL, $edit, t('Run tests'));
+    }

I don't quite understand the second question.

Oh! I see what you mean. That makes me wonder why it's breaking at all, then. I've overlooked something. Hmmm.

Hmm, this doesn't address the problem of the disabled module's tables not being cleaned or the prefix issue beejeebus mentions. However, it's still a happy fix.

Reroll tweaks the failure message a little.

So, can we test it?

I'll double-check the test later today.

#101 - #104 look great! There's a grammatical error in one of the comments and a missing docblock or two, so I'll reroll to fix that.

Regarding the earlier question, "Base table or view not found" was the text of the PDO error when the DB was emptied. This way is better, though.

Attached adds a bit more code documentation. I also switch the if/else logic in the test's setUp() to match the actual sequence of events (first the parent site, then the child).

Ah, good idea. I'll reroll with that.

With chx's suggestion from #107. Note that with the current codebase the two added assertions will pass even without the fix, because the child run of the test dies over the missing DB connection before it goes to the results page. We could try to follow the "continue to the error page" link if it's found, but that seems unnecessary to me. They're just failsafes in case it gets refactored in the future.

Also of note: The reason for not factoring into two parent and child test classes as @bfroehle suggests in #76 is that doing so would expose the child test to the UI, testbot, etc. Nothing would happen because of the drupal_valid_test_ua() check, but it would still be a waste of time since the child behavior should only be run from within the parent. So, we stick with an if/else inside each of the class's methods. Better redundant code than redundant execution. :) (This technique is already used in simpletest's other self-tests.)

Followup issue: #1353516: Tests for: Tables are not deleted when a module is disabled during test execution.

Straight reroll for D7.

#1358746: PDOException: Table 'drupaltestbotmysql.registry' doesn't exist appears to have been caused by this.

So let's see, this would need a change notification for situations where:

1. People have tests that extend DrupalTestCase directly, or otherwise do not extend DrupalWebTestCase or DrupalUnitTestCase.
2. People already have tests doing myCustomSetUpThing() rather than parent::setUp(), which presumably already does not delete all their database tables?

Am I correct here? Those do seem like pretty edge cases. If we do add a change notification we should make it very clear that tests which already extend DrupalWebTestCase or DrupalUnitTestCase are not affected.

Unrelated, it occurs to me after stating that that maybe we should also add additional test coverage for the same issue in DrupalUnitTestCase and a grandchild test class. We could open that as a separate task. I'm not sure they're even actually affected by the bug, but all the more reason to add test coverage.

Writing the change notification.

Change notice added.

Updated issue summary.