Page MenuHomePhabricator

Test WikiTextEditor class with browser tests.
Closed, ResolvedPublic3 Estimate 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 Gerrit Patches:
mediawiki/core : masterFileImporter browser tests require uploads enabled
mediawiki/extensions/FileImporter : masterBrowser tests for external dependencies
integration/quibble : masterAllow uploads to support FileImporter tests
mediawiki/core : masterExclude FileImporter browser tests
integration/config : masterProvide WikiEditor to FileImporter tests

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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.Oct 4 2019, 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.Oct 7 2019, 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.Oct 9 2019, 1:07 PM
awight added a comment.Oct 9 2019, 1:50 PM

This will be released with quibble 0.0.38, coming soon.

Unblocked! Browser tests are passing...

This is ready for final review:

@awight the patch has merge conflict.

awight closed this task as Resolved.Oct 22 2019, 12:20 PM
awight reopened this task as Open.Oct 22 2019, 12:37 PM
awight moved this task from Done to Doing on the WMDE-QWERTY-Sprint-2019-10-09 board.

Oops, the main patch is still pending!

Change 509768 merged by jenkins-bot:
[mediawiki/extensions/FileImporter@master] Browser tests for external dependencies

awight closed this task as Resolved.Oct 22 2019, 1:34 PM
awight moved this task from Doing to Done on the WMDE-QWERTY-Sprint-2019-10-09 board.

Great to see this task has now been resolved. Well done @awight!

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