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.
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Jdforrester-WMF | T247658 Make Wikimedia Production MediaWiki compatible with PHP 7.4 | |||
| Resolved | Jdforrester-WMF | T233012 Make MediaWiki core compatible with PHP 7.4 | |||
| Resolved | MaxSem | T216734 Drop devDependency on phpunit-mock-objects, which is abandoned and warns against use | |||
| Resolved | None | T192167 Upgrade PHPUnit from 4/6 to 8 | |||
| Resolved | Michael | T237164 Wikibase's cache/integration-tests incompatible with PHPUnit 8 |
Change 547911 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/Wikibase@master] WIP: Use HEAD of cache/integration-tests
Change 551528 had a related patch set uploaded (by MaxSem; owner: Daimona Eaytoy):
[mediawiki/extensions/Wikibase@master] Disable a broken test
Change 551528 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Disable a broken test
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.
Apparently the latest release of this library just had its second anniversary. I think we should drop using it altogether?
Either we could:
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?
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:
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
Change 572904 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use HEAD of cache/integration-tests
Change 572923 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/Wikibase@master] Revert "Disable a broken test"
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.
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
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.
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...
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
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
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.
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.
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
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.