Page MenuHomePhabricator

Return promises from QUnit tests
Closed, ResolvedPublic2 Estimated Story Points

Description

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.

AC

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

Notes

Files probably affected:

[0]
ag --js 'done\(\)' tests/node-qunit                                                                                                                                                                                                                                 !2609
tests/node-qunit/actions.test.js
168:		done();
197:		done();
236:		done();
261:		done();
340:		done();
394:			done();
446:		done();

tests/node-qunit/gateway/mediawiki.test.js
164:		done();
181:		done();
221:		done();

tests/node-qunit/gateway/rest.test.js
238:		done();
251:		done();
264:		done();
297:		done();

tests/node-qunit/integration.test.js
199:		done();
213:		done();
226:			done();
258:			done();

tests/node-qunit/renderer.js
305:		done();
339:		done();
373:		done();

tests/node-qunit/wait.test.js
22:		done();

There are a maximum of 22 tests affected.

Event Timeline

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

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

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

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

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.