It wasn't a layer. It was just.... something weird. This should be the fix, thanks for catching!

In T200018#5200822, @thiemowmde wrote:

Oh, the colors in this image are not entirely correct. Bonus points for flipping the mountain!

The mediawiki-core usages make it look like we're going to invalidate a lot of cached diffs with this... https://codesearch.wmflabs.org/search/?q=WikiDiff2MovedParagraphDetectionCutoff&i=nope&files=&repos=

Some successes in the WIP patch so far. It actually runs through the whole process through browser testing, at least when smoke-testing a trivial code path. It prints a medium-granularity execution plan, and flag --plan-only stops before execution. I'm happy about how all command parameters must be explicitly passed into the constructor, so there are no self.* surprises which might cause implicit coupling issues later, and the command implementations can be extracted into separate modules as desired. Personally, I think that's a good direction to go in, so that each module has one general responsibility. The ZuulCloneCommand build_params hack isn't completely disgusting, which is unexpected.

I ran into another complication: the zuul cloner takes a huge number of parameters, plus reading from the environment. The command abstraction shouldn't explicitly wire the parameters together because that would be fragile and high-maintenance. The only trick I've come up with so far is to separate the parameters into a list of projects, then an opaque **kwargs which is built by the calling code and by the quibble.zuul module. I don't like it, but there is one small benefit, that it facilitates dumping the entire list of parameters to zuul for debugging.

For entertainment:

I've copied some configuration into here, https://commons.wikimedia.beta.wmflabs.org/wiki/Extension:FileImport/Data

How does this relate to T211702: Quibble initialize step should only clone the target repository? It seems that they are redundant, if we do this task then the lint steps should be skipped in Quibble, and if we do the other task then this one is irrelevant.

Reading about the differences between beta commons and beta testwiki, I think that beta commons is the right target for this change. Specifically, betacommons currently

does not respect CommonsHelper2 config files

In T222873#5197156, @Jan_Dittrich wrote:

One possibility I see is having a rather humble pointer to it:

Oh :-) That's even simpler than I was imagining, since we own the wrapper script. I think your migration will work: we leave jjb/* configuration alone and deploy your run.sh patch, then remove the redundant subpaths from jjb. I was able to verify that the trailing path parameter is harmless.

It would be nice to package the PHP fragments in a conf.d-style directory, which gets cleaned and repopulated between browser tests on each repo, to reduce the chance of interference between tests.

How about we disable the progress bar while leaving the phan version the same?

In T223021#5180800, @thiemowmde wrote:

From my experience, the best naming scheme is FileImporter\Tests\…, followed by a copy of the namespace of the file under test. The most relevant advantage of this scheme is that it allows very convenient PSR-4 class loading.

This patch plus https://gerrit.wikimedia.org/r/#/c/integration/quibble/+/510709/ should do the job.

@hashar This is ready for review, and I've smoke-tested locally. The big question IMO is whether I've implemented the desired functionality, this will run npm install && npm run selenium-test in each of self.dependencies which includes a tests/selenium.

Reading the task description again, I see that my understanding was slightly different than the original suggestion. The request is to run *all* extension and skin tests.

I'm going to take a brief look at this. The changes will need to be made in several steps, AIUI:

• Patch quibble to run npm install && npm run selenium-test in the extension-or-skin root directory.
• Build a new quibble image
• Patch integration-config to use the new quibble image.
• Tests will be duplicated at this point. We should verify this--each repo with tests/selenium/specs/*.js should also have a selenium-test script listed in package.json.
• Remove extensions and skins from search patch in mw-core wdio.conf.js

Looks like the biggest blocker is T199116: Quibble should run `npm install` and `npm run selenium-test` for each extension/skin that has Selenium tests, because:

mwselenium.sh wouldn't quite work in our case, since the test is being run from mw-core

Because of T223431, we might have to introduce some nasty code changes and a test-only parameter to allow us to navigate to Special:ImportFile pages even when $wgEnableUploads is false. Of course, the final check must be respected to prevent actually uploading on submit.

I'm able to connect to commonswiki from an integration worker, so this theory might be wrong.

Thoughts: we can upload a file to the local test wiki, implement a test-only parameter "ignoreDuplicateHash", and import from self.

Oooh--my current guess is now simply that we can't connect to commonswiki from the test.

These tests are still failing in CI, and I'm short on clues to debug. All I know is that the expected element (edit file info button) isn't present on Special:ImportFile?clientUrl=..., which shouldn't require any special LocalSettings.php configuration, it should Just Work as long as the extension is installed.

Correction--it looks like mw-core browser tests will run for every patch, regardless. So we might as well keep what I've done, and consider additional daily tests via a patch like https://gerrit.wikimedia.org/r/#/c/integration/config/+/502356/7/jjb/mediawiki-extensions.yaml

Currently I have the browser test running with every patch submitted to Gerrit, but we might want to only run these tests daily instead, since we're mostly checking for external breaking changes and almost nothing we do in the extension should change these integrations. For example, if WikiEditor will break our ChangeInfoEditForm a few months after we stop working on FileImporter, we want to get an alert.

My manager is @Tobi_WMDE_SW, and the CI work would usually full under the maintenance / 20% category for that job. For example, right now I'm trying to help correct some flapping tests that are annoying for us, T219114: phan 1.2.6 is OOMing on MediaWiki core

In T223017#5180950, @thiemowmde wrote:

• All the test might need to do is to replace the FileImporterHttpRequestExecutor service with a mock.

I just experienced this crash 4 times in a row, when running CI on mw-ext-FileImporter. Looking at the integration-config repo,

./jjb/mediawiki-extensions.yaml:        image: 'docker-registry.wikimedia.org/releng/mediawiki-phan:0.1.11'

Based on the factor above, I would suggest this patch:

--- a/DiffEngine.h
+++ b/DiffEngine.h
@@ -33,7 +33,7 @@
 // Default value for INI setting wikidiff2.moved_line_threshold
 #define WIKIDIFF2_MOVED_LINE_THRESHOLD_DEFAULT "0.4"
 // Default value for INI setting wikidiff2.moved_paragraph_detection_cutoff
-#define WIKIDIFF2_MOVED_PARAGRAPH_DETECTION_CUTOFF_DEFAULT "30"
+#define WIKIDIFF2_MOVED_PARAGRAPH_DETECTION_CUTOFF_DEFAULT "120"

My extremely unscientific sampling of diffs, using this patch: P8510, found that in benchmarks/revs.json.gz, there were 244 total chunks, with:

@Lea_WMDE please leave a note here once you can confirm that the requested behavior is what we want.

(patch solves the issue for me, waiting for someone with CR+2 on this repo)

I'm not sure how to get into the file importer on commonswiki to test this, but I did find an example file on dewiki which uses a template with parameter mapping:
https://de.wikipedia.org/wiki/Datei:GermanyDemRepPFX1-50Pfennig-1979-donatedmjd_b.jpg