Page MenuHomePhabricator
Create Task
Maniphest T341210

FlaggedRevs ReviewHandlerTest::testWithMinParams fails in EntitySchema CI
Closed, ResolvedPublic
Actions

Description

Happened in two consecutive main test builds (Gerrit change).

1) ReviewHandlerTest::testWithMinParams
UnexpectedValueException: Flagged revision with ID 19 exists with unexpected fr_page_id, error: duplicate review

/workspace/src/extensions/FlaggedRevs/business/RevisionReviewForm.php:504
/workspace/src/extensions/FlaggedRevs/business/RevisionReviewForm.php:308
/workspace/src/extensions/FlaggedRevs/business/FRGenericSubmitForm.php:192
/workspace/src/extensions/FlaggedRevs/frontend/specialpages/actions/RevisionReview.php:315
/workspace/src/extensions/FlaggedRevs/rest/ReviewHandler.php:25
/workspace/src/includes/Rest/SimpleHandler.php:38
/workspace/src/tests/phpunit/unit/includes/Rest/Handler/HandlerTestTrait.php:164
/workspace/src/extensions/FlaggedRevs/tests/phpunit/ReviewHandlerTest.php:108
/workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:518

Details

SubjectRepoBranchLines +/-
Add flaggedrevs table to $tablesUsed in ReviewHandlerTestmediawiki/extensions/FlaggedRevsREL1_40+4 -3
Add flaggedrevs table to $tablesUsed in ReviewHandlerTestmediawiki/extensions/FlaggedRevsREL1_39+4 -3
Add flaggedrevs table to $tablesUsed in ReviewHandlerTestmediawiki/extensions/FlaggedRevsmaster+4 -3
Revert "Fix ReviewHandlerTest reusing existing test page name"mediawiki/extensions/FlaggedRevsmaster+24 -12
DNM: Empty change to test CImediawiki/extensions/EntitySchemamaster+0 -0
ReviewHandlerTest: Remove getExistingTestPage() argument againmediawiki/extensions/FlaggedRevsmaster+1 -1
Fix page name in test casemediawiki/extensions/FlaggedRevsmaster+1 -1
Customize query in gerrit

Related Objects

Event Timeline

Lucas_Werkmeister_WMDE created this task.Jul 6 2023, 11:34 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 6 2023, 11:34 AM
Lucas_Werkmeister_WMDE added subscribers: awight, thiemowmde.Jul 6 2023, 11:36 AM

Apparently the test was touched yesterday (Fix ReviewHandlerTest reusing existing test page name, T340004), CC @thiemowmde @awight.

Maintenance_bot added a project: Wikidata.Jul 6 2023, 11:45 AM
Restricted Application added a project: [DEPRECATED] wdwb-tech. · View Herald TranscriptJul 6 2023, 11:46 AM
gerritbot added a comment.Jul 7 2023, 10:39 AM

Change 936244 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/FlaggedRevs@master] Fix page name in test case

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

gerritbot added a project: Patch-For-Review.Jul 7 2023, 10:39 AM

Change 936245 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@master] DNM: Empty change to test CI

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

gerritbot added a comment.Jul 7 2023, 10:55 AM

Change 936244 abandoned by Lucas Werkmeister (WMDE):

[mediawiki/extensions/FlaggedRevs@master] Fix page name in test case

Reason:

Doesn’t work; also, on reflection, a title with :: can be valid after all (e.g. https://en.wikipedia.org/w/index.php?title=0::1&redirect=no). The error must be caused by something else then.

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

gerritbot added a comment.Jul 7 2023, 10:56 AM

Change 936114 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/FlaggedRevs@master] Revert "Fix ReviewHandlerTest reusing existing test page name"

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

Lucas_Werkmeister_WMDE added a comment.Jul 7 2023, 11:14 AM
In T341210#8993369, @Lucas_Werkmeister_WMDE wrote:

Apparently the test was touched yesterday (Fix ReviewHandlerTest reusing existing test page name, T340004), CC @thiemowmde @awight.

Reverting that change seems to fix EntitySchema CI (but might break FileImporter CI again so let’s not do it just yet). Any idea what’s going on? I can’t make much sense of the error message.

gerritbot added a comment.Jul 7 2023, 11:21 AM

Change 936252 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/FlaggedRevs@master] ReviewHandlerTest: Remove getExistingTestPage() argument again

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

Lucas_Werkmeister_WMDE added a project: Wikidata Dev Team (Sprint-∞).Jul 7 2023, 11:22 AM

(Pulling into our sprint to reflect that I’ve already spent some time on it.)

Lucas_Werkmeister_WMDE moved this task from Parents/Waiting to Doing on the Wikidata Dev Team (Sprint-∞) board.Jul 7 2023, 11:22 AM
thiemowmde added a comment.Jul 7 2023, 11:39 AM

We have T256296, T340004, and this now, all attacking the probably same issue from different perspectives. I still can't explain it 100%.

As of now, https://gerrit.wikimedia.org/r/936252 sets the page name back to "UTTest". This might indeed "fix" or more precisely hide the error in certain situations, while making it come back in others. We believe what's going on is that the flaggedrevs database table is not properly cleaned up, and later tests find unrelated rows in there that randomly happen to have the same page id again. I'm still working on it.

gerritbot added a comment.Jul 7 2023, 12:40 PM

Change 936264 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/FlaggedRevs@master] Add flaggedrevs table to $tablesUsed in ReviewHandlerTest

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

gerritbot added a comment.Jul 7 2023, 1:11 PM

Change 936252 abandoned by Lucas Werkmeister (WMDE):

[mediawiki/extensions/FlaggedRevs@master] ReviewHandlerTest: Remove getExistingTestPage() argument again

Reason:

doesn’t look like this fixed the error (see PS3 of I9e65befb14); Icf0d13a79a looks much more promising

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

Lucas_Werkmeister_WMDE mentioned this in T341344: Make MediaWikiIntegrationTestCase::getExistingTestPage() default to random title.Jul 7 2023, 1:22 PM
gerritbot added a comment.Jul 7 2023, 1:30 PM

Change 936264 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Add flaggedrevs table to $tablesUsed in ReviewHandlerTest

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

gerritbot added a comment.Jul 7 2023, 1:34 PM

Change 936245 abandoned by Lucas Werkmeister (WMDE):

[mediawiki/extensions/EntitySchema@master] DNM: Empty change to test CI

Reason:

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

gerritbot added a comment.Jul 7 2023, 1:36 PM

Change 936114 abandoned by Lucas Werkmeister (WMDE):

[mediawiki/extensions/FlaggedRevs@master] Revert "Fix ReviewHandlerTest reusing existing test page name"

Reason:

Icf0d13a79a is a better fix

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

Lucas_Werkmeister_WMDE closed this task as Resolved.Jul 7 2023, 1:52 PM
Lucas_Werkmeister_WMDE assigned this task to thiemowmde.
Lucas_Werkmeister_WMDE moved this task from Doing to Our work done on the Wikidata Dev Team (Sprint-∞) board.

Seems to be working on the original EntitySchema change now. Thanks Thiemo!

ReleaseTaggerBot added a project: MW-1.41-notes (1.41.0-wmf.17; 2023-07-11).Jul 7 2023, 2:00 PM
Maintenance_bot removed a project: Patch-For-Review.Jul 7 2023, 2:10 PM
karapayneWMDE moved this task from Our work done to 2023 completed tasks on the Wikidata Dev Team (Sprint-∞) board.Jul 11 2023, 10:15 AM
gerritbot added a comment.Oct 3 2023, 8:13 PM

Change 963045 had a related patch set uploaded (by Umherirrender; author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/FlaggedRevs@REL1_40] Add flaggedrevs table to $tablesUsed in ReviewHandlerTest

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

gerritbot added a project: Patch-For-Review.Oct 3 2023, 8:13 PM
gerritbot added a comment.Oct 3 2023, 8:46 PM

Change 963168 had a related patch set uploaded (by Umherirrender; author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/FlaggedRevs@REL1_39] Add flaggedrevs table to $tablesUsed in ReviewHandlerTest

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

gerritbot added a comment.Oct 4 2023, 8:45 AM

Change 963168 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@REL1_39] Add flaggedrevs table to $tablesUsed in ReviewHandlerTest

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

gerritbot added a comment.Oct 4 2023, 8:45 AM

Change 963045 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@REL1_40] Add flaggedrevs table to $tablesUsed in ReviewHandlerTest

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

Maintenance_bot removed a project: Patch-For-Review.Oct 4 2023, 9:10 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits