Page MenuHomePhabricator

[Epic] Make PHPUnit extension and core, Selenium, and API testing tests pass with temp account feature flag enabled
Closed, ResolvedPublic

Description

See also T359043: Enable temp account creation in CI and local development environments via DevelopmentSettings.php.

Context

Temporary accounts is a new editing paradigm in which anonymous editing results in a temporary account being created automatically for the user. This feature will roll out to testwiki in Q3 or Q4 FY23-24 and then to pilot wikis.

DevelopmentSettings: Enable auto creation of temp users is an example patch for tracking test status of the test pipeline when temp accounts flag is enabled. As we can see, there are failures in PHPUnit, Selenium, and API-testing tests when temp accounts feature flag is enabled.

Proposal

In this epic, we'll:

  • fix test failures seen in PHPUnit, Selenium, QUnit, and API-testing tests when $wgAutoCreateTempUser['enabled'] = true;.
  • Most of the work here is adjusting assumptions made in tests about anonymous editing
  • In other cases, we need to update code that breaks when temporary accounts is enabled

Acceptance criteria

  • CI for MediaWiki core and extensions has temporary accounts enabled
  • PHPUnit, Selenium, and API-testing tests that run as part of the WMF gate and submit are passing when $wgAutoCreateTempUser['enabled'] = true;

Related Objects

StatusSubtypeAssignedTask
Resolvedkostajh
DeclinedNone
ResolvedNiharika
Resolvedkostajh
Resolvedkostajh
Resolvedkostajh
Resolvedkostajh
ResolvedSTran
Resolvedkostajh
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedABran-WMF
ResolvedDreamy_Jazz
DeclinedABran-WMF
ResolvedABran-WMF
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedSecurityDreamy_Jazz
DeclinedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedBUG REPORTDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedMarostegui
ResolvedDreamy_Jazz
ResolvedPRODUCTION ERRORDreamy_Jazz
ResolvedMarostegui
ResolvedLadsgroup
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedBUG REPORTDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedBUG REPORTDreamy_Jazz
ResolvedBUG REPORTDreamy_Jazz
ResolvedDreamy_Jazz
Resolvedkostajh
ResolvedMimurawil
ResolvedSTran
Resolvedkostajh
Resolvedkostajh
Resolvedkostajh
ResolvedJakob_WMDE

Event Timeline

Change 980947 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] [DNM] DevelopmentSettings: Enable auto creation of temp users

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

kostajh renamed this task from MediaWiki unit and integration tests should pass when temp account feature flag is enabled to MediaWiki PHPUnit tests should pass when temp account feature flag is enabled.Jan 25 2024, 12:41 PM
kostajh renamed this task from MediaWiki PHPUnit tests should pass when temp account feature flag is enabled to Create job that runs PHPUnit unit and integration tests with temp account feature flag is enabled.Feb 11 2024, 8:17 PM
kostajh updated the task description. (Show Details)
kostajh renamed this task from Create job that runs PHPUnit unit and integration tests with temp account feature flag is enabled to Create jobs that run PHPUnit, Selenium, and API testing tests with temp account feature flag enabled.Feb 28 2024, 8:37 AM
kostajh updated the task description. (Show Details)
kostajh added a subscriber: hashar.

It sounds like the problem you're trying to solve is you have something merged but not turned on, and you need to run tests for every patch with it turned on.

From the Release-Engineering-Team side, it sounds like what's needed here is:

  1. Create a version of wmf-quibble-core-vendor-mysql-php74-docker with the $wgAutoCreateTempUser['enabled'] = true
  2. Add it as non-voting job

Which repos should this job be run for?

I thought about this task some more, and would like to consider an alternative approach: don't bother creating new temp-account-specific CI jobs and attempting to have CI handle two different anonymous editing paradigms simultaneously. Just set $wgAutoCreateTempUser['enabled'] = true; in DevelopmentSettings.php in the "experimental" section, like we're doing in DevelopmentSettings: Enable auto creation of temp users, get all the tests to pass (we are not that far off from doing so), then, after we've merged that patch, it would mean:

  • MediaWiki uses temp accounts by default when using DevelopmentSettings.php
  • CI only tests temp accounts paradigm for master branch (REL_ branches would be unaffected)
  • Regressions relating to anonymous IP editing would be caught by PHPUnit tests that explicitly switch off temp accounts functionality, or via manual QA / logs review on beta wikis and testwiki / testwiki2

I am leaning toward this way because:

  • Creating duplicate jobs for Selenium/API testing/PHPUnit is going to be time-consuming to do (updating Quibble, building new images, crafting new jobs, deploying a bunch of new jobs) and time-consuming / resource intensive (we already struggle at times with having enough capacity for CI)
  • Most of our existing tests are not badly broken with $wgAutoCreateTempUser['enabled'] = true; which suggests that we don't have a lot of tests targeting anonymous IP editing anyway. From that, I infer that it is probably not going to be missed much in CI if those tests no longer operate in that paradigm.
  • Temp accounts are in the near term future (see also T355880: Decide long term strategy for temporary accounts in MediaWiki core) for WMF. We should not invest substantial time and resources in maintaining a dual anonymous IP editing paradigm in CI, when those time/resources could be better used elsewhere

We talked a bit about the task during our team meeting today.

For setting up the hack you proposed, my uninformed guess is that maybe forks of the job can be made to have environment variable such as MW_AUTO_CREATE_TEMP_USER and then in includes/DevelopmentSettings.php turn on the feature when it is set (by using getenv() ). This way you don't need to patch Quibble nor do you need to alter the Docker images.

The environment variable can be set while forking the Quibble jobs by passing to the docker-run-with-log-cache-src builder an environment map:

- docker-run-with-log-cache-src:
     environment:
          MW_AUTO_CREATE_TEMP_USER: 1

That will be used to craft the command line docker run -e MW_AUTO_CREATE_TEMP_USER=1 and it will end up being passed to Quibble which runs the tests commands inhering the environment. That is not going to be available to the process running under Apache though (for Selenium/Qunit/Api testing) :-\

Instead of blindly running every tests under each condition, it would be nicer if the test framework managed it. That also has the benefit to have the tests run for developers locally without much fiddling. But I guess there is not much helper in PHPUnit to assist in doing so :-/

Most of our existing tests are not badly broken with $wgAutoCreateTempUser['enabled'] = true; which suggests that we don't have a lot of tests targeting anonymous IP editing anyway. From that, I infer that it is probably not going to be missed much in CI if those tests no longer operate in that paradigm.

This makes it seem OK to do this, especially if we ensure that currently breaking tests are still run with temp accounts switched off, rather than updated only to pass for temp users, so this can be true for them:

Regressions relating to anonymous IP editing would be caught by PHPUnit tests that explicitly switch off temp accounts functionality

Most of our existing tests are not badly broken with $wgAutoCreateTempUser['enabled'] = true; which suggests that we don't have a lot of tests targeting anonymous IP editing anyway. From that, I infer that it is probably not going to be missed much in CI if those tests no longer operate in that paradigm.

This makes it seem OK to do this, especially if we ensure that currently breaking tests are still run with temp accounts switched off, rather than updated only to pass for temp users, so this can be true for them:

Regressions relating to anonymous IP editing would be caught by PHPUnit tests that explicitly switch off temp accounts functionality

I've filed T359043: Enable temp account creation in CI and local development environments via DevelopmentSettings.php as the inverse of this task, with T355879#9585434 as the proposed solution.

kostajh renamed this task from Create jobs that run PHPUnit, Selenium, and API testing tests with temp account feature flag enabled to Make PHPUnit, Selenium, and API testing tests pass with temp account feature flag enabled.Mar 5 2024, 10:38 AM

Change 1008827 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/SpamBlacklist@master] tests: Support temp accounts in testSpam()

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

Change 1008827 merged by jenkins-bot:

[mediawiki/extensions/SpamBlacklist@master] tests: Support temp accounts in testSpam()

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

kostajh updated the task description. (Show Details)
kostajh updated the task description. (Show Details)

@hashar, @STran, @Dreamy_Jazz and myself met to discuss this. @hashar prefers that we do not duplicate CI jobs. Because there is fairly little code coverage of anonymous IP editing scenarios (as evidenced by the relatively few failures in this patch), @hashar's recommendation is to proceed with T359043: Enable temp account creation in CI and local development environments via DevelopmentSettings.php. That approach also has the additional benefit of putting temporary accounts in front of developers so we can catch more issues not found by tests in CI, before this feature is in production.

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

[mediawiki/extensions/SpamBlacklist@master] Make SpamBlacklistTest::testSpamEdit pass when temp accounts enabled

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

Change #1027181 merged by jenkins-bot:

[mediawiki/extensions/SpamBlacklist@master] Make SpamBlacklistTest::testSpamEdit pass when temp accounts enabled

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

kostajh renamed this task from Make PHPUnit, Selenium, and API testing tests pass with temp account feature flag enabled to [Epic] Make PHPUnit, Selenium, and API testing tests pass with temp account feature flag enabled.May 22 2024, 7:48 PM
kostajh updated the task description. (Show Details)
kostajh renamed this task from [Epic] Make PHPUnit, Selenium, and API testing tests pass with temp account feature flag enabled to [Epic] Make PHPUnit extension and core, Selenium, and API testing tests pass with temp account feature flag enabled.May 23 2024, 7:11 AM