Closed Bug 993961 Opened 6 years ago Closed 6 years ago

[Camera] Fix unit tests in B2G Desktop

Categories

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

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julienw, Assigned: julienw)

Details

Attachments

(1 file)

46 bytes, text/x-github-pull-request
dmarcos
: review+
Details | Review
STR:
* run "bin/gaia-test -d"
* "touch apps/camera/test/unit/lib/sounds_test.js"

Expected:
* green test

Actual:
  3) [camera-test/unit/lib/sounds_test.js] Sounds Sounds#createAudio() Should set the mozAudioChannel type:
     Error: expected false to be truthy
      at chaiAssert (http://camera.gaiamobile.org:8080/common/test/helper.js:33:1)
      at get (http://camera.gaiamobile.org:8080/common/vendor/chai/chai.js:352:1)
      at ok (http://camera.gaiamobile.org:8080/common/vendor/chai/chai.js:1225:5)
      at (anonymous) (http://camera.gaiamobile.org:8080/test/unit/lib/sounds_test.js:182:7)

Anthony, I think you got the same issue lately, can you help here?
Flags: needinfo?(anthony)
Yup I did it in bug 930364 by using mock_audio.

I tried to do the same for Gallery but somehow I can't load mocks_helper. I don't have the time to investigate why.
Flags: needinfo?(anthony)
Attached file github PR
Hey Diego,

the issue here is that on B2G Desktop the "mozAudioChannelType" seems to not stick.

The workaround is to use the existing mock for the Audio object. But this made another existing test fail because it was asserting that the audio instance was of type HTMLAudioElement, which it's not now that we use the mock. I changed this by checking the mock was actually called and that its "thisValue" equals the returned value. This should be equivalent to the previous test.

Now I don't know how you use to add mocks, I did this using the mocks helper we use in other apps, but please tell me if you use another way to do it.

I also noticed that you're creating a sinon sandbox; but the test-agent is providing one in this.sinon already, and it's automatically restored at the end of each test. I haven't changed anything about this.
Assignee: nobody → felash
Attachment #8405321 - Flags: review?(dmarcos)
Comment on attachment 8405321 [details] [review]
github PR

Hey David, would you please review this or find another reviewer?

Thanks ! :)
Attachment #8405321 - Flags: review?(dmarcos) → review?(dflanagan)
Attachment #8405321 - Flags: review?(dflanagan) → review-
r- because the unit tests is testing the implementation of the function and not that it returns a expected output for a given input. A change in the implementation of createAudio will likely break the unit test. Unit tests verify correctness of the API and must be independent on how the API is implemented internally
This is arguable but please propose something else as this is the best I could think of.
Added some possibilities on github. I'm very open to fix this the way you want :) But I want to fix this, because the other fixes are:
* disable the test (because we have no way to disable it for B2G Desktop only in Travis)... I bet it is already disabled on TBPL.
* fix the platform... which is not of high priority last time I asked Paul Adenot (last week).
Flags: needinfo?(dmarcos)
You just have to leave the test as it was and check for 

 assert.ok(audio instanceof window.Audio);

instead of HTMLAudioElement

The setup function for the test won't be necessary anymore
Comment on attachment 8405321 [details] [review]
github PR

Hey Diego,

I made the changes you proposed. I checked that the test runs fine on both B2G and Firefox.
Attachment #8405321 - Flags: review- → review?(dmarcos)
Flags: needinfo?(dmarcos)
Cool thanks! The patch looks good to me.
Attachment #8405321 - Flags: review?(dmarcos) → review+
master: 0ef016704332e9d11f91515fbe395aa60ee0cdc8

thanks !
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.