:Paolo Amadini Assignee
	Description • 10 years ago

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.

	:Paolo Amadini Assignee
	Comment 1 • 10 years ago

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.

	:Paolo Amadini Assignee
	Comment 2 • 10 years ago

(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.

	:Paolo Amadini Assignee
	Comment 3 • 10 years ago

Ah, this tryserver build can be used to reproduce the failures:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3d9cc6560612

	Doug Sherk (:drs) (inactive)
	Comment 5 • 10 years ago

Comment on attachment 8510373 [details]
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.

	Carsten Book [:Tomcat]
	Comment 7 • 10 years ago

https://github.com/mozilla-b2g/gaia/commit/d8c3e1859c399159bf17fabd112a99409406852f