906203 - unit tests fail with TypeError: TonePlayer is null

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently calllog_db_test.js fails consistently with:

     Error: TypeError: TonePlayer is null (http://communications.gaiamobile.org:8080/dialer/js/calls_handler.js?time=1376688809076:281)

This appears to just need tone_player.js required at the top of the test.  Will add pull request shortly.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Pull request at https://github.com/mozilla-b2g/gaia/pull/11583

This simply requires tone_player.js to ensure that TonePlayer is defined for the code under test.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Looks like the PR works in the browser locally, but not on travis:

 Error: NS_ERROR_FAILURE: Failure (http://communications.gaiamobile.org:8080/dialer/js/tone_player.js?time=1376690165164:73)

at onerror (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4959)

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Updated to use mock_tone_player.js.  This works locally, but I don't see where MockTonePlayer gets set as TonePlayer.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Ok, one more update and I think this gets rid of the error on travis.  Sorry for the churn.

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 791487 [details]
Pull request at https://github.com/mozilla-b2g/gaia/pull/11583

The code under test (CallLogDBManager) has no reason at all to access the TonePlayer...
So I don't want to mock the TonePlayer without understanding what's going on.

Can't reproduce the issue, but it's probably coming from elsewhere.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

No problem Etienne!  I fully admit I don't know the dialer code.

Are you running the single test file or the entire communications suite?  Its possible a component is getting initialized in an earlier test file which then causes TonePlayer to get referenced later.

By the way, as of Friday on master we can now run communications suite by itself using 'make test-agent-test APP=communications'.  Hope that helps!

Comment 7

[Etienne Segonzac (:etienne)]

(In reply to Ben Kelly [:bkelly] from comment #6)
> No problem Etienne!  I fully admit I don't know the dialer code.
> 
> Are you running the single test file or the entire communications suite? 

Ran the single test file and the dialer suite with no issues.

> By the way, as of Friday on master we can now run communications suite by
> itself using 'make test-agent-test APP=communications'.  Hope that helps!

Just tried this. All green!

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Etienne Segonzac (:etienne) from comment #7)
> > By the way, as of Friday on master we can now run communications suite by
> > itself using 'make test-agent-test APP=communications'.  Hope that helps!
> 
> Just tried this. All green!

Hmm, I just ran on fresh master got a failure.  :-\

I wonder if the difference is due to the test order being somewhat random.  See bug 899725.

The order of top level tests on my machine running Ubuntu 12.04 is:

  [communications-dialer] dialer/call_log
  [communications-dialer] dialer/keypad
  [communications-dialer] suggestion Bar
  [communications-dialer] telephony helper
  [communications-dialer] lib/simple_phone_matcher
  [communications-dialer] calls handler
  [communications-dialer] LazyL10n
  [communications-dialer] dialer/mmi
  [communications-dialer] dialer/call_log_db

There are more tests after that, but that is where the failure occurs.

Etienee, do you get the same order on your machine?

Comment 9

[Etienne Segonzac (:etienne)]

(In reply to Ben Kelly [:bkelly] from comment #8)
> (In reply to Etienne Segonzac (:etienne) from comment #7)
> > > By the way, as of Friday on master we can now run communications suite by
> > > itself using 'make test-agent-test APP=communications'.  Hope that helps!
> > 
> > Just tried this. All green!
> 
> Hmm, I just ran on fresh master got a failure.  :-\
> 
> I wonder if the difference is due to the test order being somewhat random. 
> See bug 899725.
> 
> The order of top level tests on my machine running Ubuntu 12.04 is:
> 
>   [communications-dialer] dialer/call_log
>   [communications-dialer] dialer/keypad
>   [communications-dialer] suggestion Bar
>   [communications-dialer] telephony helper
>   [communications-dialer] lib/simple_phone_matcher
>   [communications-dialer] calls handler
>   [communications-dialer] LazyL10n
>   [communications-dialer] dialer/mmi
>   [communications-dialer] dialer/call_log_db
> 
> There are more tests after that, but that is where the failure occurs.
> 
> Etienee, do you get the same order on your machine?

Nope.

  [communications-dialer] dialer/voicemail
  [communications-dialer] dialer/utils
  [communications-dialer] lib/simple_phone_matcher
  [communications-dialer] dialer/call_log_db
  ... the rest

Tried the patch in bug 899725 (in tools/test-agent/node_modules/test-agent) but I still get the same order :/

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Etienne Segonzac (:etienne) from comment #9)
> Tried the patch in bug 899725 (in tools/test-agent/node_modules/test-agent)
> but I still get the same order :/

Hmm, I had to apply it to:

  test_apps/test-agent/common/vendor/test-agent/test-agent.js

I don't know why we have the node_module and a version committed to our repo.

I think ordering might have caused the TonePlayer error to move to a different test case on my machine as well.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Also, in case it matters, I am running the tests in nightly browser built last Friday.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Unassigning since my naive implementation is apparently not the way to go.  I'm hoping we can track this down once bug 899725 is fixed.

Comment 13

[Etienne Segonzac (:etienne)]

on it

Comment 14

[Etienne Segonzac (:etienne)]

Attachment: Pointer to gaia PR

The keypad_test.js was all kind of wrong mocking-wise. And the KeypadManager is the most heavy user of the TonePlayer (who wasn't mocked in the test!).

Switched the test file to using mocksHelper and cleaned it up a bit.

Should be much better.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Thanks Etienne!

Comment 16

[Anthony Ricaud (:rik)]

Comment on attachment 793462 [details]
Pointer to gaia PR

I couldn't reproduce the test failure without the patch so I don't know if it fixes it. Plus Travis is broken at the moment.

But the patch makes sense so r+.

Comment 17

[Etienne Segonzac (:etienne)]

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