Page MenuHomePhabricator

Quibble should run `npm install` and `npm run selenium-test` for each extension/skin that has Selenium tests
Closed, ResolvedPublic

Description

Selenium framework should support multiple test runners (Mocha, Cucumber...) and assertion libraries (Assert, Chai...). So far all dependencies were managed from mediawiki core, but that has serious drawbacks (as @Krinkle points in T179190#4173431).

I have talked with @hashar and we have agreed that the way forward is to modify Quibble.

Workflow:

  • clone mediawiki core
  • clone extension(s)/skin(s)
  • find all wdio.conf.js files
  • cd into each extension/skin with wdio.conf.js file
  • run npm install
  • run npm run selenium-test

Running multiple npm install might slow Quibble down, so speeding up that part will be important.

Also, in order for the last step to work, each extension/skin has to have selenium-test script in package.json, but that will be resolved with T199113.

Note: see @Krinkle's comments at 429435.

See also: T193943: Selenium test job should install local dependencies before starting tests.

Related Objects

Event Timeline

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

Maybe that can be done during all-hands.

That would be amazing, a bunch of our patches just hit into this again!

Is there any chance that you did get time to look at this during All Hands, @hashar? I know it was very busy.

hashar added a comment.Feb 5 2019, 1:08 PM

I haven't even opened my laptop over the week! That I guess tells we all were too busy discussing. Gotta now dig into implementing this in Quibble :]

hashar removed hashar as the assignee of this task.Mar 19 2019, 8:40 PM

Sorry, I am definitely not working on this one :-(

Jdlrobson changed the task status from Stalled to Open.Apr 5 2019, 4:49 PM
Jdlrobson added a subscriber: Jdlrobson.

I'd like to unstall this one for visibility. This has been blocking my team from migrating our browser tests from Ruby to Node.js.
I the answer is we don't have the bandwidth to support Cucumber (which the timeframe of this ticket so far indicates) this would be useful to know and maybe my team can devise an alternate plan forward.

Resolving T219920 is a top priority for me, given this removes an important set of tests from our stack that are needed as we are changing code that touches them relating to our annual goal Advanced Mobile Contributions.

From my perspective T190710#5088918 is the main problem we need to unblock my team.

Change 509414 had a related patch set uploaded (by Zfilipin; owner: Zfilipin):
[mediawiki/core@master] Selenium: add jpeg-js to devDependencies

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

Change 509414 merged by jenkins-bot:
[mediawiki/core@master] selenium: Add jpeg-js to devDependencies

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

awight added a subscriber: awight.May 16 2019, 10:47 AM

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

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.

Change 510709 had a related patch set uploaded (by Awight; owner: Awight):
[integration/quibble@master] [WIP] Run node browser tests in each repo

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

awight moved this task from Enhancement to In progress on the Quibble board.May 16 2019, 12:58 PM

@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.

Change 511893 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/core@master] Stop running extension and skin browser tests with core

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

Change 511898 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/PagedTiffHandler@master] Rename tests/selenium/ folder to "phpunit-selenium"

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

Change 511898 merged by jenkins-bot:
[mediawiki/extensions/PagedTiffHandler@master] Rename tests/selenium/ folder to "phpunit-selenium"

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

@Jdforrester-WMF made the suggestion that we block on T211784, so that selenium tests are disabled for the Node 10 upgrade.

We're interested in helping with this migration if possible, but first I wanted to discuss with the RelEng team.

If others agree that this patch as written is too dangerous and we should wait for browser tests to be disabled, I could make the quibble patch conditional on a flag or on configuration, so that it's possible to run the new logic only for experimental or specific jobs. Another alternative would be to disable browser tests just for this change--independent of the Node 10 work since that's not an explicit dependency for anything we're doing here.

awight claimed this task.May 29 2019, 10:18 AM
awight added a comment.Jun 4 2019, 9:47 AM

According to @Jdforrester-WMF's comment, we're waiting to merge these patches before this task can proceed.

Given the current wdio is incompatible with node10, upgrading to node10 *after* doing per-repo versions of wdio means we'll need to urgently unbreak bump dozens of repos with must-merge patches, instead of just MW core. I'm not sure that's a deal-breaker, but we really need to Just Do It™ at this point.

Should now be good to go.

awight added a comment.Jun 7 2019, 9:09 PM

There are at least two easy speedups for npm install, the first is documented in subtask T225330: Commit package-lock.json files everywhere, which just became possible thanks to the npm 10 upgrade. The second is --prefer-offline, which only fetches packages when they are missing from the local cache. We should experiment with both.

awight added a comment.Jul 3 2019, 9:48 AM

I've adjusted the patch to detect selenium tests based on the scripts in package.json, rather than testing for directory tests/selenium existence, so we'll be compatible with Wikibase and other unusual hierarchies.

Tarrow rescinded a token.Jul 9 2019, 1:48 PM
Tarrow awarded a token.
awight added a comment.EditedJul 15 2019, 12:44 PM

Friends, I see that this task is in a few "external / watching" columns, so I want to clarify the status as I understand it. I've written a patch which should provide the needed functionality, and I believe it's both a safe migration and might not damage performance by much, since I've disabled npm's default cache-refresh network calls.

What we need now is code review of the patch.

Once this is merged, I have some more questions about a new mechanism for injecting snippets into LocalSettings.php, but that's follow-up work.

@zeljkofilipin would you be the right person to review for @awight ?

It took me a few tries to reach out to @awight patch but I have finally managed to do a proper review of https://gerrit.wikimedia.org/r/#/c/integration/quibble/+/510709/ today :] It is almost ready to be merged!

Change 510709 merged by jenkins-bot:
[integration/quibble@master] Run node browser tests in each repo

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

The feature has been implemented by @awight and I have released it today with Quibble 0.0.33 (which is broken so really use 0.0.34).

I have deployed it on CI and that worked for MinervaNeue but fails on CirrusSearch/MobileFrontend due to wdio-cucumber-framework. T229033 is to update it.

Change 525669 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] Revert "jjb: switch to quibble 0.0.34 containers"

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

Change 525669 merged by jenkins-bot:
[integration/config@master] Revert "jjb: switch to quibble 0.0.34 containers"

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

Implemented by @awight and released with 0.0.34. I have missed a few things after review and had to add a few hotfixes but overall it seems to be running fine now.

I guess now in tests/selenium/wdio.conf.js we can remove the various wildcard:

relPath( './extensions/*/tests/selenium/specs/**/*.js' ),
relPath( './extensions/VisualEditor/modules/ve-mw/tests/selenium/specs/**/*.js' ),
relPath( './extensions/Wikibase/repo/tests/selenium/specs/**/*.js' ),
relPath( './skins/*/tests/selenium/specs/**/*.js' )

Replaced by having to do: cd extensions/Foobar && npm run-script selenium-test.

Right now Quibble runs webdriver.io for mediawiki/core which glob all specs from extensions and skins AND it also run the tests via selenium-test. Thus the extensions / skins are run twice.

hashar closed this task as Resolved.Jul 25 2019, 11:20 PM

Change 525692 had a related patch set uploaded (by Hashar; owner: Hashar):
[mediawiki/core@master] selenium: stop running extensions/skins specs

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

Change 525693 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/core@master] selenium tests: Drop manual specification of extensions and skins' tests

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

Change 525693 abandoned by Jforrester:
selenium tests: Drop manual specification of extensions and skins' tests

Reason:
Antoine beat me by a few seconds. ;-)

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

Change 511893 abandoned by Awight:
Stop running extension and skin browser tests with core

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

Change 525692 merged by jenkins-bot:
[mediawiki/core@master] selenium: stop running extensions/skins specs

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

awight added a comment.Mon, Aug 5, 8:58 PM

I'm auditing a recent gate-and-submit job to sanity check the wdio.conf.js changes to mediawiki-core, and found this concerning line,

00:12:17.444 WARNING:backend.ChromeWebDriver:[1565037144.160][SEVERE]: bind() failed: Cannot assign requested address (99)

That looks like we're failing to manage the webdriver correctly.

Meanwhile, everything else looks good. Spot-checking a few test cases, each test is now run exactly once, so the core change is validated.

The separate browser tests are parsed into sections by Jenkins, which is a nice bonus.

I'm auditing a recent gate-and-submit job to sanity check the wdio.conf.js changes to mediawiki-core, and found this concerning line,

00:12:17.444 WARNING:backend.ChromeWebDriver:[1565037144.160][SEVERE]: bind() failed: Cannot assign requested address (99)

That looks like we're failing to manage the webdriver correctly.

I guess because somehow we try to start chomewebdriver a second time? That is worth a task :-]

Meanwhile, everything else looks good. Spot-checking a few test cases, each test is now run exactly once, so the core change is validated.
The separate browser tests are parsed into sections by Jenkins, which is a nice bonus.

Great! Thank you to have double checked :-]