1062798 - Replace the custom functions to load JSON files with the new shared helper in the settings app

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #990047 +++

There's a few places in the settings app that can be converted to use the new helper:

https://github.com/mozilla-b2g/gaia/blob/eced8a617b6fde7962e1746f4e65336aaddd86c3/apps/settings/js/panels/feedback_send/feedback_send.js#L110
https://github.com/mozilla-b2g/gaia/blob/72605f80ed42cb3d435c6ef5adac9e5199894316/apps/settings/js/panels/search/search.js#L44
https://github.com/mozilla-b2g/gaia/blob/3a49fbbf0834d0155b7837100d670b4c3444d6db/apps/settings/js/modules/apn/apn_utils.js#L187
https://github.com/mozilla-b2g/gaia/blob/5558ad0b068b5ddf45d7a532e133ab88a47f0beb/apps/settings/js/utils.js#L151

We might need to slightly extend the shared helper to better cover the functionality here (e.g. providing options for the request timeout).

Comment 1

[Robbie MacKay]

Hi Gabriele,

I'm just having a go at doing this. I'm assumine the 'new helper' mean LazyLoader.getJSON() ?
That looks like its only going to work for simple GET requests.

- The feedback_send code highlighted is doing a POST with data..
https://github.com/mozilla-b2g/gaia/blob/eced8a617b6fde7962e1746f4e65336aaddd86c3/apps/settings/js/panels/feedback_send/feedback_send.js#L110

- The search panel change looks simple.
- Both the apn_utils and utils code mentioned are helper functions.. would I be better to keep those functions and call LazyLoader.getJSON() from them? or replace all the calls to those helpers throughout the settings app?

Thanks

Comment 2

[Gabriele Svelto [:gsvelto]]

(In reply to Robbie MacKay from comment #1)
> I'm just having a go at doing this. I'm assumine the 'new helper' mean
> LazyLoader.getJSON() ?

Yes.

> That looks like its only going to work for simple GET requests.
> 
> - The feedback_send code highlighted is doing a POST with data..
> https://github.com/mozilla-b2g/gaia/blob/
> eced8a617b6fde7962e1746f4e65336aaddd86c3/apps/settings/js/panels/
> feedback_send/feedback_send.js#L110

My bad, that shouldn't be replaced, we'll think about a helper for POST requests if it makes sense at a later time.

> - Both the apn_utils and utils code mentioned are helper functions.. would I
> be better to keep those functions and call LazyLoader.getJSON() from them?
> or replace all the calls to those helpers throughout the settings app?

Replace them with the helper, they don't add any value so wrapping the helper within them doesn't make much sense.

Comment 3

[Robbie MacKay]

Attachment: Pull request for bug 1062798

Added a PR. I've tested it manually w/ Firefox Nightly. Haven't run it on my flame or figured out how to run the automated tests yet.

Comment 4

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8489161 [details] [review]
Pull request for bug 1062798

I'm not a settings peer so I can't r+ this code but you can ask me for feedback and naturally I'm glad to help. For a proper review when you'll feel confident enough ask one of the Settings peers, you can find them here:

https://wiki.mozilla.org/Modules/All#Settings

Or use the 'suggested reviewers' field in Bugzilla.

Now on to your patch: I've left a few comments about relatively minor stuff but there's a couple more things that don't fit in there and are the reason why I gave you a negative feedback. The first test in panel_test.js failed and second one in support_test.js. Additionally the findmydevice_panel_test.js set of tests didn't run at all as setup is broken.

Documentation on running the unit-tests is available here:

https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests

In a nutshell you'll have to do the following:

1) Set the FIREFOX environment variable to point to nightly, e.g.

  $ export FIREFOX=/opt/firefox-nightly/firefox

2) Run the gaia-test script in the bin directory under gaia:

  $ ./bin/gaia-test

3) Wait for it to settle, it might take a minute the first time while it's downloading stuff

4) In another window from gaia's root run the unit-tests for the settings app using the following command

  $ make test-agent-test APP=settings

Alternatively once you're past point 3 every time you'll change a source file in the settings app the relevant unit-tests will run automatically. Just make sure you don't close the instance of nightly you've launched at point 2.

Comment 5

[Robbie MacKay]

Thanks. Can you assign the bug to me? 
I've made some progress on the tests. I'll take another look tonight or tomorrow.

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to Robbie MacKay from comment #5)
> Thanks. Can you assign the bug to me? 

Done, sorry about that, I was convinced I already did :-P

> I've made some progress on the tests. I'll take another look tonight or
> tomorrow.

Excellent, thanks for your help here.

Comment 7

[Robbie MacKay]

I've managed to fix up most of the tests. However it looks like apps/settings/test/marionette/tests/display_settings_test.js is failing. I need to mock out LazyLoader in that test, but I'm not sure how I do that within a marionette test. Any pointers? or an example I could look at?

Comment 8

[Gabriele Svelto [:gsvelto]]

(In reply to Robbie MacKay from comment #7)
> I've managed to fix up most of the tests. However it looks like
> apps/settings/test/marionette/tests/display_settings_test.js is failing. I
> need to mock out LazyLoader in that test, but I'm not sure how I do that
> within a marionette test. Any pointers? or an example I could look at?

Have you tried loading the LazyLoader code directly instead of mocking it along with other requirements?

https://github.com/mozilla-b2g/gaia/blob/89d2418167e62f792090d7efd427e75f8b86c275/apps/settings/test/marionette/app/app.js#L3

Alternatively you can create an ad-hoc mocked object like it's already done for the loadJSON() function:

https://github.com/mozilla-b2g/gaia/blob/89d2418167e62f792090d7efd427e75f8b86c275/apps/settings/test/marionette/tests/display_settings_test.js#L22

Or a complete mock if you feel like it, see some examples here:

https://github.com/mozilla-b2g/gaia/tree/89d2418167e62f792090d7efd427e75f8b86c275/apps/settings/test/marionette/mocks

If you go for the complete mock consider adding it to the shared integration tests code as it could be used in other places too.

FYI marionette documentation is available here:

https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_integration_tests

Comment 9

[Robbie MacKay]

Still struggling to figure out the integration test 

> Have you tried loading the LazyLoader code directly instead of mocking it along with other requirements?

How would that help? its not a matter of it getting loaded or not. Its that I need to intercept the request to fake the '/resources/device-features.json' response

> Alternatively you can create an ad-hoc mocked object like it's already done for the loadJSON() function

I don't think I can.. because LazyLoader isn't global, its loaded with require() so I don't have access to replace it.

Comment 10

[Julien Wajsberg [:julienw]]

(In reply to Robbie MacKay from comment #9)
> Still struggling to figure out the integration test 
> 
> > Have you tried loading the LazyLoader code directly instead of mocking it along with other requirements?
> 
> How would that help? its not a matter of it getting loaded or not. Its that
> I need to intercept the request to fake the
> '/resources/device-features.json' response

If it's using XHR, maybe you can use sinon's fake XHR feature? See http://sinonjs.org/docs/#server for more information :)

> 
> > Alternatively you can create an ad-hoc mocked object like it's already done for the loadJSON() function
> 
> I don't think I can.. because LazyLoader isn't global, its loaded with
> require() so I don't have access to replace it.

I'm not sure, but maybe you can hack around setup.js. I didn't really dig into unit tests for require-based apps yet so I can't help much more :(

Comment 11

[Gabriele Svelto [:gsvelto]]

(In reply to Robbie MacKay from comment #9)
> I don't think I can.. because LazyLoader isn't global, its loaded with
> require() so I don't have access to replace it.

Good point; I think that Julien's suggestion of using sinon's fake XHRs is the only option here.

Comment 12

[Robbie MacKay]

I've had a couple of goes at this but I'm still stuck for a solution. 

AFAIK I can't use sinon's fake xhr because its an integration test, not a unit test. I managed to mock out LazyLoader just fine for unit tests.

The best I can come up with is that I need to inject a fake LazyLoader using a requirejs define() call.. but if I use marionette client.executeScript() its coming in before requirejs is loaded.

Either that or I just need to modify device-features on disk somehow, but can't see a way to do that either.

Comment 13

[Julien Wajsberg [:julienw]]

Ah yeah, right, it's a marionette test.

Can you explain why you need to modify the json? I actually don't get why the marionette test should need to be modified here, can you explain why it's failing?

Comment 14

[Robbie MacKay]

There are 2 tests:
- "show the toggle when with light sensor"
- "hide the toggle when without light sensor"

device-features.json has a setting for if the device has a light sensor { ambientLight: true }
If the sensor is enabled the light sensor is visible.
If the sensor is disabled the light sensor is hidden.

The 2 tests rely on opposite states for the ambientLight setting, so 1 test always fails unless we alter the response. Previously the JSON response done through a global getJSON method, so the test just faked that mathod to fake the response.

Comment 15

[Julien Wajsberg [:julienw]]

Thanks, that's very clear!

Maybe it's possible to replace LazyLoader.getJSON before starting the test?

Ok, let's ask Arthur what he thinks, he might have a good idea.

Comment 16

[Arthur Chen [:arthurcc][inactive after 6/30]]

Although LazyLoader is loaded with require() but the script still exports a LazyLoader object on the window object. So I think you should be able to replace LazyLoader.getJSON with the following script:

client.executeScript(function() {
  var theWindow = window.wrappedJSObject;
    theWindow.LazyLoader.getJSON = function(path) {
      return Promise.resolve({ ambientLight: true });
    };
  });
});

Comment 17

[Robbie MacKay]

Oh.. that's much simpler than I thought. Guess I over thought it. Thanks Arthur!

Comment 18

[Robbie MacKay]

Comment on attachment 8489161 [details] [review]
Pull request for bug 1062798

Pretty sure all the tests pass now. So ready for another review.

Comment 19

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8489161 [details] [review]
Pull request for bug 1062798

I've made some comments in github mainly regarding unit tests. Please check them, thanks!

Comment 20

[Robbie MacKay]

Attachment: 2nd PR for bug 1062798

Comment 21

[Robbie MacKay]

Comment on attachment 8515681 [details] [review]
2nd PR for bug 1062798

Finally updated my PR for this. I'm pretty sure it all works.
There's a failure in gaia-try but it looks unrelated to this.
Can you give it a review?

Comment 22

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8515681 [details] [review]
2nd PR for bug 1062798

Thanks for the effort! There are a few comments to be addressed before merging, please check them in github.

Comment 23

[Robbie MacKay]

Comment on attachment 8515681 [details] [review]
2nd PR for bug 1062798

Updated based on your previous review. Could you give this another look? 
Thanks!

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8515681 [details] [review]
2nd PR for bug 1062798

Remove the wrap to the functions and we are good to go! Please check my comments, thanks!

Comment 25

[Robbie MacKay]

Updated to remove some of the wrappers, but see my comment about the find my device tests: https://github.com/mozilla-b2g/gaia/pull/24791#discussion_r19926456

Comment 26

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8515681 [details] [review]
2nd PR for bug 1062798

r=me with the reject function removed. Please squash the commits and mark checkin-needed or needinfo me, thank you!

Comment 27

[Robbie MacKay]

Squashed and ready to land. Thanks!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/7278f26d51a387db39c87b5389bb584746a762a4