Page MenuHomePhabricator

Wikibase's cache/integration-tests incompatible with PHPUnit 8
Closed, ResolvedPublic

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.

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.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.Jan 23 2020, 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.Jan 23 2020, 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.Feb 17 2020, 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?

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.

Lydia_Pintscher removed Rosalie_WMDE as the assignee of this task.Feb 25 2020, 1:21 PM

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.

This is not merged and fixed, I guess we can't move forward unless upstream merges it and since the project seems to be abandoned (the whole org is abandoned), I wouldn't hold my breath...

See T237164#5896034

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.

Addshore updated the task description. (Show Details)Mar 9 2020, 7:25 PM

See T237164#5896034

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.

I understand but as I mentioned the quote, it's not enough because one of the fixes (there are several problems) is not even merged to master so we can't even pinpoint to a commit (and not wait for a release): https://github.com/php-cache/integration-tests/pull/107

Yup it looks like we need option 1 or 2.
I don't have a strong preference, but we could start maintaining a fork that would benefit others.
Ping @WMDE-leszek

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

which option shall we pursue? Any preference?

which option shall we pursue? Any preference?

I think "Just copy the test code we need and ditch the library."

which option shall we pursue? Any preference?

I think "Just copy the test code we need and ditch the library."

+1

Restricted Application added a project: User-Michael. · View Herald TranscriptJun 22 2020, 2:36 PM

Change 607051 had a related patch set uploaded (by Michael Große; owner: Michael Große):
[mediawiki/extensions/Wikibase@master] Reenable upstream tests for SimpleCacheWithBagOfStuff

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

I think "Just copy the test code we need and ditch the library."

Did that with the version currently in the HEAD of their master branch, but I'm getting test failures:

111) Wikibase\Lib\Tests\SimpleCacheWithBagOStuffTest::testDeleteMultipleInvalidKeys with data set #17 (2)
Failed asserting that exception of type "Psr\SimpleCache\InvalidArgumentException" is thrown.

/app/maintenance/doMaintenance.php:105

Have you seen that before? The tests for invalid keys are in principle in there since the beginning.

Michael changed the task status from Open to Stalled.Jun 22 2020, 4:40 PM
Michael moved this task from in progress to ready to go on the Wikidata board.

The reasons that the tests break is because of patch ebe58f (Ic8d5a66) which was made to fix T245396: SimpleCacheWithBagOStuff shouldnt be so easy to use bad keys with. However, as SimpleCacheWithBagOStuff is not a BagOStuff, but a PSR-16 Simple Cache, so this patch broke its contract and now the tests fail.

To reintroduce the assertions, we first need to fix the root cause that lead to all those production errors. That seems to be T250930: Wikibase receiving ⧼Lang⧽ from uselang parameter and using it everwhere. After proper handling/santizing of lang is implemented, we maybe should also have a look at other potentially strange callers ( T246207: Track down unusual callers to SimpleCacheWithBagOfStuff ).

Afterwards, we can unstall this ticket. A first step could be to reintroduce the assertions as warnings, so that we find and fix possible stray errors, before we actually introduce the exceptions again and mend the contract.

Michael moved this task from ready to go to in progress on the Wikidata board.Jun 23 2020, 11:22 AM
Addshore changed the task status from Stalled to Open.Jun 30 2020, 4:29 PM

unmarking as stalled as this is being worked on

Change 607051 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Reenable upstream tests for SimpleCacheWithBagOfStuff

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/ /607051

Change 572923 abandoned by Addshore:
[mediawiki/extensions/Wikibase@master] Revert "Disable a broken test"

Reason:
Tests re enabled in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/ /607051

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

Is this Resolved?

Is this Resolved?

It should be yes. But I think, I'd like for this ticket to hang around in our review column for another week or so to make sure we are on the lookout in case strange errors are popping up because of it.