Closed Bug 1086528 Opened 5 years ago Closed 5 years ago

telephony_helper_test.js incorrectly uses Promises as a way to delay a test assertion

Categories

(Firefox OS Graveyard :: Gaia::Dialer, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
Details | Review
The way Promise callbacks are scheduled is changing for the better in bug 1083783 and bug 1013625.

However, these changes are blocked from landing because the "telephony_helper_test.js" Gaia test contains some code that incorrectly uses Promises as a way to delay a test assertion. One such example:

      test('should display the BadNumber message', function(done) {
        mockPromise = Promise.reject('BadNumberError');
        this.sinon.spy(MockTelephonyMessages, 'handleError');
        subject.call('123', 0);
        mockPromise.catch(function() {
          sinon.assert.calledWith(MockTelephonyMessages.handleError,
                                  'BadNumberError', '123',
                                  MockTelephonyMessages.REGULAR_CALL);
        }).then(done, done);
      });

This test assumes that the callback passed to "mockPromise.catch" is called _after_ the MockTelephonyMessages.handleError mocked function. The mocked function is actually called at some point inside a promise chain in the "subject.call" function under test.

However, there is no such guarantee, as the two calls are part of two distinct "then" chains (despite they share part of promise tree). This test just happened to work because of some internal Promise implementation details.

The SinonJS assertion framework has no support for waiting until a call is eventually made, and chai-as-promised also doesn't provide this.

The best viable solution is probably to fully mock the function being asserted (MockTelephonyMessages.handleError in this case) and have it check its parameters, then invoke the "done" callback. If this is not possible, then we could temporarily use a full setTimeout(0) as a stopgap - even fake timers shouldn't alter the invariant that the setTimeout call waits at least an event loop tick.
So, it turns out that attempting to rewrite the first test (the "should bind the onconnected callback" one) in the correct way revealed an issue in the underlying code:

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/telephony_helper.js#L112

It seems "installHandlers" is called after "dial" or "dialEmergency" already returned. This means that the dialing procedure may, technically, attempt to call the hooks before they are placed in the "call" object. In practice this may not happen because of current timing (both Promise timing and internal "dial" implementation details), but it clearly seems unsafe.

The fact is that, because "dial" itself is stubbed, any production-environment timing issues with the "dial" behavior won't be detected by the test. The test just checks that "onconnected" is present on the object, without simulating calling it, but even if it did, what the stub does in terms of synchronicity/timing of the call might not match what the production environment does...

Any chance that an experienced Gaia dev could look into this bug in the next few days? I can create a patch later, but it would definitely take more time as I'd need to set up a full environment to be able to test non-trivial fixes to the Gaia code.
Flags: needinfo?(drs.bugzilla)
(In reply to :Paolo Amadini from comment #1)
> It seems "installHandlers" is called after "dial" or "dialEmergency" already
> returned. This means that the dialing procedure may, technically, attempt to
> call the hooks before they are placed in the "call" object. In practice this
> may not happen because of current timing (both Promise timing and internal
> "dial" implementation details), but it clearly seems unsafe.

I mean, this looks like an API design issue... not sure whether it can be solved.
Ah, this tryserver build can be used to reproduce the failures:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3d9cc6560612
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Flags: needinfo?(drs.bugzilla)
Attachment #8510373 - Flags: review?(drs.bugzilla)
Comment on attachment 8510373 [details] [review]
Avoiding using "catch" as a way to do "setTimeout(0)"

Sorry for the delay, and thanks for making this change. I can't think of a better way to do this either.

I left a couple of nits on the PR, but this looks good to me.
Attachment #8510373 - Flags: review?(drs.bugzilla) → review+
Attached file Nits addressed
Attachment #8510373 - Attachment is obsolete: true
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/d8c3e1859c399159bf17fabd112a99409406852f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.