Page MenuHomePhabricator

Test WikiTextEditor class with browser tests.
Open, NormalPublic3 Story Points


WikiTextEditor appears trivial, but interacts with the super-complex EditPage. Some kind of integration test should make sure this interaction can't break, probably with a browser test

The WikiTextEditor handles the editing of the file info, building the text boxes

Related Objects

Event Timeline

Lea_WMDE created this task.Mar 27 2018, 3:25 PM
Restricted Application added a project: TCB-Team. · View Herald TranscriptMar 27 2018, 3:25 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Lea_WMDE renamed this task from Improve code coverage of WikiTextEditor class to Test WikiTextEditor class with browser tests..Mar 27 2018, 3:28 PM
Lea_WMDE triaged this task as Normal priority.
Lea_WMDE updated the task description. (Show Details)
Lea_WMDE set the point value for this task to 8.
thiemowmde added subscribers: awight, WMDE-Fisch.

The WikitextEditor class is essentially just an extracted part of the ChangeFileInfoForm class. It does have some basic unit test coverage via ChangeFileInfoFormTest already, since

A lot of what WikitextEditor does is still not properly covered, though:

  • It depends on the WikiEditor extension via the EditPage::showEditForm:initial hook.
  • It does a static call to EditPage::getEditToolbar() from core, which essentially just calls the EditPageBeforeEditToolbar hook, again something exclusively used in the WikiEditor extension.
  • There is also a dependency on some ResourceLoader modules from core, namely mediawiki.action.edit and mediawiki.action.edit.styles.

I wish we would have tests covering these dependencies, and telling us when they break. Maybe it's possible to write these tests in PHPUnit. If not, we might need to introduce browser tests in the FileImporter extension. Currently there are none. So this would cost us quite something initially, even if we can probably copy-paste most of the basic setup from another extension, e.g. Two-Column-Edit-Conflict-Merge.

awight claimed this task.May 10 2019, 1:34 PM
awight moved this task from Sprint Backlog to Doing on the WMDE-QWERTY-Sprint-2019-04-30 board.

Change 509768 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/FileImporter@master] [WIP] Browser tests for external dependencies

Change 510167 had a related patch set uploaded (by Awight; owner: Awight):
[integration/config@master] Provide WikiEditor to FileImporter tests

Change 510167 merged by jenkins-bot:
[integration/config@master] Provide WikiEditor to FileImporter tests

thiemowmde changed the point value for this task from 8 to 3.May 15 2019, 1:03 PM

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.

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

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.

Getting a screenshot would help, and that should be happening by default, but seems to not.

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

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

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

@zeljkofilipin @hashar @Krinkle I would love some help either enabling screenshots for the test failure in , or generally your thoughts about why this might be happening. AIUI, the possible causes of failure I can think of all seem unlikely:

  • Test wiki in docker container cannot connect to commonswiki API?
  • FileImporter extension is not enabled at the time of the test.
  • Admin user cannot upload files
  • .jpg uploads are disallowed
[chrome #0-9]   ChangeFileInfo page
[chrome #0-9]
[chrome #0-9]   ChangeFileInfo page
[chrome #0-9]       1) "before all" hook
[chrome #0-9]
[chrome #0-9]
[chrome #0-9] 1 failing (4s)
[chrome #0-9]
[chrome #0-9] 1) ChangeFileInfo page "before all" hook:
[chrome #0-9] An element could not be located on the page using the given search parameters (".mw-importfile-edit-button button").
[chrome #0-9] Error: An element could not be located on the page using the given search parameters (".mw-importfile-edit-button button").
[chrome #0-9]     at Promise.F (node_modules/core-js/library/modules/_export.js:36:28)
[chrome #0-9]     at click() - at Context.before (extensions/FileImporter/tests/selenium/specs/wikitext_editor.js:10:42)
[chrome #0-9]

In short: do not do anything in a before hook which could eventually fail. The video capturing is not set up yet.

Krinkle removed a subscriber: Krinkle.May 16 2019, 12:58 AM

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.

Change 511402 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/core@master] Exclude FileImporter browser tests

Change 511402 merged by jenkins-bot:
[mediawiki/core@master] Exclude FileImporter browser tests

hashar removed a subscriber: hashar.May 22 2019, 12:35 PM

The dependencies could be merged as soon as next week, so I'm moving this task to remind myself to stay engaged. See

@awight aced the implementation in Quibble . Provided the repository has defined a npm script selenium-test, Quibble will detect it and run the command with Chromedriver running.

awight added a subscriber: hashar.Fri, Aug 2, 9:38 AM

@hashar Thanks for all your effort to make the per-repo tests actually work!

So, now we're at a point where we can experiment with our repo-specific browser tests. The biggest challenge remaining is how to safely set LocalSettings.php configuration which will only apply to our repo. I think it would be slightly irresponsible to merge what I've done so far, since we know it will have side effects during gate-and-submit. Ideally, there is a config snippet which is loaded for our tests but unloaded before testing other repos. My only thought is that we need an isolated devserver.