Page MenuHomePhabricator

ConfirmEdit hCaptcha: Get QUnit tests to run in CI
Closed, ResolvedPublic

Description

Summary

The ConfirmEdit (CAPTCHA extension) extension has QUnit tests for hCaptcha specific code that are not run in CI because the hCaptcha sub-extension is not loaded. We should either load the sub-extension or move the tests out of the sub-extension

Background

Acceptance criteria

  • The hCaptcha QUnit tests run in CI through any acceptable approach to get there

Event Timeline

As you have mentioned in chat, that sounds very similar to the Wikibase git project which hosts both the Wikibase client and the Wikibase repository (and who knows what else). From a CI point of view, I would like to not see this pattern used anywhere else, it is an operational challenge :-(

Then we have indeed multiple extensions defined in the same repository:

extension.json
FancyCaptcha/extension.json
hCaptcha/extension.json
QuestyCaptcha/extension.json
ReCaptchaNoCaptcha/extension.json
Turnstile/extension.json

Each of them have the extension.json, a COPYING file and some i18n file. Can we find a way to have all those merged?

For the QUnit use case, I imagine that while on CI it would be able to read the RL modules from each of the extension.json and dynamically merge them in. Then it is probably better to prioritize T398193 and get rid of the sub extensions. On CI all modules would be loaded and thus tested, for the end users some new $wgConfirmEditCaptchaEngine would let them switch between the different system and we could then have the ResourceLoaderRegisterModulesHook to register the resources for that engine?

Can we find a way to have all those merged?

This is described in T398193: ConfirmEdit: Refactor captcha implementations to not use sub-extensions

However, we are under a time pressure to get Bot detection and mitigation (WE4.2 hCaptcha editing trial) out and doing T398193: ConfirmEdit: Refactor captcha implementations to not use sub-extensions has some complications that will make it a much lengthier process:

  • We will need to conditionally register the ResourceLoader modules (so we don't register modules that are not used)
    • This should be fairly easy to do, so this isn't a problem
  • The i18n also needs to be conditionally loaded, otherwise i18n about reCAPTCHA would be present on WMF wikis (which is likely to cause confusion as to whether the captcha has been enabled)
    • Places that conditionally add i18n files are in places where global $wg... is being used which indicates that this needs to be set before config is loaded (such as update.php)
    • There is a hook that allows code to indicate what captcha instance to use for an action, so we cannot be sure about the full list of defined captchas in the lifecycle of the request before we have the ability to run hooks (which AFAIK is after config has been finalised)
    • Therefore, this seems like a catch-22 situation and so we likely can't conditionally load i18n without some work to refactor how we define what captchas are available to use

While this pattern isn't ideal, given the above I think the quickest solution for Product Safety and Integrity is to enable these extensions in CI. We were estimating a few days worth of work to get the sub-extensions removed and @kostajh would prefer that we go with the quickest route. I've pinged them here in case they want to add more context

From a CI point of view, I would like to not see this pattern used anywhere else, it is an operational challenge :-(

I understand entirely, having encountered this pain while working on PHPUnit changes in the past. To clarify, we're definitely in favor of doing T398193: ConfirmEdit: Refactor captcha implementations to not use sub-extensions, we would just not like to take a detour from the lengthy list of tasks needed to get Bot detection and mitigation (WE4.2 hCaptcha editing trial) done in the next few weeks. It would be easiest for us if we had temporary accommodation to enable wfLoadExtension( 'ConfirmEdit/hCaptcha' ); in CI, and then we can come back to T398193: ConfirmEdit: Refactor captcha implementations to not use sub-extensions when we have less time pressure. Would it be OK if we implemented a temporary workaround and agreed to undo it by e.g. end of Q2 or middle of Q3?

To clarify, you all need to:

  • Add special settings in quibble's LocalSettings.php template for these sub-extensions
  • For us to make a new quibble release
  • Someone to update all relevant quibble jobs with the new release and deploy

V.S. T398193: ConfirmEdit: Refactor captcha implementations to not use sub-extensions, steps:

  • Create repositories for the new extensions
  • Split the extension code
  • Update CI's config to add jobs for the new extensions
  • Add new extensions to branched extensions for production
  • Modify production config to enable the new extensions

Is the above correct? If so, splitting will take longer, and I gather you're all in a time crunch.

@kostajh or @Dreamy_Jazz is the above is correct? Do I have it right that you want to move forward with the former option?

To clarify, you all need to:

  • Add special settings in quibble's LocalSettings.php template for these sub-extensions
  • For us to make a new quibble release
  • Someone to update all relevant quibble jobs with the new release and deploy

V.S. T398193: ConfirmEdit: Refactor captcha implementations to not use sub-extensions, steps:

  • Create repositories for the new extensions
  • Split the extension code
  • Update CI's config to add jobs for the new extensions
  • Add new extensions to branched extensions for production
  • Modify production config to enable the new extensions

Is the above correct? If so, splitting will take longer, and I gather you're all in a time crunch.

@kostajh or @Dreamy_Jazz is the above is correct? Do I have it right that you want to move forward with the former option?

It's a bit different than this. We are proposing:

  • Add a few lines in LocalSettings.php in Quibble that will also load ConfirmEdit/hCaptcha, which is a sub-extension bundled with ConfirmEdit
  • Make a new Quibble release and update jobs to use it

The alternative is to merge the sub-extensions (e.g. ConfirmEdit/hCaptcha/extension.json) into ConfirmEdit/extension.json, which has a few unknowns and complications. We'll do a bit more investigation on this to see if we think we could make that happen, as that wouldn't require any changes to CI or effort from Release Engineering.

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

[mediawiki/extensions/ConfirmEdit@master] Merge hCaptcha ResourceLoader modules into main extension

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

A summary of the internal discussions and discussions held here:

  • Loading sub-extensions in CI isn't great practice and it is preferred that if this solution was chosen it would be used as temporary workaround that would be later removed
  • After investigating the sub-extensions approach, we discovered several problems with it that led us to think the temporary workaround would be quicker to implement
    • It is still likely to have been quicker to do the temporary workaround
  • With some more discussions internally, we decided that it would be best to at least partially implement the sub-extension merge for the hCapatcha sub-extension
    • This solves the specific issue we were trying to fix by updating CI, so means we no longer need to update CI

Therefore, we have gone with the approach of getting rid of the sub-extensions as described in T398193: ConfirmEdit: Refactor captcha implementations to not use sub-extensions. The remaining work for that ticket, such as for other sub-extensions and anything not necessary for the hCaptcha QUnit tests can happen at a slower pace as they no longer block Bot detection and mitigation (WE4.2 hCaptcha editing trial).

kostajh updated the task description. (Show Details)

Change #1194584 merged by jenkins-bot:

[mediawiki/extensions/ConfirmEdit@master] Merge hCaptcha ResourceLoader modules into main extension

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

The merging of the resources in the main extension.json seems to have been the easiest path. That makes the extension more similar to the rest of the fleet and has saved us from adding some eternal temporary hack in CI :-] Thank you!