Last modified: 2014-01-02 22:59:19 UTC
Since introduction of LBFactoryTest (https://gerrit.wikimedia.org/r/#/c/96469/), phpunit tests are failing when running *all* the tests. Tests pass when running just database group. .A database query error has occurred. Query: SELECT user_id FROM `unittest_user` WHERE user_name = 'UTSysop' LIMIT 1 Function: User::idForName Error: 1146 Table 'testclient.unittest_user' doesn't exist (localhost) I am running with specifying the /tests/phpunit/includes directory and it fails on DifferenceEngine test on addCoreDBData in MediaWikiTestCase. Specifically, it fails on line 411: if ( $user->idForName() == 0 ) { If I skip DifferenceEngineTest, then tests fail on OracleInstallerTest (and if skipped, then fails on JobQueueTest) LBFactoryTest is doing stuff with resetting the singleton, global, etc. Perhaps something is not correctly restored after test or there is another issue.
failure also occurs on jenkins in mediawiki-core-regression-master job (post merge): https://integration.wikimedia.org/ci/job/mediawiki-core-regression-master/3933/console
If I remove the FakeLBFactory::destroyInstance(); line in LBFactoryTest, then the database errors do not occur however the LBFactoryTests do not work properly and fail. I am not quite sure what the correct thing to do here is or what.
Well the call to FakeLBFactory::destroyInstance() is a bit misleading, since FakeLBFactory does not actually have a static method destroyInstance(). In fact, the usage of FakeLBFactory at all is strange, considering all the static methods (i.e., the methods that are being tested), are in the parent class. The function is actually LBFactory::destroyInstance(), which, not surprisingly, causes the database to be shut down. Since the unit test tables are temporary, that explains the error. In the end, the global state is pretty, well, global. Right now there is no way in LBFactory to change the singleton without destroying the existing object (which is quite intended since it *is* singleton). So that leaves us with two options: 1) Change the test so that it only tests LBFactory::getLBFactoryClass() rather than the actual LBFactory::singleton() function; or 2) Remove the test, because why are we testing deprecated features? Also, let me just say I spent a good five minutes trying to figure out why I was seeing "FakeLBFactory" in some places and "LBFactoryFake" in others. Naming the mock class like that was a terrible idea.
doing option #1 seems best. I think it's fine to test that we are handling the deprecation / backwards compatibility correctly. the singleton and all the global stuff makes LBFactory tricky to test but think it can be done without interfering with the singleton.
Change 104473 had a related patch set uploaded by Aude: Avoid interacting with LBFactory singleton in tests https://gerrit.wikimedia.org/r/104473
Change 104473 merged by jenkins-bot: Avoid interacting with LBFactory singleton in tests https://gerrit.wikimedia.org/r/104473
