Page MenuHomePhabricator

MobileFrontend's headless qunit tests need Sinon support
Closed, ResolvedPublic3 Story Points

Description

While porting View.js and PageGateway.js to webpack (T203100+T203817), we hit several problems porting the tests.

The tests need sinon to work and adding sinon seems like a useful thing for our tests.

Acceptance criteria

  • The sinon interface is available, with the same version as MediaWiki core (and pegged to that version)
  • sinon is available consistently with how it made available in MediaWiki core (ie. if the tests run in Special:JavaScript/qunit/plain they run the same and pass the same as in headless mode
  • The code for sinon loaded inside our headless QUnit tests is not loaded in Special:JavaScript/qunit which already has its own version of sinon
  • Headless tests that reference sinon fail if sinon is not imported in the test itself

Per @Krinkle

The this.sandbox interface (also known as "sinon-qunit") is a superset that I think we do not need and it would simplify the work here. To do this, each test will be responsible to use beforeEach and afterEach to create and restore the sinon instance (one line of code). Doing that explicitly I think would improve readability and portability of the tests, as well as to feel more familiar to developers, including those not accustomed to MediaWiki-QUnit's particular way (you can blame me for that!).

Details

Related Gerrit Patches:
mediawiki/extensions/MobileFrontend : masterHygiene: make headless test setups explicit
mediawiki/extensions/MobileFrontend : masterDemo: Sinon.JS usage in headless Node.js tests

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 19 2018, 9:32 PM
Jdlrobson triaged this task as High priority.Sep 19 2018, 9:33 PM
Jdlrobson moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.

I think it'd be better to relax the second AC to just "The sinon interface is available, with the same version as MediaWiki core" .

The this.sandbox interface (also known as "sinon-qunit") is a superset that I think we do not need and it would simplify the work here. To do this, each test will be responsible to use beforeEach and afterEach to create and restore the sinon instance (one line of code). Doing that explicitly I think would improve readability and portability of the tests, as well as to feel more familiar to developers, including those not accustomed to MediaWiki-QUnit's particular way (you can blame me for that!).

Krinkle rescinded a token.
Jdlrobson updated the task description. (Show Details)Sep 19 2018, 9:51 PM
Niedzielski updated the task description. (Show Details)Sep 20 2018, 5:57 PM
Jdlrobson set the point value for this task to 3.Sep 20 2018, 5:58 PM
Jdlrobson added subscribers: nray, Jdrewniak, Niedzielski.

@Jdrewniak @Niedzielski @nray and myself discussed and estimated this today.

Change 462811 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: add isomorphic DOM test utility

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

Change 462812 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Demo: Sinon.JS usage in headless Node.js tests

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

^ is this blocked on others or us? can you clarify?

@Jdlrobson, I thought we wanted to solicit feedback from @Jhernandez. I sent an email, presented our thoughts, and invited a conversation over email or Hangouts.

Thanks for documenting!

Change 462812 abandoned by Niedzielski:
Demo: Sinon.JS usage in headless Node.js tests

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

Change 462811 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: make headless test setups explicit

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

ovasileva assigned this task to nray.Oct 2 2018, 5:13 PM
nray closed this task as Resolved.Oct 3 2018, 3:58 PM
nray updated the task description. (Show Details)