Page MenuHomePhabricator

WBQC_ResultsSource ConstraintsServices not tested in CI for WikibaseQualityConstraints
Closed, ResolvedPublic5 Estimated Story Points

Description

The fact that this service construction is not tested caused an issue with the train T206161.

Even a simple test just calling the getter for the service would have caught this.

We should add one.

Event Timeline

Addshore created this task.Oct 4 2018, 10:18 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 4 2018, 10:18 AM
Addshore triaged this task as Medium priority.Oct 4 2018, 10:19 AM
Addshore moved this task from Incoming to Ready to estimate on the Wikidata-Campsite board.

It’s certainly supposed to be tested, see tests/phpunit/ServicesTest.php.

We should look to see if this test isn't being run.
As far as I can tell from the CI config WikibaseQualityConstraints should be run with Wikibase, which means that the patch that changed the constructor parameters in Wikibase should naveer have been able to get merged if WBQC was not updated :/

Addshore renamed this task from WBQC_ResultsSource ConstraintsServices not tested in WikibaseQualityConstraints to WBQC_ResultsSource ConstraintsServices not tested in CI for WikibaseQualityConstraints.Oct 9 2018, 1:32 PM
Addshore set the point value for this task to 5.Oct 9 2018, 1:35 PM
Addshore raised the priority of this task from Medium to High.Oct 18 2018, 4:16 PM
Tarrow claimed this task.Oct 19 2018, 12:08 PM
Tarrow added a subscriber: Tarrow.

Looks like other things may also not being being properly run:

Locally I get failures like this:

1) WikibaseQuality\ConstraintReport\Tests\Specials\SpecialConstraintReportTest::testExecute with data set "valid input - existing item" ('$id', array(), 'qqx', array(Hamcrest\Core\CombinableMatcher Object (...), Hamcrest\Core\CombinableMatcher Object (...), Hamcrest\Core\CombinableMatcher Object (...), Hamcrest\Core\CombinableMatcher Object (...), Hamcrest\Core\CombinableMatcher Object (...), WMDE\HamcrestHtml\ComplexTagMatcher Object (...), Hamcrest\Core\CombinableMatcher Object (...), Hamcrest\Core\CombinableMatcher Object (...), Hamcrest\Core\CombinableMatcher Object (...), Hamcrest\Core\CombinableMatcher Object (...), Hamcrest\Core\CombinableMatcher Object (...), Hamcrest\Core\CombinableMatcher Object (...)))
Error: Class 'WikibaseQuality\Html\HtmlTableBuilder' not found

/var/www/mediawiki/extensions/WikibaseQualityConstraints/src/Specials/SpecialConstraintReport.php:362
/var/www/mediawiki/extensions/WikibaseQualityConstraints/src/Specials/SpecialConstraintReport.php:269
/var/www/mediawiki/tests/phpunit/includes/specials/SpecialPageExecutor.php:108
/var/www/mediawiki/tests/phpunit/includes/specials/SpecialPageExecutor.php:36
/var/www/mediawiki/tests/phpunit/includes/specials/SpecialPageTestBase.php:70
/var/www/mediawiki/extensions/WikibaseQualityConstraints/tests/phpunit/Specials/SpecialConstraintReportTest.php:153
/var/www/mediawiki/tests/phpunit/MediaWikiTestCase.php:424
/var/www/mediawiki/maintenance/doMaintenance.php:94

which are not occurring on CI.

So we copied over the Html classes from WikibaseQuality into WikibaseQualityConstraints, but didn’t update the imports to refer to the WBQC versions? Sounds like T205063: Merge WikibaseQuality and WikibaseQualityConstraints isn’t done yet, then (or at least we can’t proceed with T205064: Stop loading WikibaseQuality extension on WMF sites yet).

Change 468972 had a related patch set uploaded (by Tarrow; owner: Tarrow):
[mediawiki/extensions/WikibaseQualityConstraints@master] DNM Testing if ServicesTest runs

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

It appears that the SpecialConstraintReportTest failures I get locally do not occur on CI because WikibaseQuality is still being loaded and just the classes aren't missing. Made a new ticket for this at: T207655

not sure why gerrit removed the iteration infinity

It appears to be that the Services tests are running both locally and on CI.

Looking through logstash/kibana it seems that all the errors are coming this sort of path:

#0 /srv/mediawiki/php-1.32.0-wmf.24/extensions/WikibaseQualityConstraints/src/ServiceWiring.php(249): NO_FUNCTION_GIVEN()
#1 /srv/mediawiki/php-1.32.0-wmf.24/includes/services/ServiceContainer.php(431): Closure$WikibaseQuality\ConstraintReport\#14(MediaWiki\MediaWikiServices)
#2 /srv/mediawiki/php-1.32.0-wmf.24/includes/services/ServiceContainer.php(414): MediaWiki\Services\ServiceContainer->createService(string)
#3 /srv/mediawiki/php-1.32.0-wmf.24/extensions/WikibaseQualityConstraints/src/ConstraintsServices.php(43): MediaWiki\Services\ServiceContainer->getService(string)
#4 /srv/mediawiki/php-1.32.0-wmf.24/extensions/WikibaseQualityConstraints/src/ConstraintsServices.php(155): WikibaseQuality\ConstraintReport\ConstraintsServices::getService(MediaWiki\MediaWikiServices, string)
#5 /srv/mediawiki/php-1.32.0-wmf.24/extensions/WikibaseQualityConstraints/src/Api/CheckConstraintsRdf.php(57): WikibaseQuality\ConstraintReport\ConstraintsServices::getResultsSource()
#6 /srv/mediawiki/php-1.32.0-wmf.24/includes/actions/Action.php(109): WikibaseQuality\ConstraintReport\Api\CheckConstraintsRdf::newFromGlobalState(WikiPage, RequestContext)
#7 /srv/mediawiki/php-1.32.0-wmf.24/includes/actions/Action.php(156): Action::factory(string, WikiPage, RequestContext)
#8 /srv/mediawiki/php-1.32.0-wmf.24/includes/MediaWiki.php(155): Action::getActionName(RequestContext)
#9 /srv/mediawiki/php-1.32.0-wmf.24/includes/MediaWiki.php(785): MediaWiki->getAction()
#10 /srv/mediawiki/php-1.32.0-wmf.24/includes/MediaWiki.php(525): MediaWiki->main()
#11 /srv/mediawiki/php-1.32.0-wmf.24/index.php(42): MediaWiki->run()
#12 /srv/mediawiki/w/index.php(3): include(string)
#13 {main}

It's not tested because the erroring codepath is only hit if WBQualityConstraintsCacheCheckConstraintsResults config option is set to true

Ah, I see.

I figure we could default that config variable to true by now, anyways… any reason not to?

Ah, I see.

I figure we could default that config variable to true by now, anyways… any reason not to?

I like the sound of this!

Change 471002 had a related patch set uploaded (by Tarrow; owner: Tarrow):
[mediawiki/extensions/WikibaseQualityConstraints@master] Cache constraints checks by default

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

The change mentioned above was merged now, anything else left to do here? (Production config should be adjusted as well, but that’s T207854.)

Tarrow added a comment.Nov 5 2018, 3:15 PM

The two services that are obviously switched in the service wiring based on globals are ResultSource and SparqlHelper. I've uploaded a patch to at least be sure that the various possible settings are tested.

I wonder if the solution may be to remove the logic from the service wiring and have Factory services that contain the logic for returning the right type.

Change 471742 had a related patch set uploaded (by Tarrow; owner: Tarrow):
[mediawiki/extensions/WikibaseQualityConstraints@master] Test possible global values in ServiceWiring

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

Tarrow added a comment.Nov 5 2018, 3:17 PM

Whoops, we passed like 'ships in the night' (or should that be 'at lunch'). There is a another patch that should make sure that whatever the globals are set we check that the Wiring will result in a service we can use (or a dummy one we'll ignore).

Change 471742 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Test possible global values in ServiceWiring

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

Change 468972 abandoned by Tarrow:
DNM Testing if ServicesTest runs

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