Page MenuHomePhabricator

Wikibase's cache/integration-tests incompatible with PHPUnit 8
Open, MediumPublic

Description

As can be seen in https://integration.wikimedia.org/ci/job/wmf-quibble-core-vendor-mysql-php72-docker/9691/console :

PHP Fatal error:  Declaration of Cache\IntegrationTests\SimpleCacheTest::setUp() must be compatible with PHPUnit\Framework\TestCase::setUp(): void in /workspace/src/vendor/cache/integration-tests/src/SimpleCacheTest.php on line 713

Upstream bug: https://github.com/php-cache/integration-tests/issues/99
The fix appears to be in master, but not released.

Details

Related Gerrit Patches:
mediawiki/extensions/Wikibase : masterRevert "Disable a broken test"
mediawiki/extensions/Wikibase : masterUse HEAD of cache/integration-tests
mediawiki/extensions/Wikibase : masterDisable a broken test
mediawiki/extensions/Wikibase : masterWIP: Use HEAD of cache/integration-tests

Event Timeline

MaxSem created this task.Nov 2 2019, 9:14 PM
Restricted Application added a project: Wikidata. · View Herald TranscriptNov 2 2019, 9:14 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 547911 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/Wikibase@master] WIP: Use HEAD of cache/integration-tests

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

Change 547911 abandoned by MaxSem:
WIP: Use HEAD of cache/integration-tests

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

Change 551528 had a related patch set uploaded (by MaxSem; owner: Daimona Eaytoy):
[mediawiki/extensions/Wikibase@master] Disable a broken test

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

Change 551528 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Disable a broken test

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

Jdforrester-WMF renamed this task from cache/integration-tests incompatible with PHPUnit 8 to Wikibase's cache/integration-tests incompatible with PHPUnit 8.Thu, Jan 23, 4:19 PM
Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Is this going anywhere, or are we just going to leave it disabled forever?

Is this going anywhere, or are we just going to leave it disabled forever?

I guess it's going to sit in the bitrot limbo until a new version is released upstream. Which, in turn, seems implausible, judging from the last commit dates and the open issues/PRs.

Jdforrester-WMF changed the task status from Open to Stalled.Thu, Jan 23, 4:28 PM

Joy.

Disabling this test looks fine for now until upstream releases

Disabling this test looks fine for now until upstream releases

Apparently the latest release of this library just had its second anniversary. I think we should drop using it altogether?

Disabling this test looks fine for now until upstream releases

Apparently the latest release of this library just had its second anniversary. I think we should drop using it altogether?

Either we could:

  • Fork and release our own version with the fix that is already in there?
  • Just copy the test code we need and ditch the library.
  • Point to a specific commit hash in composer.json
Addshore changed the task status from Stalled to Open.Mon, Feb 17, 9:28 AM
Addshore triaged this task as Medium priority.

Option #3 point to a specific commit hash in composer.json
Lets do this one!

Wasn't this what @MaxSem tried already in https://gerrit.wikimedia.org/r/547911?

What's the point of this library anyway? As far as I can tell it's used in a single (!) file: SimpleCacheWithBagOStuffTest. What does it do? What do we loose when we get rid of the library? A bit of coverage maybe? Can't we restore this coverage by writing a few more test cases, without relying on the external library? Or even better, aren't the existing test cases enough to cover the code?

Rosalie_WMDE removed Rosalie_WMDE as the assignee of this task.Tue, Feb 18, 1:04 PM
Rosalie_WMDE claimed this task.
Rosalie_WMDE added a subscriber: Rosalie_WMDE.

Wasn't this what @MaxSem tried already in https://gerrit.wikimedia.org/r/547911?

Yes it looks like it, and that seemed to pass CI

What's the point of this library anyway?

A shared test base and set of tests for a shared interface shared between multiple projects.
It includes tests for all of the basic functionality and spec of the PSR, making sure implementations are doing the right things.

As far as I can tell it's used in a single (!) file: SimpleCacheWithBagOStuffTest.

Yes

What does it do?

Runs more tests

What do we loose when we get rid of the library? A bit of coverage maybe? Can't we restore this coverage by writing a few more test cases, without relying on the external library?

We either:

  • choose to have less coverage
  • copy and paste the test cases into our own code and have the same coverage
  • send time writing other different tests probably resulting in the same or less coverage

Or even better, aren't the existing test cases enough to cover the code?

Probably not, we only test a few very specific cases on top of what is provided by the base tests

Thanks a lot for the detailed response! However, I feel my main point got lost. My question really is what the benefit of an isolated unit test for "being compatible with PSR-16" is? It's not like the class SimpleCacheWithBagOStuff is used in isolation. It's not even used in an external context we don't know anything about but that it expects compatibility with PSR-16. No. This class is used in Wikibase. We know where it is used. My question is: Isn't it enough to have good coverage for all the code that is using this class?

"Coverage" is not about a number. Coverage is about the team having confidence the code they own does not break without a test protecting them. As far as I can see these tests exist.

Change 572904 had a related patch set uploaded (by Rosalie Perside (WMDE); owner: Rosalie Perside (WMDE)):
[mediawiki/extensions/Wikibase@master] Use HEAD of cache/integration-tests

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

Change 572904 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use HEAD of cache/integration-tests

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

Change 572923 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/Wikibase@master] Revert "Disable a broken test"

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

For some reason, cache/integration-tests is not being installed, see https://integration.wikimedia.org/ci/job/wikibase-repo-docker/11815/console. This used to work before the test was disabled.

Also, the library is currently broken: its public interface is using symfony/phpunit-bridge, but it's listed in require-dev instead of require, see https://github.com/php-cache/integration-tests/pull/107. I believe we had discussed this briefly on gerrit at the time, and agreed that disabling the test was the best solution for the time being.

So back when this lib was initially used it was needed to add to the require-dev of core it seems https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/446568/ (done as part of T199440)

It looks like that was removed in https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/552166/ but not linked to this ticket.

So to move forward this would have to be re added, pinned at the specific version too.

And then this starts becoming messier, as outlined in T199440 this is not an ideal situation but apparently the only way we could make it work with the WMF CI.
I'd propose we also restore this require-dev in the core composer.json for now pinned to the same version as in Wikibase, essentially restoring the way things were.

It's not even used in an external context we don't know anything about but that it expects compatibility with PSR-16. No. This class is used in Wikibase. We know where it is used. My question is: Isn't it enough to have good coverage for all the code that is using this class?
"Coverage" is not about a number. Coverage is about the team having confidence the code they own does not break without a test protecting them. As far as I can see these tests exist.

We want to conform to the PSR spec, hence the tests.
If we didn't care about that detail, then we wouldn't be using the PSR instead and would just use bag o stuff directly.
Some more details @ https://doc.wikimedia.org/Wikibase/master/php/adr_0001.html

It looks like that was removed in https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/552166/ but not linked to this ticket.

Ah, thanks, I forgot about that step.

And then this starts becoming messier

+1

I'd propose we also restore this require-dev in the core composer.json for now pinned to the same version as in Wikibase, essentially restoring the way things were.

And +1, let's restore the status quo.