Page MenuHomePhabricator

Wrong definition of MW_ENTRY_POINT when integration tests run.
Open, Needs TriagePublic

Description

Motivation

MW_ENTRY_POINT is defined in many places.

Running integration tests cause the conflict in defining MW_ENTRY_POINT between Setup.php (which loaded directly using require) and Maintenance.php which is lazily loaded by class loader later than Setup.php.

Maintenance.php:

define( 'MW_ENTRY_POINT', 'cli' );

Setup.php:

// Define MW_ENTRY_POINT if it's not already, so that config code can check the
// value without using defined()
if ( !defined( 'MW_ENTRY_POINT' ) ) {
	/**
	 * The entry point, which may be either the script filename without the
	 * file extension, or "cli" for maintenance scripts, or "unknown" for any
	 * entry point that does not set the constant.
	 */
	define( 'MW_ENTRY_POINT', 'unknown' );
}

All above make the very first test marked as 'risky' and might lead to undefined/unexpected behaviour.

FAILURES!
Tests: 42, Assertions: 407, Failures: 1, Risky: 1.
Script phpunit --colors=always --testsuite=core:integration,extensions:integration,skins:integration handling the phpunit:integration event returned with error code 1

Expected result:

Conflict should be avoided and tests shouldn't be marked as risky.

Two options are possible:

  1. Explicitly define the order of loading.
  2. Get rid off the MW_ENTRY_POINT constant.

Note

MW_ENTRY_POINT isn't used anywhere except edwardspec / mediawiki-moderation
https://codesearch.wmflabs.org/search/?q=MW_ENTRY_POINT&i=nope&files=&repos=

Additional info

1) DatabaseSqliteTest::testAddQuotes with data set #0 ('', '''')
This test printed output: 
Notice: Constant MW_ENTRY_POINT already defined in /Users/peter/work/Wiki/gerrit/mediawiki/maintenance/Maintenance.php on line 26

Call Stack:
    0.0169     401160   1. {main}() /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/phpunit:0
    0.0288    2044280   2. PHPUnit\TextUI\Command::main(???) /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/phpunit:61
    0.0288    2044392   3. PHPUnit\TextUI\Command->run(???, ???) /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/TextUI/Command.php:159
    0.0940    6323680   4. PHPUnit\TextUI\TestRunner->doRun(???, ???, ???) /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/TextUI/Command.php:200
    0.1936    9490312   5. PHPUnit\Framework\TestSuite->run(???) /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:621
    0.1985    9490840   6. PHPUnit\Framework\TestSuite->run(???) /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
    0.2002    9491368   7. PHPUnit\Framework\TestSuite->run(???) /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
    0.2812   15764680   8. PHPUnit\Framework\DataProviderTestSuite->run(???) /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
    0.2816   15765208   9. DatabaseSqliteTest->run(???) /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
    1.0107   32098896  10. DatabaseSqliteTest->run(???) /Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/MediaWikiIntegrationTestCase.php:417
    1.0108   32098896  11. PHPUnit\Framework\TestResult->run(???) /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/Framework/TestCase.php:756
    1.0112   32110288  12. DatabaseSqliteTest->runBare() /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/Framework/TestResult.php:691
    1.0120   32129064  13. DatabaseSqliteTest->mediaWikiSetUp() /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/Framework/TestCase.php:1024
    1.0162   32120416  14. spl_autoload_call(???) /Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/MediaWikiIntegrationTestCase.php:547
    1.0162   32120456  15. AutoLoader::autoload(???) /Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/MediaWikiIntegrationTestCase.php:547
    1.0178   32322936  16. require('/Users/peter/work/Wiki/gerrit/mediawiki/maintenance/Maintenance.php') /Users/peter/work/Wiki/gerrit/mediawiki/includes/AutoLoader.php:109
    1.0178   32322936  17. define(???, ???) /Users/peter/work/Wiki/gerrit/mediawiki/maintenance/Maintenance.php:26

Event Timeline

daniel added a subscriber: daniel.Feb 26 2020, 10:25 AM

Quick fix: in Maintenance.php, do:

if ( !defined( 'MW_PHPUNIT_TEST' ) ) {
  define( 'MW_ENTRY_POINT', 'cli' );
}

It's interesting that MW_ENTRY_POINT isn't actually used anywhere. Perhaps it can just be removed. Perhaps ask on wikitech-l.

Anomie added subscribers: kostajh, Anomie.

This doesn't seem to have anything to do with SQLite, you merely found it while running an SQLite test.

CCing @kostajh since this seems more related to the project of making composer phpunit work. If there's a better person to CC or a tag to use, please let me know.

Quick fix: in Maintenance.php, do:

if ( !defined( 'MW_PHPUNIT_TEST' ) ) {
  define( 'MW_ENTRY_POINT', 'cli' );
}

This alone wouldn't be complete, I think. The problem as described is a conflict between Setup.php defining it as "unknown" and Maintenance.php defining it as "cli".

Whatever entry point is calling Setup.php should already have defined the constant to something other than "unknown". We'd probably still need the change to Maintenance.php though (or a refactoring of maintenance scripts in general to separate the script from the entry point, cf. T99268).

It's interesting that MW_ENTRY_POINT isn't actually used anywhere. Perhaps it can just be removed. Perhaps ask on wikitech-l.

MW_ENTRY_POINT was recently added as a general mechanism, intended to be more flexible than individual boolean defines like MW_API and MW_REST_API. It probably shouldn't be removed.

Krinkle added a subscriber: Krinkle.EditedFeb 26 2020, 7:42 PM
1.0112   32110288  12. DatabaseSqliteTest->runBare() /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/Framework/TestResult.php:691
1.0120   32129064  13. DatabaseSqliteTest->mediaWikiSetUp() /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/Framework/TestCase.php:1024

There is no runBare defined in MediaWiki in the mentioned class. If this is part of a work in progress, be sure to add a link to that as well :)

From a quick glance, I think the issue of MW_ENTRY_POINT is a symptom here, and not the cause. It should not be worked around as that would mask or hide the underlying problem - the problem of multiple entry points being executed in a single process, which not supported.

The main phpunit entry point is phpunit.php which itself is a Maintenance script. This means maintenance scripts can be tested as it won't re-include the same entry point to define the class (per require_"once").

The newer composer entry point either can only be for pure non-integration tests, or must itself become a maintenance script. Otherwise MW_ENTRY_POINT will either be undefined (as well as all other setup stuff being missing), or it will attempt to be defined multiple times (as well as all other setup stuff running a second time and/or in an incompatible context).

In general, maintenance scripts are pretty high level and not the sort of thing that can or should be covered by a "unit" unit test. If such lower-level coverage is desired, it would need to be broken up and have the covered logic moved to a regular autoloaded class instead. Including the Maintenance subclass in "unit" context is not meant to work, the same way that including index.php from a unit test isn't meant to work.

There is no runBare defined in MediaWiki in the mentioned class. If this is part of a work in progress, be sure to add a link to that as well :)

This happens on absolutely clean MediaWiki repo.
DatabaseSqliteTest is derived from MediaWikiIntegrationTest. I think that's the reason.

Still Just a few more things:
php ./tests/phpunit/phpunit.php --colors=always --testsuite=core:integration,extensions:integration,skins:integration '--exclude-group' 'Broken,ParserFuzz,Stub' '--group' 'MediaModeration' '--verbose' --configuration ./phpunit.xml.dist

Gives the following warnings:

Notice: Constant MEDIAWIKI already defined in /Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/bootstrap.php on line 49

Notice: Constant MW_PHPUNIT_TEST already defined in /Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/bootstrap.php on line 50

Notice: Constant MW_CONFIG_FILE already defined in /Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/bootstrap.php on line 54

Stacktrace is the same for all cases:

Call Stack:
    0.0020     410440   1. {main}() /Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/phpunit.php:0
    0.0042     664920   2. require('/Users/peter/work/Wiki/gerrit/mediawiki/maintenance/doMaintenance.php') /Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/phpunit.php:130
    0.0663   10780976   3. PHPUnitMaintClass->execute() /Users/peter/work/Wiki/gerrit/mediawiki/maintenance/doMaintenance.php:99
    0.0712   11665000   4. MediaWikiPHPUnitCommand->run() /Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/phpunit.php:71
    0.0712   11665000   5. MediaWikiPHPUnitCommand->handleArguments() /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/TextUI/Command.php:167
    0.0770   12032024   6. MediaWikiPHPUnitCommand->handleBootstrap() /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/TextUI/Command.php:884
    0.0772   12038520   7. PHPUnit\Util\FileLoader::checkAndLoad() /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/TextUI/Command.php:1087
    0.0772   12038808   8. PHPUnit\Util\FileLoader::load() /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/Util/FileLoader.php:47
    0.0773   12051096   9. include_once('/Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/bootstrap.php') /Users/peter/work/Wiki/gerrit/mediawiki/vendor/phpunit/phpunit/src/Util/FileLoader.php:59
    0.0781   12051256  10. define() /Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/bootstrap.php:54

Still Just a few more things:
php ./tests/phpunit/phpunit.php --colors=always --testsuite=core:integration,extensions:integration,skins:integration '--exclude-group' 'Broken,ParserFuzz,Stub' '--group' 'MediaModeration' '--verbose' --configuration ./phpunit.xml.dist

tests/phpunit/phpunit.php is intended to be used with tests/phpunit/suite.xml, not ./phpunit.xml.dist.

phpunit.xml.dist is part of an attempt to use the standard phpunit executable instead of MediaWiki's customized wrapper. As part of that, it moves some logic from tests/phpunit/phpunit.php into tests/phpunit/bootstrap.php instead. It's not surprising that you get duplicate constant definitions when trying to use both together.