Write integration tests for cron job updatePageTriageQueue.php
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Novem_Linguae
	Mar 8 2023, 5:01 PM

Description

This important bi-daily cleanup script located at updatePageTriageQueue.php is pretty much unmonitored and hard to manually test. Any bugs in it that take down the entire cleanup script (such as T331412) will likely not be discovered until a couple months later when a developer finally notices the various PageTriage queues are not getting cleaned out.

Suggested strategy

extend PageTriageTestCase, which extends IntegrationTestCase
look at /tests/phpunit/MaintenancePopulateDraftQueueTest.php for inspiration. that is a similar integration test of a maintenance script
use $this->setMwGlobals() to set as many globals as needed to match enwiki

This would also make refactoring and bug fixing in updatePageTriageQueue.php safer. For example, T321982

Details

	Subject	Repo	Branch	Lines +/-
	Change Database::select to new query builder	mediawiki/extensions/PageTriage	master	+981 -581

Customize query in gerrit

Related Objects

Mentioned Here: T285896: Ingest logs from scheduled maintenance scripts at WMF in Logstash
T321982: Unexpected namespaces in pagetriage_page table
T331412: InvalidArgumentException from line 234 of /srv/mediawiki/php-1.40.0-wmf.25/includes/libs/rdbms/platform/SQLPlatform.php: Wikimedia\Rdbms\Platform\SQLPlatform::makeList: empty input for field page_namespace

Event Timeline

Novem_Linguae created this task.Mar 8 2023, 5:01 PM

Restricted Application added a project: Growth-Team. · View Herald TranscriptMar 8 2023, 5:01 PM

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Novem_Linguae updated the task description. (Show Details)Mar 8 2023, 5:02 PM

Novem_Linguae moved this task from Backlog to Priority technical debt on the PageTriage board.Mar 8 2023, 5:13 PM

We should probably finally get T285896: Ingest logs from scheduled maintenance scripts at WMF in Logstash done, that's a more reliable and less resource-intensive way of catching production errors in maintenance scripts.

Integration tests are still useful for local development, though.

If I'm reading it right, an integration test would have probably caught the bug in T331412 during code review by Verified -1ing the patch. It was a simple PHP InvalidArgumentException. Plus I need to write an integration test to safely refactor (gerrit 865581). So that's my idea behind this ticket.

In T331548#8684536, @Novem_Linguae wrote:

If I'm reading it right, an integration test would have probably caught the bug in T331412 during code review by Verified -1ing the patch. It was a simple PHP InvalidArgumentException.

It depends, the exception was triggered by $secondaryNamespaces being empty. In general, it's hard to include all conceivable conditions in an integration test; and a scheduled maintenance script failing in production is typically not a big deal (not having the setup to detect that it is failing *is* a big deal, but that's a different issue). But yeah, it's definitely helpful for refactoring.

Novem_Linguae updated the task description. (Show Details)Apr 11 2023, 8:27 PM

Change 911959 had a related patch set uploaded (by Scardenasmolinar; author: Scardenasmolinar):

[mediawiki/extensions/PageTriage@master] Change Database::select to new query builder

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

gerritbot added a project: Patch-For-Review.Jun 14 2023, 2:04 AM

Scardenasmolinar changed the task status from Open to In Progress.Jun 14 2023, 2:05 AM

Scardenasmolinar claimed this task.

Scardenasmolinar added a project: Moderator-Tools-Team (Kanban).

Scardenasmolinar moved this task from Ready to Code review on the Moderator-Tools-Team (Kanban) board.

jsn.sherman moved this task from Code review to Done on the Moderator-Tools-Team (Kanban) board.Jun 14 2023, 2:13 PM

Change 911959 merged by jenkins-bot: