Page MenuHomePhabricator

Split mediawiki tests into unit and integration tests
Open, LowPublic

Description

Rather than our current setup in which all tests are bundled in a single directory split the unit and integration tests into 2 separate directories.

This would enable us to:

  • Run unit (fast) and integration (slow) tests separately
  • Get a better idea of where we actually stand in regard to unit tests

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

As suggested by @hashar , this ticket could potentially be a nice hackathon project—if not for Barcelona, then for Prague 😉It's still quite relevant and would make developers' life a whole lot easier.

@zeljkofilipin , what's your opinion?

I do think this should be done, if that is your question. I'm not a PHP developer, so I would not be a lot of help. It's also hard for me to estimate if it could be done at the Wikimedia-Hackathon-2019. Since the project doesn't have to be finished there, it could start during the hackathon and continue after it.

TK-999 added a comment.May 6 2019, 2:55 PM

Yeah, that was my question, thanks!

@TK-999 I may be interested to work on this at Hackathon, will you be there?

@kostajh yup, that's right!

I have an idea on how to proceed, it's better to split MediaWikiTestCase class to MediaWikiUnitTestCase and MediaWikiIntegrationTestCase (for transition, MediaWikiTestCase should have both functionalities I guess) and then slowly replace and separate tests we have under phpunit directory. How does that sound?

@Ladsgroup let’s get together on the first day and talk about a strategy. I think this could work but what would go into the MediaWikiUnitTestCase, why not just extend PHPUnit TestCase?

I’m also curious about your thoughts on T89432#5173026

@Ladsgroup let’s get together on the first day and talk about a strategy. I think this could work but what would go into the MediaWikiUnitTestCase, why not just extend PHPUnit TestCase?

Good point. In general, there are three things the current tests might need:

  • Global variables
  • Mediawiki services
  • Database

As a rule of thumb, if it needs database or mediawiki services, it's an integration test. Global variables are bad in general but still used widely, so we need to provide setMwGlobals to unit tests (So turning this to a unit tests would be doable).

I’m also curious about your thoughts on T89432#5173026

Oh, It reminded me of WikibaseLexeme. https://github.com/wikimedia/mediawiki-extensions-WikibaseLexeme/tree/master/tests/phpunit/composer and https://github.com/wikimedia/mediawiki-extensions-WikibaseLexeme/tree/master/tests/phpunit/mediawiki (See https://github.com/wikimedia/mediawiki-extensions-WikibaseLexeme/blob/master/phpunit.xml.dist) We can do similar in core as well.

Global variables are bad in general but still used widely, so we need to provide setMwGlobals to unit tests (So turning this to a unit tests would be doable).

I'm willing to be convinced otherwise, but my first reaction is that we should not support setMwGlobals when we separate out unit tests from integration tests. For one, it makes executing the tests more complicated; as a second point by allowing these to be used in unit tests, we will provide indirect encouragement for them to be added in non-test code.

In my opinion we should leave tests which rely on globals as integration tests – but we should identify which of these tests are for classes that can be revised to remove globals. From a quick look this seems like an example of a class that could be reworked without much effort to not rely on globals, if ORESService is revised to accept a Config object as the second parameter of its constructor?

T89432#5173026

Regarding that, what I'm trying to say here (now that I'm typing on a computer and not my phone :) ) is that I think there are three things that would be nice to do with improving unit testing and PHPUnit in MediaWiki which I'd like to work on at hackathon:

  1. Cleanly separate out unit tests and integration tests into separate directories (tests/phpunit/unit, tests/phpunit/integration), with separate namespaces (\MediaWiki\Tests\Unit, \MediaWiki\Tests\Integration). Integration tests can extend from MediaWikiTestCase and unit tests could extend MediaWikiUnitTest but, my preference is that said class would only contain a single method to load all tests for the suite, just like Drupal core is doing.
  2. Unit tests for core and extensions should be runnable with vendor/bin/phpunit --suite=unit (T90875). This would mean having a bootstrap.php file (see below) that can facilitate running the unit tests without a DB connection.
  3. Unit tests should run without a database connection or MediaWiki installation (T89432). This will require something like this bootstrap.php file in Drupal core for finding and loading tests by namespace and directory.

As a plan for the Hackathon I would think we could get a framework in place to accomplish these points above, then begin porting over some core and extension tests to verify the proof of concept.

I’m also curious about your thoughts on T89432#5173026

Oh, It reminded me of WikibaseLexeme. https://github.com/wikimedia/mediawiki-extensions-WikibaseLexeme/tree/master/tests/phpunit/composer and https://github.com/wikimedia/mediawiki-extensions-WikibaseLexeme/tree/master/tests/phpunit/mediawiki (See https://github.com/wikimedia/mediawiki-extensions-WikibaseLexeme/blob/master/phpunit.xml.dist) We can do similar in core as well.

For what it's worth, those tests in https://github.com/wikimedia/mediawiki-extensions-WikibaseLexeme/tree/master/tests/phpunit/composer are still run via mediawiki's phpunit.php on CI, and as running them without MW has stopped to be practiced on CI a while ago (2018, if not 2017), I am not even sure if all tests in this dir could run without mediawiki.

Ladsgroup added a comment.EditedMay 14 2019, 9:40 AM

I'm willing to be convinced otherwise, but my first reaction is that we should not support setMwGlobals when we separate out unit tests from integration tests. For one, it makes executing the tests more complicated; as a second point by allowing these to be used in unit tests, we will provide indirect encouragement for them to be added in non-test code.

Good point, I'm convinced.

In my opinion we should leave tests which rely on globals as integration tests – but we should identify which of these tests are for classes that can be revised to remove globals. From a quick look this seems like an example of a class that could be reworked without much effort to not rely on globals, if ORESService is revised to accept a Config object as the second parameter of its constructor?

Yeah, it boils down to proper dependency injection.

Regarding that, what I'm trying to say here (now that I'm typing on a computer and not my phone :) ) is that I think there are three things that would be nice to do with improving unit testing and PHPUnit in MediaWiki which I'd like to work on at hackathon:

  1. Cleanly separate out unit tests and integration tests into separate directories (tests/phpunit/unit, tests/phpunit/integration), with separate namespaces (\MediaWiki\Tests\Unit, \MediaWiki\Tests\Integration). Integration tests can extend from MediaWikiTestCase and unit tests could extend MediaWikiUnitTest but, my preference is that said class would only contain a single method to load all tests for the suite, just like Drupal core is doing.

I'm willing to help with that.

  1. Unit tests for core and extensions should be runnable with vendor/bin/phpunit --suite=unit (T90875). This would mean having a bootstrap.php file (see below) that can facilitate running the unit tests without a DB connection.
  2. Unit tests should run without a database connection or MediaWiki installation (T89432). This will require something like this bootstrap.php file in Drupal core for finding and loading tests by namespace and directory.

Indeed they are good idea and should have been done a long time ago but 1 can be done without doing 2 and 3 right now and still it would be a nice improvement (by just substituting MediawikiTestCase with PhpunitTestCase for unit tests) but still running them via mediawiki phpunit test runner (I mean it's a good first step, 2 and 3 can be done later but should be done eventually).

@Ladsgroup @WMDE-leszek @TK-999 (and anyone else interested), how about we meet at 11 AM on Friday to make a poster, in case others want to help? We can make a plan for dividing up the work at that time too.

Change 510225 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Use phpunit TestCase instead of MediawikiTestCase class for unittests

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

Change 510226 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/core@master] Use phpunit TestCase instead of MediawikiTestCase class for unittests

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

Change 511035 had a related patch set uploaded (by Michael Große; owner: Michael Große):
[mediawiki/extensions/Wikibase@master] DNM! Bootstrap and move unittests to their own dir

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

Hey, this is the script to find unittests:

import os
import sys
import re
import subprocess
from subprocess import PIPE

def find_test_files(path):
    files = []
    for r, d, f in os.walk(path):
        for file_name in f:
            if file_name.endswith('Test.php'):
                files.append(os.path.join(r, file_name))

    return files

def check_file(path):
    with open(path, 'r') as f:
        content = f.read()
        old_content = content
    search_result = re.findall(r'(\nclass (\S+) extends \\?MediaWikiTestCase)', content)
    if not search_result:
        return
    with open(path, 'w') as f:
        f.write(
            content.replace(
                search_result[0][0],
                '\nclass {} extends \MediaWikiUnitTestCase'.format(
                    search_result[0][1])).replace('\nuse MediaWikiTestCase;', '')
                        .replace('\n\n\n', '\n\n').replace('\n\n\n', '\n\n'))
    res = subprocess.run(["php", sys.argv[2], path], stdout=PIPE, stderr=PIPE)
    if res.returncode != 0:
        print('It did not work :((')
        with open(path, 'w') as f:
            f.write(old_content)
    else:
        print('IT WORKED!!!')

for f in find_test_files(sys.argv[1]):
    #print('Checking ' + f)
    check_file(f)

You can run the code to change unittests like this:

python unittest_checker.py /var/lib/mediawiki/extensions/Wikibase/tests/phpunit/ /var/lib/mediawiki/tests/phpunit/phpunit.php

The first argument is the directory of tests you want to do, the second one is the phpunit entry point (but be careful, it run integration tests as unit test, it might drop things in your localhost database if you don't protect it properly).

Change 513106 had a related patch set uploaded (by TK-999; owner: TK-999):
[mediawiki/core@master] [WIP/PoC]: Separate MediaWiki unit and integration tests

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

Daimona added a subscriber: Daimona.Jun 2 2019, 2:17 PM

Change 510226 abandoned by Ladsgroup:
Introduce and use MediawikiUnitTestCase instead of MediawikiTestCase class for unittests

Reason:
Suppressed by Iad01033a0548afd4d2a6f2c1ef6fcc9debf72c0d

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

Change 513106 merged by Jforrester:
[mediawiki/core@master] Separate MediaWiki unit and integration tests

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

Change 517550 had a related patch set uploaded (by TK-999; owner: TK-999):
[mediawiki/core@master] Introduce separate unit tests PHPUnit configuration

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

Change 517550 merged by jenkins-bot:
[mediawiki/core@master] Introduce separate unit tests PHPUnit configuration

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

Change 517701 had a related patch set uploaded (by TK-999; owner: TK-999):
[mediawiki/core@master] Move trivially compatible tests to the unit tests suite

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

Change 519165 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] Unit tests: prevent access to global functions and variables

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

Change 519165 merged by jenkins-bot:
[mediawiki/core@master] Define unit and integration test suites

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

kostajh changed the task status from Stalled to Open.Sun, Jun 30, 1:05 AM

Change 517701 merged by jenkins-bot:
[mediawiki/core@master] Move trivially compatible tests to the unit tests suite

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

Change 519166 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[integration/quibble@master] Support running PHPUnit unit tests

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

Change 520193 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Move trivial unit tests to the new structure

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

Change 520212 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/MobileFrontend@master] Migrate unit tests to the new structure

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

Izno added a subscriber: Izno.Tue, Jul 2, 1:12 PM

Change 520905 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] Reorganize testsuites

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

Change 519166 merged by jenkins-bot:
[integration/quibble@master] Support running PHPUnit unit tests

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

Change 521046 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[integration/quibble@master] Fix composer phpunit:unit command

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

Change 521186 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/core@master] Move unit tests, round III

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

Change 521182 had a related patch set uploaded (by Kosta Harlan; owner: Ladsgroup):
[mediawiki/core@master] Move unit tests FormatJsonTest.php to a dedicated file in unit tests

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

Agabi10 added a subscriber: Agabi10.Mon, Jul 8, 6:13 AM

Change 520905 merged by jenkins-bot:
[mediawiki/core@master] Reorganize testsuites

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

Change 521046 merged by jenkins-bot:
[integration/quibble@master] Fix composer phpunit:unit command

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

Change 521182 merged by jenkins-bot:
[mediawiki/core@master] Move unit tests FormatJsonTest.php to a dedicated file in unit tests

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

Change 521264 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] Unset all globals unneeded for unit tests

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

I was wondering whether we should also disable DB access via MediaWikiServices::disableStorageBackend. Although maybe unit tests for the DB class and related could need it?

Change 521264 abandoned by Kosta Harlan:
Unset all globals unneeded for unit tests

Reason:
in favor of I16691fc8ac063705ba0c2bc63b96c4534ca8660b

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

Change 521268 had a related patch set uploaded (by Kosta Harlan; owner: Ladsgroup):
[mediawiki/core@master] Unset all globals unneeded for unit tests

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

Change 520212 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Migrate unit tests to the new structure

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

OK, I think it's time to announce this?

Change 510225 abandoned by Ladsgroup:
Use phpunit TestCase instead of MediawikiTestCase class for unittests

Reason:
Suppressed by Ib4411ca2e12ceecaa258fd0d9f962df6d3316806

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

Change 521477 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] [POC] UnitTests: disable storage backend

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

Change 521515 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[integration/quibble@master] Don't run phpunit-unit stage if the composer script doesn't exist

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

Change 521477 abandoned by Daimona Eaytoy:
[POC] UnitTests: disable storage backend

Reason:
Seems unnecessary

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

Change 521268 merged by jenkins-bot:
[mediawiki/core@master] Unset all globals unneeded for unit tests, assert correct directory

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

Change 521186 merged by jenkins-bot:
[mediawiki/core@master] Move unit tests, round III

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

Change 521908 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] PHPUnit bootstrap: less aggressive unsetting of globals

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

Change 521909 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] Tests: Assert that integration tests are not in unit test directory

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

Change 521909 merged by jenkins-bot:
[mediawiki/core@master] Tests: Assert that integration tests are not in unit test directory

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

Change 521908 merged by jenkins-bot:
[mediawiki/core@master] PHPUnit bootstrap: less aggressive unsetting of globals

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

Testing https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CirrusSearch/+/521955 I found that the extension dependencies are not loaded by the php unit boostrap script.
I'm perhaps running this incorrectly (vendor/bin/phpunit extensions/CirrusSearch/tests/phpunit/unit/ or composer phpunit:unit extensions/CirrusSearch/tests/phpunit/unit/).
Loading extensions/*/vendor/autoload.php makes the tests pass for cirrus.

Testing https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CirrusSearch/+/521955 I found that the extension dependencies are not loaded by the php unit boostrap script.
I'm perhaps running this incorrectly (vendor/bin/phpunit extensions/CirrusSearch/tests/phpunit/unit/ or composer phpunit:unit extensions/CirrusSearch/tests/phpunit/unit/).
Loading extensions/*/vendor/autoload.php makes the tests pass for cirrus.

In general, it's better to combine composer dependencies of extension into core using composer merge plugin: https://www.mediawiki.org/wiki/Composer#Using_composer-merge-plugin

That would fix lots of headaches (in here and lots of other places)

In general, it's better to combine composer dependencies of extension into core using composer merge plugin: https://www.mediawiki.org/wiki/Composer#Using_composer-merge-plugin

Thanks, should this be done by default or will we have instructions to tell developers to tweak their composer.local.json file to run the unit tests?

Change 522506 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[integration/quibble@master] Use vendor/bin/phpunit instead of composer phpunit:unit

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

Change 521515 abandoned by Kosta Harlan:
Don't run phpunit-unit stage if the composer script doesn't exist

Reason:
abandoning in favor of Ib8c5b7679689df089f183eea181b3fcf9eed3d0d

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

Change 522166 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/core@master] Split HttpTest and SessionTest to unit and integration

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

In general, it's better to combine composer dependencies of extension into core using composer merge plugin: https://www.mediawiki.org/wiki/Composer#Using_composer-merge-plugin

Thanks, should this be done by default or will we have instructions to tell developers to tweak their composer.local.json file to run the unit tests?

I think it's tricky to provide defaults in this case since the composer.local.json file is intended to be ignored by VCS by design. So the next best thing would be to document this caveat so people working on extensions with composer dependencies know how to get started.

Change 522618 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/core@master] Load GlobalFunctions.php to tests/phpunit/bootstrap.php

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

Change 522166 merged by jenkins-bot:
[mediawiki/core@master] Split HttpTest and SessionTest to unit and integration

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

Change 522618 merged by jenkins-bot:
[mediawiki/core@master] Load GlobalFunctions.php to tests/phpunit/bootstrap.php

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

Change 521515 restored by Kosta Harlan:
Don't run phpunit-unit stage if the composer script doesn't exist

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

Change 522506 abandoned by Kosta Harlan:
Use vendor/bin/phpunit instead of composer phpunit:unit

Reason:
let's go with https://gerrit.wikimedia.org/r/c/integration/quibble/ /521515 instead

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

Huji awarded a token.Wed, Jul 17, 6:22 PM

Change 521515 merged by jenkins-bot:
[integration/quibble@master] Don't run phpunit-unit stage if the composer script doesn't exist

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