unit tests fail with TypeError: TonePlayer is null

RESOLVED FIXED

Status

Firefox OS
Gaia::Dialer
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bkelly, Assigned: etienne)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ c= p=1 ])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Created attachment 791487 [details]
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.
Attachment #791487 - Flags: review?(etienne)
(Reporter)

Updated

4 years ago
Attachment #791487 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 2

4 years ago
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)
(Reporter)

Comment 3

4 years ago
Updated to use mock_tone_player.js.  This works locally, but I don't see where MockTonePlayer gets set as TonePlayer.
(Reporter)

Comment 4

4 years ago
Ok, one more update and I think this gets rid of the error on travis.  Sorry for the churn.
(Assignee)

Comment 5

4 years ago
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.
Attachment #791487 - Flags: review?(etienne) → review-
(Reporter)

Comment 6

4 years ago
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!
(Assignee)

Comment 7

4 years ago
(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!
(Reporter)

Comment 8

4 years ago
(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?
(Assignee)

Comment 9

4 years ago
(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 :/
(Reporter)

Comment 10

4 years ago
(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.
(Reporter)

Comment 11

4 years ago
Also, in case it matters, I am running the tests in nightly browser built last Friday.
(Reporter)

Updated

4 years ago
Depends on: 899725
(Reporter)

Comment 12

4 years ago
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.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 13

4 years ago
on it
Assignee: nobody → etienne
(Assignee)

Comment 14

4 years ago
Created attachment 793462 [details]
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.
Attachment #791487 - Attachment is obsolete: true
Attachment #793462 - Flags: review?(anthony)
(Reporter)

Comment 15

4 years ago
Thanks Etienne!
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+.
Attachment #793462 - Flags: review?(anthony) → review+
(Assignee)

Comment 17

4 years ago
https://github.com/mozilla-b2g/gaia/commit/abd81ff5e49bf0c9479cdc5c5eb6975cc776f632
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.