Make MediaWikiIntegrationTestCase::getExistingTestPage() default to random title
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Lucas_Werkmeister_WMDE
	Jul 7 2023, 1:22 PM

Description

Currently, MediaWikiIntegrationTestCase::getExistingTestPage() defaults to the constant title UTPage. This means that tests which don’t properly clear all database tables can have state related to this UTPage page leaking between them, as several tests will all target the same page; such issues can be quite tricky to debug (see T340004 and T341210 for an example where this happened in FlaggedRevs).

To avoid this, I think getExistingTestPage() should choose a random title (it could still start with UTPage as a prefix), like e.g. getNonexistingTestPage() already does ('UTPage-' . rand( 0, 100000 )).

Details

Subject	Repo	Branch	Lines +/-
tests: Make getExistingTestPage default to random title	mediawiki/core	master	+3 -3
phpunit: Randomize and improve default test page names	mediawiki/core	master	+68 -18
Follow-Up 77d4c2c: Make getExistingTestPage() check content type first	mediawiki/core	master	+4 -6
Follow-Up 77d4c2c: Make getExistingTestPage() not throw if it didn't work out for now	mediawiki/core	master	+2 -1
Use explicit title in RevertedTagUpdateIntegrationTest	mediawiki/core	master	+8 -9

Customize query in gerrit

Related Objects

Mentioned Here: T342301: Remove MediaWikiIntegrationTestCase::$tablesUsed in favour of automatic query tracking
T340004: FileImporter extension suite tests fail with UnexpectedValueException from FlaggedRevs
T341210: FlaggedRevs ReviewHandlerTest::testWithMinParams fails in EntitySchema CI

Event Timeline

Lucas_Werkmeister_WMDE created this task.Jul 7 2023, 1:22 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 7 2023, 1:22 PM

A possible counterargument to this is that for correctly implemented tests, this shouldn’t be a problem: if a test is broken, it means the test is failing to clear some database table (if I understand correctly). You could argue that it’s better to let such tests fail so that they can be fixed.

But given how confusing the test failure in T341210 was, and how I was totally unable to spot the real reason, I’m not very convinced that the current behavior really helps with identifying such tests.

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

[mediawiki/core@master] tests: Make getExistingTestPage default to random title

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

gerritbot added a project: Patch-For-Review.Jul 7 2023, 1:26 PM

Note this was not really the root cause for the problem in the linked tickets. That was not because of the title but because of a revision id (e.g. 4) that got "reused" in unexpected ways.

I'm a little worried that a random title might make our test suite slower. Most tests probably don't care that much if a page existed before or not. If they do they should say so. Maybe it's a naming issue? Can we come up with something that's better than getExistingTestPage and maybe makes the difference between "always the same page" vs. "you have to specify a page name" more visible?

In T341344#8997365, @thiemowmde wrote:

Note this was not really the root cause for the problem in the linked tickets. That was not because of the title but because of a revision id (e.g. 4) that got "reused" in unexpected ways.

Ah okay, I missed that part.

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

[mediawiki/core@master] Use explicit title in RevertedTagUpdateIntegrationTest

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

Change 936282 merged by jenkins-bot:

[mediawiki/core@master] Use explicit title in RevertedTagUpdateIntegrationTest

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

ReleaseTaggerBot added a project: MW-1.41-notes (1.41.0-wmf.17; 2023-07-11).Jul 7 2023, 4:00 PM

I'm not sure if I like the original proposal as written here. The reason why getExistingTestPage defaults to a fixed title is to avoid unnecessarily creating a new page unless there's a good reason to. For every test in the Database group, the page UTPage is always created by the test framework, meaning any database test that needs an existing page has it for free without doing anything special. Now, we could argue whether creating a page for every test class is a good idea; maybe not. But as long as we do that, I think the title used by getExistingTestPage should match the one used in addCoreDBData.

That said, making it something other than "UTPage", and possibly randomized, would help with the tests that use this hardcoded value (search), which might be very surprising if you don't know what UTPage actually is.

Also, IIUC, the root cause of the two failures linked in the task description was a test forgetting to set tablesUsed. I've proposed an alternative solution to that in T342301.

Daimona claimed this task.Aug 9 2023, 3:45 PM

Change 947378 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] phpunit: Randomize and improve default test page names

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

Change 947378 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Randomize and improve default test page names

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

Test page names now default to the name of the caller method, and also have a random component for good measure.

Change 949192 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/core@master] Follow-Up 77d4c2c: Make getExistingTestPage() check content type first

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

gerritbot added a project: Patch-For-Review.Aug 16 2023, 4:36 AM

Change 949401 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/core@master] Follow-Up 77d4c2c: Make getExistingTestPage() not throw if it didn't work out for now

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

Change 949401 abandoned by Jforrester:

[mediawiki/core@master] Follow-Up 77d4c2c: Make getExistingTestPage() not throw if it didn't work out for now

Reason:

We're going with Ia381fb729e1da6af90b8c3165fa072dfe5675b17 instead.

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

Change 949192 merged by jenkins-bot:

[mediawiki/core@master] Follow-Up 77d4c2c: Make getExistingTestPage() check content type first

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

ReleaseTaggerBot edited projects, added MW-1.41-notes (1.41.0-wmf.25; 2023-09-05); removed MW-1.41-notes (1.41.0-wmf.17; 2023-07-11).Aug 29 2023, 5:10 PM

Change 936275 abandoned by Lucas Werkmeister (WMDE):