Maniphest T193069

Replace $.Deferred() with assert.async() in tests that assert failure
Closed, ResolvedPublic2 Estimated Story Points
Actions

Description

See https://www.mediawiki.org/wiki/Reading/Web/QUnit_test_guidelines

Many of the tests in MobileFrontend and perhaps some in Popups use $.Deferred() to wait for an asynchronous result to assert. This works well but assert.async() seems preferable as a promise does not need to be returned from the test and an unnecessary direct dependency on jQuery is avoided.

Consider the following example:

QUnit.test( 'checking bad reference', function ( assert ) {
  var done = $.Deferred();                                                                             
  this.referencesGateway.getReference( '#cite_note-bad', this.page ).fail( function ( err ) {
    assert.ok( err === ReferencesGateway.ERROR_NOT_EXIST, 'When bad id given false returned.' );
    done.resolve();
  } );
  return done;
} );

Now with assert.async():

QUnit.test( 'checking bad reference', function ( assert ) {
  var done = assert.async();                                                                             
  this.referencesGateway.getReference( '#cite_note-bad', this.page ).fail( function ( err ) {
    assert.ok( err === ReferencesGateway.ERROR_NOT_EXIST, 'When bad id given false returned.' );
    done();
  } );
} );

Acceptance Criteria

All assertion promises use assert.async() instead of Deferreds.
rg '\$.Deferred' tests/ only shows mocked results

Details

	Subject	Repo	Branch	Lines +/-
	Replace $.Deferred() with assert.async()	mediawiki/extensions/MobileFrontend	master	+32 -50
	Hygiene: rename and symbolize InfiniteScroll event	mediawiki/extensions/MobileFrontend	master	+20 -16

Customize query in gerrit

Event Timeline

• Niedzielski created this task.Apr 25 2018, 6:43 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 25 2018, 6:43 PM

• Niedzielski moved this task from Incoming to Needs Prioritization on the Web-Team-Backlog board.Apr 25 2018, 6:52 PM

Change 428986 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: rename and symbolize InfiniteScroll event

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

gerritbot added a project: Patch-For-Review.Apr 25 2018, 7:17 PM

It definitely feels neater.
@Krinkle any thoughts around this? I noticed your comments on https://gerrit.wikimedia.org/r/#/c/429108/1/tests/qunit/mobile.mediaViewer/test_ImageGateway.js so I suspect you have an opinion.

Krinkle renamed this task from Replace $.Deferred() with assert.async() in tests to Replace $.Deferred() with assert.async() in tests that assert failure.May 1 2018, 4:53 PM

@Jdlrobson Yep, using that method here would make a lot of sense.

@Niedzielski Avoiding a locally wrapping Deferred is definitely an improvement. Both for source code and test alike.

For production code, one should always try to chain instead which avoids a bunch issues, such as:

Forgetting to forward the error handler.
Catching exceptions and turning them into rejections.
Improved debugging by code being part of a logical chain.
More consistent in how it looks, easier to extend in the future, and easier to convert to async-await later on.

For tests, returning a Promise to QUnit.test is generally preferred over manually tracking with assert.async(), as it (just like chaining) has the benefit of QUnit being able to inspect the Promise and also asserting that it was resolved, and reporting a failure if otherwise. (Whereas with a manual async, the test would idle until timed-out even if it was rejected for some reason.)

But, for asserting failure (rejected promise), there wasn't a clear "best practice" in QUnit 1. I've seen a bunch of different patterns, all have their own trade-offs. If only tracking a single state (like in the task description), I would agree with you that assert.async() is logically the same as the manual Deferred, where assert.async() actually has a few benefits: It avoids use of manual Deferreds (which is always suspicious), and avoids use of Deferreds when only 1 state is tracked. But both approaches result in the test timing out if getReference() unexpectedly gets resolved instead of rejected, so in QUnit 1 you'd probably want to also add a .then() handler, in which you'd assert.ok(false); done();.

Anyway, we've been on QUnit 2 for a while now, and it actually introduces a method that supersedes these: assert.rejects(). This new assertion method is essentially a combination of the (synchronous) assert.throws(), combined with the async logic behind "return Promise to QUnit.test". You'll want to use that here.

We'll discuss this Monday. Then probably ready for an estimation.

Change 428986 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: rename and symbolize InfiniteScroll event

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

We talked about this in CR session and sounds like there's no objections to doing this.
I've captured the discussion here: https://www.mediawiki.org/wiki/Reading/Web/QUnit_test_guidelines#Under_discussion

Jdlrobson updated the task description. (Show Details)May 7 2018, 7:22 PM

pmiazga set the point value for this task to 2.May 29 2018, 4:21 PM

Jdlrobson moved this task from Triaged but Future to Upcoming on the Web-Team-Backlog board.May 29 2018, 5:19 PM

ovasileva triaged this task as Medium priority.Jun 12 2018, 4:10 PM

Jdlrobson added a project: Readers-Web-Kanbanana-Board-Old.Jun 18 2018, 5:24 PM

Jdlrobson moved this task from Upcoming to 2017-18 Q4 on the Web-Team-Backlog board.

• Vvjjkkii renamed this task from Replace $.Deferred() with assert.async() in tests that assert failure to a8daaaaaaa.Jul 1 2018, 1:13 AM

• Vvjjkkii raised the priority of this task from Medium to High.

• Vvjjkkii added projects: CheckUser, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), Tamil-Sites, Gamepress, Hashtags, Jade, KartoEditor, Language-2018-Apr-June, New-Editor-Experiences, Mail, TCB-Team (now WMDE-TechWish).

• Vvjjkkii updated the task description. (Show Details)

• Vvjjkkii removed the point value for this task.

• Vvjjkkii removed subscribers: gerritbot, Aklapper.

1339861mzb renamed this task from a8daaaaaaa to Replace $.Deferred() with assert.async() in tests that assert failure .Jul 1 2018, 10:40 AM

1339861mzb lowered the priority of this task from High to Medium.

1339861mzb updated the task description. (Show Details)

JJMC89 set the point value for this task to 2.Jul 1 2018, 4:47 PM

JJMC89 removed projects: TCB-Team (now WMDE-TechWish), Mail, New-Editor-Experiences, Language-2018-Apr-June, KartoEditor, Jade, Hashtags, Gamepress, Tamil-Sites, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), CheckUser.

JJMC89 added a subscriber: Aklapper.

MBinder_WMF moved this task from 2017-18 Q4 to Current Sprint (and resolved 2018-19 Q1) pre-milestone on the Web-Team-Backlog board.Jul 3 2018, 6:56 PM

Jdlrobson claimed this task.Jul 5 2018, 5:36 PM

Jdlrobson moved this task from To Do to Doing on the Readers-Web-Kanbanana-Board-Old board.

Change 444040 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Replace $.Deferred() with assert.async()

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