Page MenuHomePhabricator

[Task] Use PHPUnit 6 forward compatibility aliases in all Wikibase code bases
Closed, DeclinedPublic

Description

PHPUnit backported namespaced class names from PHPUnit 6 back to the older versions 4 and 5. We can start using these right now. This will make the later migration to PHPUnit 6 quite a lot easier, as the necessary patches will be much smaller.

In contrast to what the changelog states, PHPUnit 4.8.35 introduced these aliases:

  • PHPUnit_Framework_AssertAssert
  • PHPUnit_Framework_BaseTestListenerBaseTestListener
  • PHPUnit_Framework_TestCaseTestCase
  • PHPUnit_Framework_TestListenerTestListener

In addition, PHPUnit 4.8.36 introduced these aliases:

  • PHPUnit_Framework_AssertionFailedErrorAssertionFailedError
  • PHPUnit_Framework_TestTest
  • PHPUnit_Framework_TestSuiteTestSuite

The suggestion is to not care about any of these, but exclusively focus on replacing PHPUnit_Framework_TestCase with TestCase. This is the vast majority of replacements we need to do (about 84% when I count appearances in my local codebase, with the next one being 7% of PHPUnit_Framework_MockObject_MockObject). Focusing on this one class will make reviewing patches easier. All the other updates can be done later when actually updating to PHPUnit 6.

The suggestion is to not replace … extends \PHPUnit_Framework_TestCase with … extends \PHPUnit\Framework\TestCase, but with … extends TestCase and an import in the use section above. This fits better to the code style the Wikidata team is used to, where full qualified class names are avoided.

If a code base requires it's own PHPUnit version (this is the case for all PHP components that are not MediaWiki extensions), the dependency in composer.json needs to be updated like this:

"phpunit/phpunit": "^4.8.35",

List of components that need an update:

Event Timeline

thiemowmde moved this task from incoming to ready to go on the Wikidata board.
This comment was removed by thiemowmde.

Change 414642 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Don’t extend fully qualified class name for TestCase

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

Change 414642 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Don’t extend fully qualified class name for TestCase

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

The suggestion is to not replace … extends \PHPUnit_Framework_TestCase with … extends \PHPUnit\Framework\TestCase, but with … extends TestCase and an import in the use section above. This fits better to the code style the Wikidata team is used to, where full qualified class names are avoided.

Please don't do this until someone fixes the structure test to support this: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/master/tests/phpunit/structure/StructureTest.php#28

WikibaseLexeme already used the non-namespaced version last week: https://gerrit.wikimedia.org/r/410795 – why is this suddenly a problem now? Because WikibaseLexeme isn’t part of some standard build yet?

And where does this “structure test” run? I haven’t heard of it before, and it doesn’t seem part of the CI on each change, otherwise https://gerrit.wikimedia.org/r/414642 couldn’t have been merged.

WikibaseLexeme already used the non-namespaced version last week: https://gerrit.wikimedia.org/r/410795 – why is this suddenly a problem now? Because WikibaseLexeme isn’t part of some standard build yet?

I or no one else noticed I guess. There's some discussion about it on https://gerrit.wikimedia.org/r/#/c/413731/ which is partly why it hasn't been merged yet.

And where does this “structure test” run? I haven’t heard of it before, and it doesn’t seem part of the CI on each change, otherwise https://gerrit.wikimedia.org/r/414642 couldn’t have been merged.

It runs as part of every test run, you can find it in https://integration.wikimedia.org/ci/job/mwext-testextension-php55-composer-jessie/1569/testReport/(root)/ It won't cause CI to fail, it just means that the structure test is no longer checking those files.

It won't cause CI to fail, it just means that the structure test is no longer checking those files.

Okay, I see – the structure test uses this regex to find test classes, then runs tests on them. Thanks!

I’ve uploaded a proposed commit to update StructureTest (I saw your comment just in time to attach the right phabricator ticket to it).

@Legoktm wrote:

Please don't do this until someone fixes the structure test […]

As argued in https://gerrit.wikimedia.org/r/413731 I feel asking people to not do stuff isn't the best approach. The least thing needed would be a PHPCS sniff that enforces all code bases to always use the full qualified \PHPUnit\Framework\TestCase.

I can see the value of StructureTest, but I disagree with it being a technical blocker for said reason. It's not like a code base is somehow degenerating just because StructureTest does not understand it's code style any more.

This looks like it is stalled, and could probably do with being broken down into sub tasks?
@thiemowmde @Lucas_Werkmeister_WMDE thoughts?

Personally I don't think it makes sense to split this into subtasks. Either update all codebases the same way, or none.

I furthermore suggest to close this as "declined" (as this is what the lack of interest by the Wikidata team tells me) and wait till PHPUnit 4 support is actually dropped.