Page MenuHomePhabricator

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

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
zeljkofilipin moved this task from Backlog to Enhancement on the Quibble board.Jul 9 2018, 2:24 PM
zeljkofilipin moved this task from Inbox to Selenium on the MediaWiki-Core-Testing board.
zeljkofilipin updated the task description. (Show Details)Jul 9 2018, 2:35 PM
zeljkofilipin updated the task description. (Show Details)
zeljkofilipin updated the task description. (Show Details)

At least for extensions, we already run "npm install" "npm test" but that is then discarded entirely to prevent dependencies from leaking in. So probably the resulting node_modules install should be moved to some transient place and be restored later on when we need it.

To run those tests, we would need the WebServer to be exposed as well as ChromeDriver. That can be seen in Quibble/cmd.py. Roughly:

if self.should_run('selenium') and \
        with quibble.backend.ChromeWebDriver(display=display):
            quibble.test.run_webdriver(...)

That run_webdriver() function runs npm run selenium-test from the root of mediawiki/core. It has the spec wildcard to glob every specs under extensions / skins.

So we would need a new command that implement the workflow mentioned here. npm install for all extensions, find wdio.conf.js files, run npm install / npm selenium-test.

Krinkle added a comment.EditedJul 16 2018, 11:19 PM

At least for extensions, we already run "npm install" "npm test" but that is then discarded entirely to prevent dependencies from leaking in.

Leaking to where? The only context in which node_modules in an extension is used by Node.js is when executing Node.js programs inside that directory.

Currently, the only process that runs a program in that directory is npm test (typically eslint or grunt with banana-checker).

The proposal to use npm run selenium-test would create a second process in that directory, and that needs the same dependencies (per the above).

As far as I know, there are no other Node.js programs in that directory. And if there would be, they would also expect the same dependencies.

zeljkofilipin changed the task status from Open to Stalled.

As far as I can see it, this should be done in Quibble. I'm not familiar with it, so it's unlikely I would be working on this.

phuedx added a subscriber: phuedx.Sep 19 2018, 1:31 PM

As far as I can see it, this should be done in Quibble. I'm not familiar with it, so it's unlikely I would be working on this.

Who's responsible for the development of Quibble? I'd like to understand what kind of timeline we're looking at to get this functionality added since it blocks the migration of MobileFrontend/MinervaNeue's browser tests from Ruby to Node.js.

Who's responsible for the development of Quibble?

Please read its project description page as it lists an owner...

Who's responsible for the development of Quibble?

Team: Release-Engineering-Team, person: @hashar.

I'd like to understand what kind of timeline we're looking at to get this functionality added since it blocks the migration of MobileFrontend/MinervaNeue's browser tests from Ruby to Node.js.

I'll leave that to @hashar.

The logic is Quibble quibble/cmd.py, roughly what it does is:

  • clone all repositories
  • change dir to extension or skin
    • npm install && npm test
    • clean the directory git clean -xqdff

git clean does remove node_modules, and that is intentional.

That is done before MediaWiki get installed. So we can not just add npm run-script selenium there. We will want to invoke way later next to where we run the selenium tests provided by mediawiki/core. That would require to run npm install again for the extension.

So probably we want to save the extension node_modules from the first phase and restore it when running the selenium tests. Or maybe have the node_modules to be installed outside of the source tree and point to it when needed using NODE_PATH.

Running multiple npm install might slow Quibble down

In my experience, newer versions of NPM are significantly faster than pre-lockfile versions. Perhaps, merely upgrading NPM and enabling reinstalls will make this task obsolete.

@phuedx poked Greg about it. The logic is in my previous comment, gotta implement it :]

β€’ Addshore added a comment.EditedJan 4 2019, 2:58 PM

This is and has been blocking the Wikidata team with T200011, which appear to also be blocking T211038 and T211120

An example of the issue can be seen in https://integration.wikimedia.org/ci/job/quibble-vendor-mysql-hhvm-docker/29617/console where Wikibase tests are running, but this also runs the WikibaseLexeme browser tests, these require the wdio-wikibase npm package, but npm install is not run within WikibaseLexeme so the package doesn't exist in the context of WikibaseLexeme.

I'll try to think about a workaround until this is fixed here so that we can unblock our things

Ping @hashar, @zeljkofilipin: Has any progress been made on this? Like the Wikidata folk, Readers Web can't move forward with T190710: Minerva Ruby and Node.js browser tests running side by side because of this issue.

As far as I know, this is blocked on @hashar being too busy. 😒 I can't really help because I'm not familiar with python or quibble.

hashar claimed this task.Jan 25 2019, 9:00 PM

This got raised during this week Release-Engineering-Team team meeting. I would have to read again the analysis above and do the implementation in Quibble. Maybe that can be done during all-hands.

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

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.Wed, May 29, 10:18 AM
awight added a comment.Tue, Jun 4, 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.Fri, Jun 7, 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.