Page MenuHomePhabricator

Return promises from QUnit tests
Closed, ResolvedPublic2 Estimated Story Points


Per the QUnit docs, QUnit#test can handle promise resolution and rejection if the test function returns a thenable. Using assert#async when testing promises, therefore, is over-complicating the tests.


  • Any QUnit test that invokes assert#async and is dealing with promises:
    • Shouldn't invoke assert#async.
    • Should return a promise.


Files probably affected:

ag --js 'done\(\)' tests/node-qunit                                                                                                                                                                                                                                 !2609
168:		done();
197:		done();
236:		done();
261:		done();
340:		done();
394:			done();
446:		done();

164:		done();
181:		done();
221:		done();

238:		done();
251:		done();
264:		done();
297:		done();

199:		done();
213:		done();
226:			done();
258:			done();

305:		done();
339:		done();
373:		done();

22:		done();

There are a maximum of 22 tests affected.

Event Timeline

Jdlrobson triaged this task as Medium priority.Jul 17 2017, 5:12 PM
Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.
Jdlrobson set the point value for this task to 2.Aug 15 2017, 4:23 PM
Jdlrobson added a subscriber: Jdlrobson.

Estimations were 1-2. We decided on 2 as this is quite a mechanical change, but there is some tidy up to do. There are also some gotchas with Promise's that we need to look out for when working on this.

Change 372173 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Return promises from QUnit test

Change 372173 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Return promises from QUnit test

I've re-reviewed the patch and it is done and tests pass.

I've noticed we have a lot of .done which makes promises behave non-standard, so I'm going to submit a follow up patch to fix that.