Page MenuHomePhabricator

Make MediaWikiTestCase::teardownTestDB work reliably
Closed, ResolvedPublic

Description

This doesn't cause any build failures, but it looks rather confusing.

At the end of most PHPUnit jobs, the following is printed:

Time: 5.94 minutes, Memory: 1153.67MB

OK, but incomplete, skipped, or risky tests!
Tests: 20182, Assertions: 154688, Skipped: 112.

Warning: Destructor threw an object exception: exception 'Wikimedia\Rdbms\DBAccessError' with message 'Database access has been disabled.' in /workspace/src/includes/libs/rdbms/loadbalancer/LoadBalancer.php:1108
Stack trace:
#0 /workspace/src/includes/libs/rdbms/loadbalancer/LoadBalancer.php(933): Wikimedia\Rdbms\LoadBalancer->reallyOpenConnection()
#1 /workspace/src/includes/libs/rdbms/loadbalancer/LoadBalancer.php(881): Wikimedia\Rdbms\LoadBalancer->openLocalConnection()
#2 /workspace/src/includes/libs/rdbms/loadbalancer/LoadBalancer.php(757): Wikimedia\Rdbms\LoadBalancer->openConnection()
#3 /workspace/src/includes/GlobalFunctions.php(2655): Wikimedia\Rdbms\LoadBalancer->getConnection()
#4 /workspace/src/extensions/CentralAuth/includes/CentralAuthHooks.php(1518): wfGetDB()
#5 /workspace/src/includes/Hooks.php(174): CentralAuthHooks::onUnitTestsBeforeDatabaseTeardown()
#6 /workspace/src/includes/Hooks.php(202): Hooks::callHook()
#7 /workspace/src/tests/phpunit/MediaWikiTestCase.php(1359): Hooks::run()
#8 /workspace/src/tests/phpunit/bootstrap.php(20): MediaWikiTestCase::teardownTestDB()
#9 (): MediaWikiPHPUnitBootstrap->__destruct()

The wrapper that MediaWiki uses around PHPUnit uses the __destruct interface to run code after PHPUnit is done (but before the program actually exits with the exit status code that PHPUnit decided on).

This is done that way because PHPUnit will call exit(), hence we cannot normally run code "after" the last test. There is also no built-in method for that purpose within the PHPUnit interface (actually, PHPUnit 8 does, but we're still on PHPUnit 4 + 6).

The problem with __destruct is that it doesn't run at a deterministic time. In the case of the above example, it ran after the database objects were already garbage collected, which means trying to access it will actually create a fresh state, and leads to a post-shutdown fatal error, which weirdly, isn't actually fatal.

This task is to find a different way to run MediaWikiTestCase::teardownTestDB. It should:

  • Run exactly once, after the last test (regardless of whether tests passed or failed).
  • Actually work. – E.g. run before the PHP engine state starts getting garbage collected, as currently it sometimes runs after that, and thus produce a fatal error as shown above.

Event Timeline

Change 500109 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] phpunit: Call 'teardownTestDB' from shutdown instead of destruct.

https://gerrit.wikimedia.org/r/500109

Change 500109 merged by jenkins-bot:
[mediawiki/core@master] phpunit: Call 'teardownTestDB' from shutdown instead of destruct.

https://gerrit.wikimedia.org/r/500109

The code is now no longer failing due to the DB objects being dereferenced before the code runs, which is great.

Now that it runs at-shutdown instead of post-shutdown, it's actually trying to do something and getting further along.

But, it seems that it is actually broken underneath that in another way:

Time: 5.68 seconds, Memory: 136.00MB

OK (1447 tests, 23076 assertions)

PHP Fatal error:  Uncaught Wikimedia\Rdbms\DBUnexpectedError: Foreign domain connections are still in use (wikidb). in /workspace/src/includes/libs/rdbms/loadbalancer/LoadBalancer.php:1948
Stack trace:
#0 /workspace/src/includes/libs/rdbms/lbfactory/LBFactory.php(642): Wikimedia\Rdbms\LoadBalancer->setLocalDomainPrefix('')
#1 /workspace/src/includes/libs/rdbms/lbfactory/LBFactorySimple.php(145): Wikimedia\Rdbms\LBFactory->Wikimedia\Rdbms\{closure}(Object(Wikimedia\Rdbms\LoadBalancer))
#2 /workspace/src/includes/libs/rdbms/lbfactory/LBFactory.php(643): Wikimedia\Rdbms\LBFactorySimple->forEachLB(Object(Closure))
#3 /workspace/src/includes/db/CloneDatabase.php(140): Wikimedia\Rdbms\LBFactory->setLocalDomainPrefix('')
#4 /workspace/src/tests/phpunit/MediaWikiTestCase.php(1367): CloneDatabase::changePrefix('')
#5 /workspace/src/tests/phpunit/bootstrap.php(30): MediaWikiTestCase::teardownTestDB()
#6 [internal function]: PHPUnit\Util\Fileloader::{closure}()
#7 {main}
  thrown in /workspace/src/includes/libs/rdbms/loadbalancer/LoadBalancer.php on line 1948

----
 returned non-zero exit status 255

It seems the teardown is unable to restore the database prefix because of other state in the LoadBalancer object not allowing that operation yet. So probably in MediaWikiTestCase::teardownTestDB it needs to do something else first?

Change 501677 had a related patch set uploaded (by Krinkle; owner: Aaron Schulz):
[mediawiki/core@master] Make teardownTestDB() close any dangling connections before changing the prefix

https://gerrit.wikimedia.org/r/501677

Change 501677 merged by jenkins-bot:
[mediawiki/core@master] Make teardownTestDB() close any dangling connections before changing the prefix

https://gerrit.wikimedia.org/r/501677

Can't reproduce after this. Calling it fixed… for now.

(actually, PHPUnit 8 does, but we're still on PHPUnit 4 + 6).

I was looking at whether this can be implemented using the AfterLastTestHook now. The answer is yes, but there's an important difference with shutdown: it won't run if a test calls exit, or terminates PHP in a similar way. Is this acceptable?