Page MenuHomePhabricator

MediaWikiIntegrationTestCase does not clear tablesUsed before first test
Closed, DuplicatePublic

Description

MediaWikiIntegrationTestCase has some code in its run() method that’s supposed to clear the tables which the test uses before the first test run, added by @daniel in I8c33be7b1b:

if ( $this->oncePerClass() ) {
	$this->setUpSchema( $this->db );
	$this->resetDB( $this->db, $this->tablesUsed );
	$this->addDBDataOnce();
}

However, if you step through this with the debugger, you can see that $this->tablesUsed is actually empty at this point. This is because it’s usually populated in setUp() – but that method is only called when MediaWikiIntegrationTestCase::run() calls parent::run(), which hasn’t happened at this point yet. (This may have been different in an earlier PHPUnit version, I don’t know.) Is there some way to fix this?

Event Timeline

Just hit the same curiosity and it is indeed rather annoying. Used 'truncateTable' directly in the setup, but that's not the right way to go.

I think addDBDataOnce is more fundamentally broken, and shouldn't be used.

But tablesUsed works correctly for addDBData, as long as you mutate it in addDBData. That seems to be the expected use case.

Having tablesUsed be empty in the first call to resetDB is correct -- there are other things which get reset there, so the call isn't a no-op, but the DB should still be clean at that point. (If it's not then you are probably defensive coding against some *other* test case which is leaving crap in the DB, and you should instead fix that other test case not to leave crap around.) You shouldn't mutate the DB in setup, you should do that in addDBData or addDBDataOnce.

In addDBDataOnce you should really have a tablesUsedOnce instance variable which is mutated only by addDBDataOnce. And what's missing is a 'tearDownOnce` which would then call resetDB at the very end of the class on the tablesUsedOnce.

That way every test class starts with a clean DB, and ends with a clean DB.

I stumbled on this problem again just now.

I think addDBDataOnce is more fundamentally broken, and shouldn't be used.

Do we have some task about tracking this and describing what exactly needs to change to solve what problem?

But tablesUsed works correctly for addDBData, as long as you mutate it in addDBData. That seems to be the expected use case.

Maybe someone expects this to be the use case, but it is not what is documented in the only documentation that seems to exist: https://www.mediawiki.org/wiki/Manual:PHP_unit_testing/Writing_unit_tests#Databases

I looked more into \MediaWikiIntegrationTestCase::run() and it seems that actually conceptually, tablesUsed is expected to be set by the time that \MediaWikiIntegrationTestCase::needsDB() is first called in line 415, well before \MediaWikiIntegrationTestCase::addDBData, otherwise the check in line 415 does not make sense.

(If it's not then you are probably defensive coding against some *other* test case which is leaving crap in the DB, and you should instead fix that other test case not to leave crap around.)

Well, yeah. But given that it could be any one of hundreds of tests that leave stuff around, defensive coding to establish isles of cleaner code seems like an established pattern in modernizing legacy code.
But on the note of fixing other leaky tests: Is there an efficient way to take a "snapshot" of the contents to (some tables of) the temporary test database? That would help in finding tests that leave data in the DB after they are done.

For now, I found this to work correctly on multiple levels: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/752639/5 (though I will squash this into the preceding commit.)

That way every test class starts with a clean DB, and ends with a clean DB.

That would seem to be a worthy goal. Is there some way of evaluating how clean the DB is when a new test starts? That way we could track if we're making progress towards that goal.

(If it's not then you are probably defensive coding against some *other* test case which is leaving crap in the DB, and you should instead fix that other test case not to leave crap around.)

Well, yeah. But given that it could be any one of hundreds of tests that leave stuff around, defensive coding to establish isles of cleaner code seems like an established pattern in modernizing legacy code.

Even more importantly: it can change what tables a test is writing code into long after a test has been created. So we can either:

  1. have a standard list of Wikibase tables that are added to usedTables by default, in order to be future-proof, or
  2. adjust tests all over the place if we change when we add a line to another table when saving an entity, or
  3. have every test make sure their tables are clean when starting out

Neither of those is really great.

That would seem to be a worthy goal. Is there some way of evaluating how clean the DB is when a new test starts? That way we could track if we're making progress towards that goal.

FWIW, I am also exploring the opposite default scenario (T297348), where tests only get a clean database if they actually need it. I suspect most tests don't actually need a "clean" database in order to make meaningful assertions, and the time spent on database table structure cloning is not cheap.

Daimona subscribed.

Closing as duplicate because, as Lucas noted in T342301#9030637, if we track all tables that the test wrote to and reset them when the test ends, there's no need to reset anything when the next test starts.