Once Wikibase CI is working again (T353957), it might make sense to have a job there that has temporary accounts enabled.
That might be even more effective if we also could manage to get browser and API tests to run there.
Description
Description
Details
Details
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| In Progress | • Niharika | T324492 Temporary accounts - MVP | |||
| Open | None | T326816 Update features for IP Masking | |||
| Open | None | T326908 Update WMDE Engineering-owned products that may be affected by IP Masking | |||
| Resolved | Arian_Bozorg | T351968 Update Wikidata-related extensions for IP Masking | |||
| Open | kostajh | T359043 Enable temp account creation in DevelopmentSettings.php | |||
| Resolved | Lucas_Werkmeister_WMDE | T353961 Consider creating a Wikibase Secondary CI job with temporary accounts enabled | |||
| Open | None | T353957 [SW] Wikibase secondary CI is broken and email sending is broken again (Dec 2023) |
Event Timeline
Comment Actions
T355879: Make PHPUnit, Selenium, and API testing tests pass with temp account feature flag enabled is also relevant, and would handle running CI tests for Wikibase with temp accounts enabled in WMF infrastructure.
Comment Actions
Yeah, that sounds like a good thing to have for browser and API tests (which we currently don’t run in secondary CI, as mentioned in the task description). But apart from that, I think it’s still a good idea to test IP masking in secondary CI too; we already have five secondary CI jobs, so I think we can just change some of them to enable IP masking (rather than add more jobs).
I’ll try out whether the tests even work or not.
Comment Actions
Sounds good.
I’ll try out whether the tests even work or not.
Just as a heads up, I think we are leaning towards enabling temp account creation by default in CI rather than having a second set of jobs (T359043: Enable temp account creation in DevelopmentSettings.php).
Comment Actions
The tests mostly work in secondary CI, and I think I managed to fix all of them, but one of the fixes is in core. SpecialSetSiteLinkTest has two tests that end up creating temporary users, and it also has a addDBDataOnce() method that makes some test edits, which involves creating (non-temporary) test users. This means that the user table ends up in $dbDataOnceTables, and is not cleared between tests, whereas user_autocreate_serial is cleared between tests; as a result, the second test tries to create a temporary user with the same name as the first test, and fails because the user already exists in the non-cleared user table. Suggested fix:
diff --git a/tests/phpunit/MediaWikiIntegrationTestCase.php b/tests/phpunit/MediaWikiIntegrationTestCase.php index 4448f3ebc8..71b15fca92 100644 --- a/tests/phpunit/MediaWikiIntegrationTestCase.php +++ b/tests/phpunit/MediaWikiIntegrationTestCase.php @@ -689,6 +689,15 @@ private function maybeSetupDB(): void { $this->addDBDataOnce(); static::$dbDataOnceTables = ChangedTablesTracker::getTables( $this->db->getDomainID() ); ChangedTablesTracker::stopTracking(); + if ( + in_array( 'user', static::$dbDataOnceTables, true ) + && !in_array( 'user_autocreate_serial', static::$dbDataOnceTables, true ) + ) { + // if the user table will not be cleared between tests, + // also preserve the temp user counter between tests, + // otherwise later tests will try to recreate temp users that already exist and fail + static::$dbDataOnceTables[] = 'user_autocreate_serial'; + } } ChangedTablesTracker::startTracking();
Which is a bit of a shame, because @Daimona removed a bunch of inter-table dependencies like this when introducing ChangedTablesTracker (it used to be something like “if slot_roles is cleared then also clear ip_changes”, yuck), and now we’d be reintroducing something similar again and go behind ChangedTablesTracker’s back, so to speak :/ but I can’t really think of a better solution. Any thoughts?
Comment Actions
To elaborate a bit: we could probably work around this in Wikibase (the easiest would be to move createItems() and addBadgeMatcher() from addDBDataOnce() to setUp(); we could also try to make createItems() use a temp user if temp users are enabled, to make sure that the user_autocreate_serial is also registered as used in $dbDataOnceTables, but that’d be a bit more annoying to implement), but IMHO that would just leave the door open for another test to run into the same problem. I’d prefer to fix it in core instead, for all tests with similar patterns.
Comment Actions
The tests mostly work in secondary CI, and I think I managed to fix all of them
Update: With this commit (incorporating the fixes in Wikibase and the above patch to MediaWiki core), all the secondary CI runs are indeed green \o/ which would also be useful for T359043, I assume.
Comment Actions
Thanks. I added T359043 as a parent task, assuming that this task is also about fixing the CI errors when AutoCreateTempUser is enabled. The three errors I see (log):
13:15:46 8) Wikibase\Repo\Tests\Specials\SpecialSetSiteLinkTest::testExecutePostModifySiteLink 13:15:46 CannotCreateActorException: Cannot create an actor for an IP user when temporary accounts are enabled 13:15:46 13:15:46 /workspace/src/includes/user/ActorStore.php:639 13:15:46 /workspace/src/includes/user/ActorStore.php:416 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:875 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:760 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:626 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:477 13:15:46 /workspace/src/includes/libs/rdbms/database/Database.php:2300 13:15:46 /workspace/src/includes/libs/rdbms/database/DBConnRef.php:119 13:15:46 /workspace/src/includes/libs/rdbms/database/DBConnRef.php:672 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:485 13:15:46 /workspace/src/includes/Storage/PageUpdater.php:1375 13:15:46 /workspace/src/includes/Storage/PageUpdater.php:923 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/WikiPageEntityStore.php:416 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/WikiPageEntityStore.php:275 13:15:46 /workspace/src/extensions/Wikibase/lib/includes/Store/TypeDispatchingEntityStore.php:84 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/EditEntity/StatsdSaveTimeRecordingEntityStore.php:53 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/EditEntity/MediaWikiEditEntity.php:610 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/EditEntity/StatsdSaveTimeRecordingEditEntity.php:77 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/Specials/SpecialWikibaseRepoPage.php:193 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/Specials/SpecialModifyEntity.php:224 13:15:46 /workspace/src/tests/phpunit/includes/specials/SpecialPageExecutor.php:120 13:15:46 /workspace/src/tests/phpunit/includes/specials/SpecialPageExecutor.php:50 13:15:46 /workspace/src/tests/phpunit/includes/specials/SpecialPageTestBase.php:75 13:15:46 /workspace/src/extensions/Wikibase/repo/tests/phpunit/includes/Specials/SpecialSetSiteLinkTest.php:266 13:15:46 phpvfscomposer:///workspace/src/vendor/phpunit/phpunit/phpunit:106 Logs generated by test Show Details 13:15:46 13:15:46 9) Wikibase\Repo\Tests\Specials\SpecialSetSiteLinkTest::testExecutePostRemoveSiteLink 13:15:46 CannotCreateActorException: Cannot create an actor for an IP user when temporary accounts are enabled 13:15:46 13:15:46 /workspace/src/includes/user/ActorStore.php:639 13:15:46 /workspace/src/includes/user/ActorStore.php:416 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:875 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:760 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:626 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:477 13:15:46 /workspace/src/includes/libs/rdbms/database/Database.php:2300 13:15:46 /workspace/src/includes/libs/rdbms/database/DBConnRef.php:119 13:15:46 /workspace/src/includes/libs/rdbms/database/DBConnRef.php:672 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:485 13:15:46 /workspace/src/includes/Storage/PageUpdater.php:1375 13:15:46 /workspace/src/includes/Storage/PageUpdater.php:923 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/WikiPageEntityStore.php:416 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/WikiPageEntityStore.php:275 13:15:46 /workspace/src/extensions/Wikibase/lib/includes/Store/TypeDispatchingEntityStore.php:84 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/EditEntity/StatsdSaveTimeRecordingEntityStore.php:53 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/EditEntity/MediaWikiEditEntity.php:610 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/EditEntity/StatsdSaveTimeRecordingEditEntity.php:77 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/Specials/SpecialWikibaseRepoPage.php:193 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/Specials/SpecialModifyEntity.php:224 13:15:46 /workspace/src/tests/phpunit/includes/specials/SpecialPageExecutor.php:120 13:15:46 /workspace/src/tests/phpunit/includes/specials/SpecialPageExecutor.php:50 13:15:46 /workspace/src/tests/phpunit/includes/specials/SpecialPageTestBase.php:75 13:15:46 /workspace/src/extensions/Wikibase/repo/tests/phpunit/includes/Specials/SpecialSetSiteLinkTest.php:294 13:15:46 phpvfscomposer:///workspace/src/vendor/phpunit/phpunit/phpunit:106 Logs generated by test Show Details 13:15:46 13:15:46 10) Wikibase\Repo\Tests\Store\Sql\WikiPageEntityStoreTest::testUserWasLastToEdit 13:15:46 CannotCreateActorException: Cannot create an actor for an IP user when temporary accounts are enabled 13:15:46 13:15:46 /workspace/src/includes/user/ActorStore.php:639 13:15:46 /workspace/src/includes/user/ActorStore.php:416 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:875 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:760 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:626 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:477 13:15:46 /workspace/src/includes/libs/rdbms/database/Database.php:2300 13:15:46 /workspace/src/includes/libs/rdbms/database/DBConnRef.php:119 13:15:46 /workspace/src/includes/libs/rdbms/database/DBConnRef.php:672 13:15:46 /workspace/src/includes/Revision/RevisionStore.php:485 13:15:46 /workspace/src/includes/Storage/PageUpdater.php:1506 13:15:46 /workspace/src/includes/Storage/PageUpdater.php:925 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/WikiPageEntityStore.php:416 13:15:46 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/WikiPageEntityStore.php:275 13:15:46 /workspace/src/extensions/Wikibase/repo/tests/phpunit/includes/Store/Sql/WikiPageEntityStoreTest.php:483 13:15:46 phpvfscomposer:///workspace/src/vendor/phpunit/phpunit/phpunit:106
And for Selenium:
14:32:33 [0-1] RUNNING in chrome - /repo/tests/selenium/specs/item.js 14:32:41 [0-1] Error in "item.has its label not rendered when linked on a Wikipage" 14:32:41 Evaluation failed: TypeError: Cannot read property 'set' of undefined 14:32:41 at eval (eval at <anonymous> (:3:20), <anonymous>:2:14) 14:32:41 at eval (eval at <anonymous> (:3:20), <anonymous>:3:6) 14:32:41 at __puppeteer_evaluation_script__:3:20 14:32:41 Error: Evaluation failed: TypeError: Cannot read property 'set' of undefined 14:32:41 at eval (eval at <anonymous> (:3:20), <anonymous>:2:14) 14:32:41 at eval (eval at <anonymous> (:3:20), <anonymous>:3:6) 14:32:41 at __puppeteer_evaluation_script__:3:20 14:32:41 at process.processTicksAndRejections (node:internal/process/task_queues:95:5) 14:32:41 at async DevToolsDriver.executeScript (/workspace/src/extensions/Wikibase/node_modules/devtools/build/commands/executeScript.js:39:20) 14:32:41 at async Browser.wrappedCommand (/workspace/src/extensions/Wikibase/node_modules/devtools/build/devtoolsdriver.js:128:26) 14:32:41 at async EntityPage.open (/workspace/src/extensions/Wikibase/node_modules/wdio-wikibase/pageobjects/entity.page.js:8:3) 14:32:41 at async Context.<anonymous> (/workspace/src/extensions/Wikibase/repo/tests/selenium/specs/item.js:88:3) 14:32:41 [0-1] RETRYING in chrome - /repo/tests/selenium/specs/item.js 14:32:42 [0-1] RUNNING in chrome - /repo/tests/selenium/specs/item.js 14:32:51 [0-1] Error in "item.has its label not rendered when linked on a Wikipage" 14:32:51 Evaluation failed: TypeError: Cannot read property 'set' of undefined 14:32:51 at eval (eval at <anonymous> (:3:20), <anonymous>:2:14) 14:32:51 at eval (eval at <anonymous> (:3:20), <anonymous>:3:6) 14:32:51 at __puppeteer_evaluation_script__:3:20 14:32:51 Error: Evaluation failed: TypeError: Cannot read property 'set' of undefined 14:32:51 at eval (eval at <anonymous> (:3:20), <anonymous>:2:14) 14:32:51 at eval (eval at <anonymous> (:3:20), <anonymous>:3:6) 14:32:51 at __puppeteer_evaluation_script__:3:20 14:32:51 at process.processTicksAndRejections (node:internal/process/task_queues:95:5) 14:32:51 at async DevToolsDriver.executeScript (/workspace/src/extensions/Wikibase/node_modules/devtools/build/commands/executeScript.js:39:20) 14:32:51 at async Browser.wrappedCommand (/workspace/src/extensions/Wikibase/node_modules/devtools/build/devtoolsdriver.js:128:26) 14:32:51 at async EntityPage.open (/workspace/src/extensions/Wikibase/node_modules/wdio-wikibase/pageobjects/entity.page.js:8:3) 14:32:51 at async Context.<anonymous> (/workspace/src/extensions/Wikibase/repo/tests/selenium/specs/item.js:88:3) 14:32:51 [0-1] FAILED in chrome - /repo/tests/selenium/specs/item.js (1 retries)
Comment Actions
Yeah, it's not ideal :( Unfortunately, while the addDBDataOnce idea was noble and it definitely helped a lot making the suites faster, I believe its implementation wasn't planned very carefully. For once, the method might have been made static. That does mean not being able to access non-static properties in it, but it would've allowed us to run it as part of the beforeClass hook. This was already clear when addDBDataOnce was being introduced, but nobody noticed that making the method non-static actually made that harder. Then there's the problem of leaving the database in a consistent state. Any "once before class" method needs to be paired with an "once after class" counterpart; this wasn't done for addDBDataOnce. Everyone just relied on tablesUsed to truncate tables (assuming you wouldn't forget about it altogether), and the combination of addDBDataOnce + tablesUsed was also never considered carefully enough. All in all, $dbDataOnceTables is itself a hack; one that comes directly from the flaws of the previous addDBDataOnce implementation.
Sadly, all I had for you is the rant above and not an actual solution :/ Ideally, I think addDBDataOnce should be treated as a "seeding" method. You use it to define the initial state of the database for the test class, and the database is brought back to that state after every test. I'm just not sure how to do that, particularly considering the performance requirements. It would be a larger change. As long as it's just one test that needs the behaviour to change, can we maybe try fixing it in place? I know it's not ideal, but I worry that changing the code in MWIntegrationTestCase may introduce more edge cases and unwanted effects that people may rely on. What about adding something like the following to your addDBDataOnce method?
$dbw->insert( 'user_autocreate_serial', /* random row */ ); $dbw->truncateTable( 'user_autocreate_serial' );
This should add the table to $dbDataOnceTables and hopefully fix your issue. It's ugly, I know... But I'd rather not change this in core unless absolutely necessary...
Comment Actions
Those are the same three I saw, yes.
(Emphasis added.) Heh, I was about to write something similar ^^ but unfortunately, I assume some tests rely on having outer transaction scope, so we probably can’t implement this with a transaction in MediaWikiIntegrationTestCase. It would probably be possible by copying the affected tables around, but that might be prohibitively slow :(
Comment Actions
Change 1008455 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Fix tests when IP Masking is enabled
Comment Actions
Change 1008456 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Test temporary accounts (IP Masking) in secondary CI
Comment Actions
Change 1008455 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Fix tests when IP Masking is enabled
Comment Actions
Change 1008456 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Test temporary accounts (IP Masking) in secondary CI