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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.Aug 2 2019, 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.

We need browser tests for a stable deployment. Let's put this on the roadmap again, now that the preparatory work has been done.

If it doens't break any other extensions, I think a quick-and-dirty cat >> LocalSettings.php is on the table, so we don't get too blocked on configuration.

hashar removed a subscriber: hashar.Sep 6 2019, 2:09 PM

Change 540118 had a related patch set uploaded (by Awight; owner: Awight):
[integration/quibble@master] Allow uploads to support FileImporter tests

This will take some tweaks to , and then in theory should work, but realistically will need another round of debugging CI.

If I'm not mistaken, two patches are still open here. They depend on each other, so it's conceptually only one patch.

@hashar left some super helpful comments. However, to me it does not sound like we have an obvious way forward. @awight, what do you suggest?

awight added a comment.Fri, Oct 4, 7:48 AM

If I'm not mistaken, two patches are still open here. They depend on each other, so it's conceptually only one patch.

Thanks for nudging us towards a more verbose update :-) That's exactly right, the main patch is in good shape but cannot be merged until CI passes. The current blocker to CI is that $wgEnableUploads defaults to false, so I've tried to address that in . I'll work on this today and will move back to the Review column after that's ready. I can run Quibble locally to verify my patch.

awight removed awight as the assignee of this task.Mon, Oct 7, 4:32 PM

Change 540118 merged by jenkins-bot:
[integration/quibble@master] Allow uploads to support FileImporter tests

Watching until quibble 0.0.36 is deployed, then we can try to run and debug the FileImporter tests.

awight claimed this task.Wed, Oct 9, 1:07 PM
awight added a comment.Wed, Oct 9, 1:50 PM

This will be released with quibble 0.0.38, coming soon.