1075770 - [Contacts] Lots of tests depends on the mozContacts API

Status: RESOLVED WONTFIX

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

Description

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

In bug 1043562 we want to remove the mozContact API from places where it should not be exposed.

In bug 1075100 we try alternative ways to grant the permission in unit tests but it doesn't work in B2G Desktop so far. Therefore the only way is to remove the dependency in the unit tests by using a proper mock.

Note that we have such a mock in shared/test/unit/mocks but I don't know if it's good. We also have something bad in SMS, see [1].

[1] https://github.com/mozilla-b2g/gaia/blob/191d805f4911628d37a8a90a1e23a6013995138f/apps/sms/test/unit/contacts_test.js#L33-L193

You can see the failing tests in [2].

[2] https://tbpl.mozilla.org/php/getParsedLog.php?id=49313960&tree=Try

Francisco, can you please have a look at this? This is preventing Ehsan from landing bug 1043562 (unless he finds how to give the contacts perm in bug 1075100 in B2G Desktop...). Thanks!

Comment 1

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

To reproduce the issue locally, you can run the unit tests in a build from this try push: https://tbpl.mozilla.org/?tree=Try&rev=185bc3597224 (click on the "B", then on "go to build directory" in the bottom left, then take the .tar.bz2 archive).

Comment 2

[Francisco Jordano [:arcturus] [:francisco]]

Hi Julien,

thanks for sharing, fortunately we use a mock as well, after seeing the tests that fails I'm a bit less afraid.

Will try to solve this during this sprint.

Comment 3

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Pointer to PR 25149

Jose I did the modifications and tried with a Firefox from here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-185bc3597224/try-macosx64/

I'm seeing some errors that are not related to the mozContacts api, or at least couldn't see the direct relationship.

Comment 4

[Jose Manuel Cantera]

Comment on attachment 8504928 [details] [review]
Pointer to PR 25149

the code looks good to me. I've also run the comms tests locally and worked perfectly. Treeherder seems to be ok as well. 

What I've not tested is to disable the mozContacts API on the browser and see what happens. I leave that final double check to you :)

thanks!

Comment 5

[Francisco Jordano [:arcturus] [:francisco]]

Just realised that the build directory containing the patch for bug 1043562 dissapeared.

Submiting this patch for review against current firefox

Comment 6

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #5)
> Just realised that the build directory containing the patch for bug 1043562
> dissapeared.

You can ask ehsan to push a new try, if you want.

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

Eshan could you push a new try for your patch in bug 1043562

Comment 8

[(no longer active)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #7)
> Eshan could you push a new try for your patch in bug 1043562

https://tbpl.mozilla.org/?tree=Try&rev=185bc3597224 is still there.  Do you still want me to push to try again?

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #8)
> (In reply to Francisco Jordano [:arcturus] [:francisco] from comment #7)
> > Eshan could you push a new try for your patch in bug 1043562
> 
> https://tbpl.mozilla.org/?tree=Try&rev=185bc3597224 is still there.  Do you
> still want me to push to try again?

Yes please :)

Comment 10

[Jose Manuel Cantera]

Comment on attachment 8504928 [details] [review]
Pointer to PR 25149

thanks Francisco

Comment 11

[(no longer active)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #9)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #8)
> > (In reply to Francisco Jordano [:arcturus] [:francisco] from comment #7)
> > > Eshan could you push a new try for your patch in bug 1043562
> > 
> > https://tbpl.mozilla.org/?tree=Try&rev=185bc3597224 is still there.  Do you
> > still want me to push to try again?
> 
> Yes please :)

https://tbpl.mozilla.org/?tree=Try&rev=4964f590275e

Comment 12

[(no longer active)]

(Note that I'm not sure what you expect to have changed...)

Comment 13

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Ehsan, your older try is there, but the build directories for this old try are absent.

Comment 14

[Francisco Jordano [:arcturus] [:francisco]]

Ehsan, can you provide the build with the patch so we can double check against it?

Comment 15

[(no longer active)]

Sure: <http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-4964f590275e/try-linux64_gecko/>

FWIW you can get builds from TBPL by clicking on a "B" job and selecting "go to build directory".

Comment 16

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #15)
> Sure:
> <http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-
> 4964f590275e/try-linux64_gecko/>
> 
> FWIW you can get builds from TBPL by clicking on a "B" job and selecting "go
> to build directory".

Thanks, we have the build there but the directory was empty, now we can download again a version with the patch.

Comment 17

[Francisco Jordano [:arcturus] [:francisco]]

\o/ got a green build with the gecko from that try build.

Rebased and waiting for a green build as well in gaia-try before merging.

Comment 18

[Francisco Jordano [:arcturus] [:francisco]]

Landed:

https://github.com/mozilla-b2g/gaia/commit/33af5fd1bbb459e1f7f1f8ea4f67a4e0815626cd

Comment 19

[(no longer active)]

Francisco, was your patch supposed to fix all of these test failures?  I'm still getting a ton of Gu test failures on try:

https://tbpl.mozilla.org/php/getParsedLog.php?id=51452325&tree=Try&full=1
https://tbpl.mozilla.org/?tree=Try&rev=bb3c2276c163

Comment 20

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Mmm some are in the SMS app, I don't think they were showing up in your previous try builds :/

Comment 21

[Francisco Jordano [:arcturus] [:francisco]]

Hi Ehsan,

With that patch we passed all the unit test in contacts with a previous try-build firefox.
I'm wondering if we are having some intermittent failures and we didn't detected.

Will reopen the bug to try the latest try-build that you copied here.

Comment 22

[(no longer active)]

Thanks guys.  I hope to retry landing my stuff in three weeks when I'm back from vacation. :)

Comment 23

[BMO Automation]

Firefox OS is not being worked on