Page MenuHomePhabricator

Provide an alternative way to use MediaWikiUnitTestCase for tests not in unit/
Closed, DeclinedPublic

Description

Currently, if a class extends MediaWikiUnitTestCase, a structure test ensures that the class is located under tests/phpunit/unit. While that's fine for most of the repos, there are some special cases: Wikibase, to mention a big one.
AFAICS, the alternative chosen until now is to extend PHPUnit\Framework\TestCase, and use MediaWikiCoversValidator and PHPUnit4And6Compat. This is bad, though: first of all because it's unnecessarily longer. Second, because you'd be renouncing to all MediaWikiUnitTestCase features. Third, because it makes it harder to make any change to the used traits; e.g., rename them, or even remove (like we want to do with PHPUnit4And6Compat now).

Hence, I have three ideas:

  1. Add a new class, MediaWikiUnitTestCaseNoStructure, extending MediaWikiUnitTestCase and overriding the structure test with empty body;
  2. Add a new class, MediaWikiUnitTestCaseBase identical to the current UnitTestCase minus the structure test; make MediaWikiUnitTestCase extend it and add the structure test;
  3. Add a method, say protected function shouldTestDirStructure() { return true; }, and override it + return false where necessary.

This should be tackled before making the direct usage of TestCase grow wildly.

Event Timeline

Daimona triaged this task as High priority.Oct 5 2019, 4:15 PM

And this is what I think about my ideas:

Add a new class, MediaWikiUnitTestCaseNoStructure, extending MediaWikiUnitTestCase and overriding the structure test with empty body;

This would be easy to migrate, but overriding with empty body seems weird.

Add a new class, MediaWikiUnitTestCaseBase identical to the current UnitTestCase minus the structure test; make MediaWikiUnitTestCase extend it and add the structure test;

This is also easy to migrate, but adding a new class is probably unnecessary.

Add a method, say protected function shouldTestDirStructure() { return true; }, and override it + return false where necessary.

I vote for this. Migrating is still pretty easy, and this has two advantages:

  • We don't need a new class;
  • The new method takes space; it is easy to spot. And hopefully, it will remind people that upgrading the dir structure is a good thing.

So I'm tentatively pushing a patch with (3.), given that it should be very easy to implement.

Change 541073 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] UnitTestCase: make directory test optional

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

Well, the reason Wikibase doesn't use MediaWikiUnitTestCase is not just the directory, it's lots of factors:

  • Extension registry, mediawiki unit test case relies on the extension using extension.json schema which currently Wikibase doesn't do. We are working on it (TM). Even if you just include the file, the configs don't work and no test work
  • MediaWikiUnitTestCase empties globals for a good reason (like not letting tests depend on the global state, don't write to production database, etc.) but phpunit test case doesn't do that which enables us run Wikibase tests faster while not worrying about the resetting the database (lots of these tests, don't write to the database but somehow they need some global variables.) This is also a code health issue and same as above, we are working on it (TM)

Once these two are fixed, I have a script that moves tests around to make them comply with the new order of the world. So I would not worry about it.

Ah, OK, this makes sense. Then we can wait. As for this task, it can probably be declined, maybe opening a separate one for future development.

Change 541073 abandoned by Daimona Eaytoy:
UnitTestCase: make directory test optional

Reason:
Per phab

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