Problem/Motivation
If the setUp()
method of a child class of DrupalWebTestCase
fails to call parent::setUp()
, the test will delete all database tables for the actual site when it is run.
Additionally, the existing code does not clean up tables belonging to modules that were disabled - since those tables do not exist in drupal_get_schema().
Repeatable
Always
Steps to Reproduce
To reproduce, create a new module with the following mymodule.test
and run the "My Module Test":
class MyModuleTestCase extends DrupalWebTestCase {
public static function getInfo() {
return array(
'name' => 'My Module Test',
'description' => 'Test functionality of My Module',
'group' => 'My Module',
);
}
public function setUp() {
// Enable module and dependencies.
//parent::setUp('mymodule');
}
public function testMyModuleFoo() {
}
}
Expected result
Nothing happens, because there is no valid test to run.
Actual result
All tables are dropped from the site's database.
Clearly, the test's code is not valid without the parent::setUp()
. However, we should prevent the result that it deletes all tables if at all possible. Commenting the wrong line of a test file should not result in the loss of all the user's data. The user has absolutely no indication this is going to happen and no reason to suspect it might without an in-depth understanding of how simpletest works.
Proposed resolution
Rather than deleting all tables in the current schema, delete tables with the simpletest database prefix after restoring the host database connection. Patch in #29 implements this change.
Remaining tasks
Patch needs review and probably testing. Things to test:
- A test that disables a module (e.g. Field API -> Field Info Tests). There should be no leftover database tables after a successfully completed test when the patch is applied.
- A test with the problematic code pattern. (See #9 for a tarball of a module with this test.) Do not test this code on a production site! Ensure that this test does not cause the host site's database tables to be dropped when the patch is applied.
- The "clean environment" button after a generic failed test. The leftover simpletest tables should be removed by the operation.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#115 | d7-1212992-115.patch | 6.13 KB | xjm |
#109 | parent-setup-1212992-109-tests-only.patch | 2.78 KB | xjm |
#109 | parent-setup-1212992-109-combined.patch | 6.19 KB | xjm |
#109 | interdiff-106-109.txt | 1.69 KB | xjm |
#106 | parent-setup-1212992-106-test-only.patch | 2.16 KB | xjm |
Comments
Comment #1
catchOuch.
Comment #2
Dave ReidI don't think we can support broken code since it should be possible to skip setUp and tearDown().
Comment #3
catchCan't we just not delete tables that aren't prefixed with simpletest?
Comment #4
xjmJust 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.
Comment #5
xjmAn aside, I see a distinction between "supporting broken code" and "preventing broken code from dropping all database tables." ;)
Comment #6
xjmThe function to clean broken installations does just such a check to only delete tables with the simpletest prefix:
So, we would just need to do something similar in DrupalWebTestCase::tearDown.
I'll try my hand at rolling and testing such a patch.
Comment #7
xjmPatch 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.Comment #8
xjmTagging this for backport to D7. We might also want to check if the issue exists in the D6 contrib module.
Comment #9
xjmThe 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
.Comment #10
seandunaway CreditAttribution: seandunaway commentedReproduced in 7.4.
Console log attached for anyone interested.
Comment #11
catchIf you disable any modules during a test, it looks like the current code will not delete tables owned by those modules - since it's using drupal_get_schema() which only contains tables from enabled modules. So it might be better to use something like simpletest_clean_tables() instead of the schema at all here.
Comment #12
xjmAh, that is a good idea. code reuse++. Edit: I did not find any existing issue open regarding disabled tables not being cleaned.
Comment #13
catchLet's try that.
Comment #14
xjmI 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.
Comment #15
xjmIt 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.
Comment #16
xjmComment #18
xjmHmm, 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.
Comment #19
xjm#16: simpletest-prevent-dropped-tables-1212992-16.patch queued for re-testing.
Comment #20.0
xjmRemove inaccurate information
Comment #21
sunComment #22
catchHmm, passes for me locally again. Re-uploading.
Comment #24
catchMaybe running it a bit later.
Comment #26
sunI'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.
19 days to next Drupal core point release.
Comment #27
xjmHm, would it make sense to factor this into a helper, something like
simpletest_clean_database_by_prefix($prefix)
?Comment #28
xjmHere'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.
Comment #29
xjmOopsie, extra line of whitespace. Here's a fix for that.
Comment #29.0
xjmUpdated issue summary.
Comment #29.1
xjmUpdated issue summary.
Comment #29.2
xjmClarifications.
Comment #29.3
xjmUpdated issue summary.
Comment #30
catchLooks great, while we wouldn't want to commit it, is it worth uploading a before/after patch with a broken test that would cause this to break? Or will that cause test bot troubles too?
Comment #31
xjmWell, 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.
Comment #32
catchMaybe the tests for simpletest itself would allow us to test that, they create a grandchild site inside the child site, which means it might be possible to check the child site.
Comment #33
xjmTagging for tests based on feedback from catch and chx.
Comment #33.0
xjmUpdated issue summary.
Comment #34
catchI'm working on tests for this.
Comment #35
catchHere's a test, fails locally.
Also combining with the patch here - did not check that locally though.
While testing this I managed to trigger fatal errors via typos etc, which of course leaves stale simpletest tables in the db - so we need to consider that when we commit this, local tests will start to fail if there are stale simpletest tables left over in the db from previously failed tests that have not been cleaned up yet. I think that's probably fine but flagging it here. Worst case we can leave the tests in the issue if people think it's a problem.
Comment #36
chx CreditAttribution: chx commentedI am not fond of that preg. + return preg_match('/simpletest\d+.*/', $prefix); -- why is there a .* if there is no ending anchor (dollar sign)? And,
$this->databasePrefix = Database::getConnection()->prefixTables('{simpletest' . mt_rand(1000, 1000000) . '}');
so that \d+ would be a lot better if it'd be \d{4,} 'cos it can't be less than 4 digits. More, why not change the upper bound to 999999 and make it \d{4,6} ?Comment #37
catch@chx, your suggestion to remove .* won't match tables like "simpletest931796simpletest755174url_alias". Also the regular expression pre-exists this patch so isn't really on topic here. Back to needs review since it'd be good for the bot to see this.
Comment #39
catchThe assertIdentical() was generating output too large for the db, added a message to stop this.
Comment #41
catchTest passes for me locally..
Comment #42
catchSee if we can't get some better debug from the test bot with this one.
Comment #44
catchComment #46
catchOK http://qa.drupal.org/pifr/test/186099 is the result, and explains why this is failing on the test bot - it's finding simpletest tables from multiple different simpletest installs - likely from tests that got aborted. I haven't confirmed this yet though.
I can only see a couple of ways around this:
1. Get the simpletest prefix of the grandchild site if that's possible and look only for tables with that prefix so there's no chance of false positives.
2. Just don't commit the tests here to avoid false positives.
Comment #47
catchRealised why this test will fail on the test bot - concurrency, doh!
This one is a lot more targeted so might be better - test only, xjm suggested checking for the double prefix in irc which is way better than trying to diff.
Comment #49
catchWell except for last minute patch edit fail.
Comment #51
catch#49: fail.patch queued for re-testing.
Comment #53
catchOK finally - those are the right fails. Here's the test and patch combined.
Comment #55
xjm#53 fails locally for me as well. Looking into it.
Comment #56
xjmAlright, 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.
Comment #57
xjmSo, 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.Comment #58
bfroehle CreditAttribution: bfroehle commentedIn addition to this revised table dropping plan, what about a simple flag $this->_initialized which is set to TRUE in setUp() and an exception is thrown in cleanUp if it is not set? That would prevent the table dropping issue in the original post.
Comment #59
boombatower CreditAttribution: boombatower commentedWould a drop all tables (optional pattern) method in the database layer be helpful?
Comment #60
xjmHere'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.
Comment #61
xjmBack to NW for the demon tests.
Comment #62
chx CreditAttribution: chx commentedWhile it is possible to test this, the amount of work to do it can't be justfified.
Comment #63
xjmComment #64
Anonymous (not verified) CreditAttribution: Anonymous commentedthis looks good, but i think the regex (which is not introduced by this patch) should be tightened to match the prefix intent. so, in simpletest_valid_prefix(), instead of:
we should do:
otherwise we match table names like 'ohhaisimpeletest1234'.
Comment #65
Anonymous (not verified) CreditAttribution: Anonymous commentedoh wait, my mistake, we concat the simpletest prefix on the old prefix, so we don't want '^'. looking at how hard it would be to use the original prefix in the regex...
Comment #66
xjmRight, 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.
Comment #67
Anonymous (not verified) CreditAttribution: Anonymous commentedthis doesn't work at all when the prefix is not '', you end up with a
simpletest_clean_database_by_prefix --> db_find_tables --> DatabaseSchema::findTables flow ends up passing down something like 'simpletest818561%' to the table name like clause :-(
i think we prolly do need tests :-(
and maybe something like this in tearDown():
where simpletest_is_test_table() looks like:
Comment #68
xjmI'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?
Comment #69
Anonymous (not verified) CreditAttribution: Anonymous commentedanother interesting fact - if the {simpletest} table has a different prefix than the default for a database, then simpletest_clean_database() doesn't work. lol wtf bbq.
Comment #70
Anonymous (not verified) CreditAttribution: Anonymous commentedthis patch works.
Comment #71
xjmWith #70 and a non-prefixed parent site 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.
Comment #72
xjm:( 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.
Comment #73
bfroehle CreditAttribution: bfroehle commentedHow is $this->break_setup ever going to be set by this point?
Wouldn't it be easier to split this into two test cases, one to be run as the parent and one to be run as the child? The parent one should just choose to run the child one?
11 days to next Drupal core point release.
Comment #74
bfroehle CreditAttribution: bfroehle commentedComment #75
xjmIt gets set before running the grandchild, here:
I don't quite understand the second question.
Comment #76
bfroehle CreditAttribution: bfroehle commented@xjm: But the grandchild site is running in a different php process so as far is it is concerned $this->break_setup is never set.
As for the second question, I mean it might be easier to refactor into two tests:
- SimpleTestBrokenSetUpParent
- SimpleTestBrokenSetUpChild
Comment #77
xjmOh! I see what you mean. That makes me wonder why it's breaking at all, then. I've overlooked something. Hmmm.
Comment #78
chx CreditAttribution: chx commented// Remove all prefixed tables. just like that, eh? This needs a ton more comments that's for sure. Like how databasePrefix is a random so can be considered unique and how db_drop_table(preg_replace('/^.*' . $this->databasePrefix . '(.*)$/', '$1', $table)); actually drops every table in any descendant Drupal and also regardless what was the original prefix but it's safe because it's a unique.
Also, wouldn't the code be simpler if you'd pass an optional argument into db_drop_table to be able to drop unprefixed tables? $prefix = TRUE.
Edit: sorry this was about #70 and I have not seen the newer comments.
Comment #79
Anonymous (not verified) CreditAttribution: Anonymous commentedyeah, i give up.
Comment #80
chx CreditAttribution: chx commentedWhat can we say about the state of Drupal if setUp is not called? Not. much.
Comment #81
Anonymous (not verified) CreditAttribution: Anonymous commentedi like it! if we add a comment explaining why we need the flag, i'm happy to RTBC this, and fix the bug i mention in #69 elsewhere.
Comment #82
catchWe'd also need to open a follow-up issue for #1212992-11: Prevent tests from deleting main installation's tables when parent::setUp() is not called, but that's also not as important as the original bug so fine handling that separately.
Comment #84
chx CreditAttribution: chx commentedComment #85
chx CreditAttribution: chx commentedRetry.
Comment #86
Anonymous (not verified) CreditAttribution: Anonymous commentedok, added some comments to the setup variable, no code changes. if the comments are ok, this is RTBC.
Comment #87
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedTypo: destoying -> destroying.
Otherwise the comment looks good :)
Comment #88
Anonymous (not verified) CreditAttribution: Anonymous commenteddestroyed the typo.
Comment #89
Anonymous (not verified) CreditAttribution: Anonymous commentedok. and another typo, teadDown() --> tearDown().
Comment #90
chx CreditAttribution: chx commentedteadDown method.
Comment #91
xjmHmm, 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?
Comment #92
BTMash CreditAttribution: BTMash commentedI've combined the patches from #91 1 and #72 . Tests seem to pass correctly showing the right error messages when looking at the verbose info.
Comment #94
chx CreditAttribution: chx commentedYOU! SHALL! PASS!
Comment #95
chx CreditAttribution: chx commentedComment #96
bfroehle CreditAttribution: bfroehle commentedI don't think the test actually detects the bug. See my previous posts. Maybe we can just commit without the test hunk for now.
Comment #97
chx CreditAttribution: chx commented@bfroehle I think we test the patch because #92 failed.
Comment #98
xjmI'll double-check the test later today.
Comment #99
BTMash CreditAttribution: BTMash commentedI ran and checked through the test; it is running correctly (the verbose message results of the test show it all :))
Comment #100
bfroehle CreditAttribution: bfroehle commentedI see, the addition of this line is what made the test actually work.
Since we never actually set $break_setup to anything except TRUE, the whole thing should just be omitted.
I don't see this text anywhere in Drupal Core, so what are we actually trying to match?
I'll whip up a more concise test soon.
Comment #101
bfroehle CreditAttribution: bfroehle commentedComment #102
chx CreditAttribution: chx commentedOK now I rewrote the test as well. Edit: crosspost as you can see from the patch numbering. Still, I think asserts are good / necessary, no?
Comment #103
bfroehle CreditAttribution: bfroehle commentedYes, I think chx's test in 102 is preferable with the assertions.
Comment #104
Anonymous (not verified) CreditAttribution: Anonymous commentedyeah, i like #102 as well. simple reroll to fix spelling error, 'eist' to 'exist', no other changes.
Comment #105
xjm#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.
Comment #106
xjmAttached 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).
Comment #107
chx CreditAttribution: chx commentedOne more thing (which I realized thanks to the doxygen): the test should add a $this->pass('The test has run') into the test when if (drupal_valid_test_ua()) and one ->pass() into tearDown as well and the parent should assertNoRaw against both. Then we know the test and tearDown indeed did not run.
Comment #108
xjmAh, good idea. I'll reroll with that.
Comment #109
xjmWith 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.)Comment #110
bfroehle CreditAttribution: bfroehle commentedThis all looks really good to me. I think it's ready to go. :)
Comment #111
BTMash CreditAttribution: BTMash commentedThis looks really good to me as well.
Comment #112
chx CreditAttribution: chx commentedWell, then.
Comment #113
xjmFollowup issue: #1353516: Tests for: Tables are not deleted when a module is disabled during test execution.
Comment #114
catchI'm happy with this. The proposed resolution is slightly different to the issue summary (however the report and steps to reproduce are the same). Instead we're looking at wider issues of relying on the schema to clear up tables in #1353516: Tests for: Tables are not deleted when a module is disabled during test execution. Since that follow-up issue is already in place, committed/pushed this to 8.x.
The patch here isn't anything like the ones I originally worked on so I'm fine committing this myself despite having spent some time working on the bug, it's had lots and lots of review and work since then.
Will need a re-roll for 7.x.
Comment #115
xjmStraight reroll for D7.
Comment #116
xjm#1358746: PDOException: Table 'drupaltestbotmysql.registry' doesn't exist appears to have been caused by this.
Comment #117
webchickSo first of all, EEEEE! Thanks for fixing this awful, terrible bug. :)
However, in looking at all the places we need to pepper $this->setup = TRUE, most of them are encapsulated in the base drupal_web_test_case.php file, but the upgrade tests need to patch their setUp() function as well. That leaves me wondering how many contrib/custom tests out there might need to do the same thing, and break because they do not, and whether we should make a change notice for this?
However, I don't think that concern is worth holding up this bug fix; we have a bit of time before next release to polish some if needed.
Committed and pushed to 7.x. Thanks!
Comment #118
chx CreditAttribution: chx commentedI would venture that there are extremely few tests out there that do not call the core setup functions. Update tests, in this regard, relate to other tests as update to normal Drupal.
However, we need to upgrade Simpletest 7.x in contrib , likely, but that's not too hard.
Comment #119
xjmSo let's see, this would need a change notification for situations where:
DrupalTestCase
directly, or otherwise do not extendDrupalWebTestCase
orDrupalUnitTestCase
.myCustomSetUpThing()
rather thanparent::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
orDrupalUnitTestCase
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.Comment #120
xjmWriting the change notification.
Comment #121
xjmChange notice added.
Comment #123
xjmComment #123.0
xjmUpdated issue summary.