Page MenuHomePhabricator

Fix unit tests that test methods that use globals
Open, Needs TriagePublic

Description

Unit tests on methods that use global variables are inherently flawed.
This has mostly gone unnoticed, but now with the push to move more globals into a config object, not having the service container in unit tests is proving to be an issue.

Here is an (incomplete) list of unit tests that failed CI because of a premature access to the service container when trying to access config. Note that just because something is on this list doesn't mean that it should (or can) be fixed, or if it can even be moved to config in the first place.

  • CoreParserFunctions::register called by ParserFactoryTest::testCreate
  • CoreTagHooks::register called by ParserFactoryTest::testCreate
  • FauxResponse::setCookie called by FauxResponseTest::testCookie
  • MailAddress.php called by MailAddressTest.php
  • ObjectCache::newFromId called by the AbuseFilter extension test AutoPromoteGroupsHandlerTest::testFactory
  • PoolCounter::factory called by the Translate extension test TranslatorActivityTest due to Statistics/TranslatorActivity.php using PoolCounterWorkViaCallback
  • RequestContext::getRequest called by the CirrusSearch extension test AnalysisConfigBuilderTest::testLanguageAnalysis
  • Title::legalChars called by BadFileLookupTest::testIsBadFile, UserDefTest::testValidate, TitleTest::testLegalChars, and MediaWikiLinkValidatorTest::test
  • ResourceLoaderFileModule::extractBasePaths called by the Cite extension test CiteDataModuleTest::testGetScript, ::testGetDefinitionSummary
  • WebRequest::getIP called by DerivativeRequestTest::testSetIp and the CirrusSearch extension tests ElasticsearchIntermediaryTest::testConcludeRequestTwice, UserTestingEngineTest::testFromProvidedConfig, and VersionTest::testHappyPath, ::testFailure

These also fail due to being indirectly called but I suspect they shouldn't be touched in the first place

  • includes/HookContainer/GlobalHookRegistry.php in GlobalooksRegistryTest.php
  • LoggerFactory::GetProvider and includes/debug/logger/LegacyLogger.php
  • includes/GlobalFunctions.php
  • MWDebug (with MWDebug::setup excluded)
  • includes/profiler/Profiler.php
  • includes/exception/MWExceptionHandler.php

For completeness, here files that should not be touched:

  • Anything checking $wgFullyInitialised
  • Anything setting global variables
  • Bootstrap code (although it's hard to determine what's ran during bootstrapping)

More will be added as they're encountered.

Event Timeline

tchin updated the task description. (Show Details)

Here is an (incomplete) list of unit tests that failed CI because of a premature access to the service container when trying to access config.

Do you have a patch you can point to or example command(s) to reproduce this?

I basically just converted every global $wgGlobal into MediaWikiServices::getInstance()->getMainConfig()->get( 'Global' ) to see what blows up and then backtracked from there.

Here's a patch that contains every global change that doesn't fail CI.