LBFactoryTest causes test failures when running all tests
Closed, ResolvedPublic
Actions

Assigned To

None

Authored By

	aude
	Dec 29 2013, 7:59 PM

Description

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.

Version: unspecified
Severity: normal

Details

Reference: bz59105

Event Timeline

• bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 2:35 AM

• bzimport added a project: MediaWiki-Core-Tests.

• bzimport set Reference to bz59105.

• bzimport added a subscriber: Unknown Object (MLST).

aude created this task.Dec 29 2013, 7:59 PM

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:

Change the test so that it only tests LBFactory::getLBFactoryClass() rather than the actual LBFactory::singleton() function; or
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

LBFactoryTest causes test failures when running *all* testsClosed, ResolvedPublicActions

Description

Details

Event Timeline

LBFactoryTest causes test failures when running all tests
Closed, ResolvedPublic
Actions