Page MenuHomePhabricator

Migrate processLinks.test.js to node-qunit
Closed, ResolvedPublic3 Story Points

Description

processLinks.test.js is the only qunit test on Page-Previews that doesn't run in node-qunit.

Let's migrate it to run in node-qunit so that we only have one suite of unit tests to run when developing.

Notes

  • Test is written in a test-case manner, heavily depending on mw libraries that require a running mediawiki
    • mw.Regex.escape can be stubbed
    • mw.Uri depends on mw.template, hard to stub
    • mw.Title depends on correct mw.config values, very hard to stub

My take would be to change the tests to be unit tests instead of integration tests, and verify via stubs that Regex, Uri and Title are called correctly escape and parse links, and remove the test-case approach.

Maybe add a browser test with a few of the test-case links for having an integration test.

AC

  • All qunit tests are now node-qunit
  • processLinks remains well tested
  • The "qunit" Grunt task is removed along with the grunt-qunit dependency

Details

Related Gerrit Patches:
mediawiki/extensions/Popups : masterHygiene: Tidy up QUnit references
mediawiki/extensions/Popups : masterHygiene: Tests: Remove unused stubs
mediawiki/extensions/Popups : masterTests: Unit test getTitle
mediawiki/extensions/Popups : masterTests: Refactor processlinks test
mediawiki/extensions/Popups : masterHygiene: QUnit setup -> beforeEach & teardown -> afterEach
mediawiki/extensions/Popups : masterHygiene: Lint JS files on tests/node-qunit too
mediawiki/extensions/Popups : masterTests: Remove grunt-contrib-qunit
mediawiki/extensions/Popups : masterTests: Migrate processLinks.test.js to node-qunit
mediawiki/extensions/Popups : masterHygiene: Clear global stub after the test

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 14 2017, 12:14 PM

Sounds great!

Note that as of this week node-qunit has joined the qunitjs organisation for improved maintenance and general support.

The oojs.git repo has an example of node-qunit usage that may help. See oojs.git:/tests/node-index.js.

phuedx triaged this task as Normal priority.Mar 16 2017, 11:35 AM
phuedx moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
phuedx updated the task description. (Show Details)Apr 19 2017, 5:44 PM
Jdlrobson set the point value for this task to 3.Apr 19 2017, 5:47 PM
Jdlrobson moved this task from Needs Analysis to To Do on the Reading-Web-Sprint-96 board.
Jhernandez moved this task from To Do to Doing on the Reading-Web-Sprint-96 board.
phuedx added a subscriber: phuedx.Apr 25 2017, 9:33 AM

You might also consider removing the watch Grunt task as part of this task as it won't be necessary after the last remaining browser-based QUnit test has been migrated.

Change 350182 had a related patch set uploaded (by Jhernandez):
[mediawiki/extensions/Popups@master] Hygiene: Clear global stub after the test

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

Change 350183 had a related patch set uploaded (by Jhernandez):
[mediawiki/extensions/Popups@master] Tests: Migrate processLinks.test.js to node-qunit

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

Change 350184 had a related patch set uploaded (by Jhernandez):
[mediawiki/extensions/Popups@master] Tests: Remove grunt-contrib-qunit

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

Change 350185 had a related patch set uploaded (by Jhernandez):
[mediawiki/extensions/Popups@master] Hygiene: Lint JS files on tests/node-qunit too

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

@phuedx Watch is still useful for the linters for now, although I'd like to remove it definitely.

I'm also going to remove references to setup/teardown now that I'm at it. Will do later.

Change 350226 had a related patch set uploaded (by Jhernandez):
[mediawiki/extensions/Popups@master] Hygiene: QUnit setup -> beforeEach & teardown -> afterEach

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

Change 350182 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Hygiene: Clear global stub after the test

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

@Jhernandez FYI that I've left some comments in case you've missed them.

Thanks for the review @bmansurov, I answered, let me know what you think.

BTW I've re-enabled email notifos for gerrit stuff to not miss comments and such. It was a pain to look into all the patches before to see if there were updates.

Change 350183 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Tests: Migrate processLinks.test.js to node-qunit

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

Change 350184 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Tests: Remove grunt-contrib-qunit

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

Change 350185 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Hygiene: Lint JS files on tests/node-qunit too

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

Change 350226 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Hygiene: QUnit setup -> beforeEach & teardown -> afterEach

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

@Jhernandez to follow up with a patch removing the stubs in favour of simpler ones for more readability.

Change 350611 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Tests: Refactor processlinks test

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

Change 350612 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Tests: Unit test getTitle

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

Change 350613 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Hygiene: Tests: Remove unused stubs

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

Change 350611 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Tests: Refactor processlinks test

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

Change 350612 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Tests: Unit test getTitle

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

Change 350613 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Hygiene: Tests: Remove unused stubs

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

phuedx removed Jhernandez as the assignee of this task.Apr 28 2017, 9:55 PM
phuedx moved this task from Needs Code Review to Ready for Signoff on the Reading-Web-Sprint-96 board.

💥 💥 💥 There's nothing left to do here, right?

Jdlrobson closed this task as Resolved.May 1 2017, 4:55 PM
Jdlrobson claimed this task.

This is a technical task and this is done! w00t!

Change 351798 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/extensions/Popups@master] Hygiene: Tidy up QUnit references

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

Change 351798 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Hygiene: Tidy up QUnit references

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