Page MenuHomePhabricator

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

Description

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 https://gerrit.wikimedia.org/r/422173.

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.

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

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

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

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

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

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

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 https://gerrit.wikimedia.org/r/#/c/integration/config/+/502356/7/jjb/mediawiki-extensions.yaml

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 https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/FileImporter/+/509768/ , 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

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

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

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

hashar removed a subscriber: hashar.Wed, May 22, 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 https://gerrit.wikimedia.org/r/#/c/integration/quibble/+/510709/