892153 - Create base unit test for Gaia System Customization work

Status: RESOLVED FIXED

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

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Most System Customization work hinges on the sim card being switched out, which changes the MNC/MCC setting. It'd be nice to have a way to simulate this change in a marionette case, both for the emulator and for desktop B2G.

There's already some work on this done in the mobileconnection API, see

http://dxr.mozilla.org/mozilla-central/source/dom/network/tests/marionette/test_mobile_iccinfo.js

However most of this relies on the emulator's RIL mocking capabilities, which we don't have available in desktop.

Comment 1

[Aus Lacroix [:aus]]

It looks like we can key off all customizations from by looking at this pref (and observing it for changes): "ril.lastKnownHomeNetwork". Hooray!

Comment 2

[Aus Lacroix [:aus]]

Going to take a stab at a basic helper object to detect whether customizations should be applied. This helper will be the first piece to receive a unit-test. This should ensure that all other customizations can be triggered as expected.

Comment 3

[Aus Lacroix [:aus]]

RFC -- https://github.com/nullaus/gaia/compare/market-customizations

A helper object that can be used to trigger customizations for any application. Based on apps/system/js/operator_variant.js. Let me know what you guys think about this approach.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Looks like it might be missing the listener argument for function OperatorVariantListener?

Seems to make sense other than that, but I'm just about the last person that should be reviewing js. :)

Comment 5

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - OperatorVariantListener and Base Unit Tests

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 783378 [details]
Patch - v1 - OperatorVariantListener and Base Unit Tests

I've added my comments on github regarding the tests part, and some small comments on the actual code.

I don't feel I'm very qualified on this code (besides the tests) so I'm asking :timdream a review too.

Please ask me review again when my comments are fixed :)

Comment 7

[Candice Serran (:cserran)]

Ghislain 'Aus' Lacroix changed story state to started in Pivotal Tracker

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 783378 [details]
Patch - v1 - OperatorVariantListener and Base Unit Tests

Sorry for the long delay ... I was loaded.

Many of the SIM-based customization involves setting a user-change-able default to the app. I wonder if the help library serves that well? I mean, if the user changes her/his SIM, does that really means we could overwrite whatever value he/she had already set implicitly?

Obviously the question should be asked when an app is actually using the library, so I am setting the r+ anyway.

Comment 9

[Aus Lacroix [:aus]]

Hi Tim! Thanks for the review.

The applications can opt to do what they like. For our own stack of applications we will only apply the customizations _once_ EXCEPT in the case of Data, MMS, A-GPS and similar configuration items that are very dependent on the network the user is on.

That's why the helper itself supports both a one time check at run-time as well as the ability to listen for further changes in SIM cards.

Hope that clears things up. :)

Comment 10

[Aus Lacroix [:aus]]

Hi Julien,

I've updated a lot of the tests to use the latest and greatest mocks helper. I think we're good to go as long as you approve. :)

Comment 11

[Aus Lacroix [:aus]]

Work for this is complete, merged to master.

Comment 12

[Candice Serran (:cserran)]

Ghislain 'Aus' Lacroix changed story state to finished in Pivotal Tracker

Comment 13

[Candice Serran (:cserran)]

Ghislain 'Aus' Lacroix changed story state to delivered in Pivotal Tracker

Comment 14

[Jason Smith [:jsmith]]

(In reply to Ghislain 'Aus' Lacroix from comment #11)
> Work for this is complete, merged to master.

Can you include a commit and move this to resolved fixed then?

Comment 15

[Aus Lacroix [:aus]]

Resolved Fixed

Commit: https://github.com/nullaus/gaia/commit/510a4be9c0aa06b9cf2aa59297155c414b0137f4

Comment 16

[Julien Wajsberg [:julienw]]

Ghislain> you specifically asked me a review for the test part, therefore you should not merge without my last review, even if Tim gave you r+ for the Operator Variant part.

You can merge after doing changes if the reviewer says "r+ with nits" but that's the only case.

I had a look and the patch looks ok to me but I'd appreciate that you wait next time. I really considered backing out the patch, you know :)

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 783378 [details]
Patch - v1 - OperatorVariantListener and Base Unit Tests

r=me for the test part

Comment 18

[Julien Wajsberg [:julienw]]

master merge commit is 43557e5392ab37d4f8c1fa08184cf1541b249a54

Comment 19

[Candice Serran (:cserran)]

Candice Serran changed story state to accepted in Pivotal Tracker