Page MenuHomePhabricator

SqlChangeStoreTest::testSaveChange_insert fails due to EntityChange change (call to undefined method setMetadataFromRC())
Closed, ResolvedPublic

Description

1) Warning
The data provider specified for Wikibase\Lib\Tests\Store\Sql\SqlChangeStoreTest::testSaveChange_insert is invalid.
Error: Call to undefined method Wikibase\Lib\Changes\ItemChange::setMetadataFromRC()
/var/www/html/wiki1/extensions/Wikibase/lib/tests/phpunit/Store/Sql/SqlChangeStoreTest.php:58
/var/www/html/wiki1/maintenance/doMaintenance.php:107

This happens because since If17c21e81d, EntityChange no longer has a setMetadataFromRC() method – that method is now in RepoEntityChange (and inherited in RepoItemChange), a Repo-only class.

Event Timeline

This is a relatively simple fix:

diff --git a/lib/tests/phpunit/Store/Sql/SqlChangeStoreTest.php b/lib/tests/phpunit/Store/Sql/SqlChangeStoreTest.php
index 572379afaa..cca72807e1 100644
--- a/lib/tests/phpunit/Store/Sql/SqlChangeStoreTest.php
+++ b/lib/tests/phpunit/Store/Sql/SqlChangeStoreTest.php
@@ -15,6 +15,8 @@
 use Wikibase\Lib\Changes\ItemChange;
 use Wikibase\Lib\Store\Sql\SqlChangeStore;
 use Wikibase\Lib\WikibaseSettings;
+use Wikibase\Repo\Notifications\RepoEntityChange;
+use Wikibase\Repo\Notifications\RepoItemChange;
 
 /**
  * @covers \Wikibase\Lib\Store\Sql\SqlChangeStore
@@ -179,14 +181,15 @@ public function testSaveChange_update() {
 
     private function getEntityChangeFactory() {
         $changeClasses = [
-            Item::ENTITY_TYPE => ItemChange::class,
+            Item::ENTITY_TYPE => RepoItemChange::class,
             // Other types of entities will use EntityChange
         ];
 
         return new EntityChangeFactory(
             new EntityDiffer(),
             new ItemIdParser(),
-            $changeClasses
+            $changeClasses,
+            RepoEntityChange::class
         );
     }

However, that reintroduces a coupling from Lib to Repo, which I don’t think we want. I’m not sure how else to fix this.

  • We could more or less copy the code of setMetadataFromRC() into the test (specifically into saveChangeInsertProvider()), since it’s not a hugely complicated method. But is that a good idea, or does it mean that we’re no longer testing important code?
  • We could move the relevant parts of the test to Repo. But does this make sense as a Repo test?

Upon closer inspection:

  • Both test methods skip themselves if Repo is not enabled.
  • The class under test, SqlChangeStore, hard-codes the wb_changes name, which only exists on Repo wikis.
  • SqlChangeStore is also not used at all from Client or DataAccess – the only such class in the Wikibase\Lib\Store\Sql namespace, if I’m not mistaken. (I didn’t check the Terms subnamespace, only classes directly in Wikibase\Lib\Store\Sql. As far as I can tell, all of them are used in Client and/or DataAccess, but SqlChangeStore is only used in Repo, and in one other Lib test.)

I’m tempted to say that SqlChangeStore and its test should completely move to the Repo. However, in that case I’m not sure what to do with EntityChangeLookupTest, because the test uses SqlChangeStore to write changes to the wb_changes table (so that EntityChangeLookup can then read them back). Would it make sense to move EntityChangeLookupTest to Repo even though the class it tests stays in Lib?

Hah, I wondered if the class had maybe been in Repo before, and in fact the very last commit to SqlChangeStore was Move SqlChangeStore from repo to lib :D

See T255885: Figure out what to do with uses of Repo classes in Lib and Client tests.

I’m tempted to say that SqlChangeStore and its test should completely move to the Repo.

I think I suggested this too a while back but I don't know why we decided against that, probably the EntityChangeLookupTest?

I would be happy with copying the logic of production to test to decouple them and I hope this production code is tested in another place (like EntityChangeTest?) I feel it's more of "mocking" the logic than leaving it untested.

Change 618961 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Fix SqlChangeStoreTest after code rearrangement

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

Change 618961 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Fix SqlChangeStoreTest after code rearrangement

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