Page MenuHomePhabricator

DiscussionTools CI failing due to CheckUser test failure
Closed, ResolvedPublic

Description

DiscussionTools CI tests are failing due to a test written in the CheckUser extension failing. Examples:

The failing test compares the HTML output to HTML output generated in the data provider. This comparison aims to check that the CheckUser extension generates the correct action text (and it's HTML) when displaying a log entry. However, the URL that is used in the link is different between the data provider and the one generated in the code under test. Both are generated by a call to LogFormatter::getActionText which should return the same HTML and does when running CI for CheckUser. However, this seems to be an incorrect assumption for the DiscussionTools repository.

The HTML differs in the URL structure of the link and therefore I suspect that the DiscussionTools extension / it's tests have a modified URL structure such that the tests run fine for the CheckUser extension but fail when on the DiscussionTools extension due to the order in which they are run. The fix for this should be easily done by generating the expected HTML in the actual test and not the data provider, such that the change in URL structure applies to the expected and actual action text.

Event Timeline

Change 958061 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CheckUser@master] Generate expected action text from LogFormatter::getActionText

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

Dreamy_Jazz triaged this task as Unbreak Now! priority.Sep 17 2023, 8:00 PM
Dreamy_Jazz lowered the priority of this task from Unbreak Now! to High.EditedSep 17 2023, 8:03 PM

DiscussionTools isn't in the Wikimedia "gate" and this doesn't fail on extensions in the gate from what I can see, so triaged this as High.

Change 958061 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Get expected action text in test method of CheckUserGetEditsPagerTest

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

Fix to the CheckUser tests seems to have worked, so closing this as resolved. Thanks for the quick merge @DLynch.