Page MenuHomePhabricator

Standardising our use of headless node qunit tests across Popups and MobileFrontend
Open, NormalPublic

Description

During code review of the Hygiene: introduce headless QUnit tests a couple of us were surprised to see qunit listed in package.json of MobileFrontend, given that Popups uses node-qunit which provides a wrapper around qunit and some of the scaffolding e.g. MockMediaWiki that now lives inside MobileFrontend.

Stephen later explained mw-node-qunit adds jsdom, sinon, and wraps QUnit and since @Jhernandez wrote it and the team is not familiar with it we may want to revisit it.

Possible options down the road include

  • Spinning out wikimedia/mock-mediawiki for handling MediaWiki dependencies
  • Use qunit in all extensions using webpack
  • Expanding/adapting mw-node-qunit

Related tasks: T212521

Acceptance criteria

  • Use of headless qunit runner is consistent between Popups and MobileFrontend
  • Tests should gate merging. This is currently the case for Popups but not headless tests in MobileFrontend.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 30 2018, 1:06 AM
Jdlrobson triaged this task as Normal priority.

Move scaffolding to mw-node-qunit

I would prefer to create a new NPM package, @wikimedia/mock-mediawiki, that we include as a devDependency where needed (perhaps in mw-node-qunit or MobileFrontend). Tests that need it can import the mock. The reason is that other repos don't care about QUnit but do care about mocking out MediaWiki.

Update MobileFrontend to use node-qunit

Can we wait until this is required? mw-node-qunit adds jsdom, sinon, and wraps QUnit. @Jhernandez is quite familiar with this code having written it but I don't think the current team is and I would prefer to reevaluate which parts are still necessary and which aren't to avoid unnecessary complexity.

Thanks for the background!
It feels like we might need to use mw-node-qunit (given we are likely to need sinon and jsdom) but I also see where you are coming from that we don't want to use it prematurely.
I like the idea of a new NPM package
As long as we are aiming to be consistent for both Popups and MobileFrontend I think this can indeed wait.

Should I make this task about "Standardising our use of headless node qunit tests across Popups and MobileFrontend"?

sounds good to me.

Jdlrobson renamed this task from Discussion: Why does MobileFrontend use qunit and Popups wikimedia/node-qunit to Standardising our use of headless node qunit tests across Popups and MobileFrontend.Sep 5 2018, 6:21 PM
Jdlrobson updated the task description. (Show Details)

Tracking for time being (will pull in from MobileFrontend.js when we are further along)

Jdlrobson lowered the priority of this task from High to Low.

I was just re-reading the task and I find these comments funny 😃:

and since @Jhernandez wrote it and the team is not familiar with it we may want to revisit it.

@Jhernandez is quite familiar with this code having written it but I don't think the current team is and I would prefer to reevaluate which parts are still necessary and which aren't to avoid unnecessary complexity.

mw-node-qunit/wikimedia-setup.js and mw-node-qunit/index.js are a total of only 72 LOC 😂 I'm fairly certain the team can get familiar with the runner code and understand its complexity in about 10 minutes tops.

The rest of the API surface (jQuery, qunit, sinon) are already present independently of using mw-node-qunit or not.


BTW it has been 5 months since the last release, it is probably a good idea to do a dependency upgrade and a new release 👍

When we get round to doing this, we may find that what we've created is near identical or the existing mw-node-qunit is a subset.
I suspect we'll repurpose mw-node-qunit regardless (either replacing it or adding to it)
If the library can handle MobileFrontend I think it's safe to say it can handle any extension.

A good time to do this is probably just before we start thinking about porting code in Minerva to webpack.

Niedzielski updated the task description. (Show Details)Dec 21 2018, 6:08 PM
Niedzielski updated the task description. (Show Details)Dec 21 2018, 6:11 PM
Niedzielski updated the task description. (Show Details)Dec 21 2018, 6:15 PM
Jdlrobson raised the priority of this task from Low to Normal.