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

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gsvelto, Assigned: Robbie MacKay, Mentored)

Tracking

unspecified
2.1 S8 (7Nov)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(b2g-v2.2 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 1 obsolete attachment)

46 bytes, text/x-github-pull-request
arthurcc
: review+
Details | Review | Splinter Review
(Reporter)

Updated

3 years ago
Summary: Replace the custom functions to load JSON files with the new shared helper → Replace the custom functions to load JSON files with the new shared helper in the settings app
(Assignee)

Comment 1

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

Comment 2

3 years ago
(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.
(Assignee)

Comment 3

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

Comment 4

3 years ago
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.
Attachment #8489161 - Flags: review?(gsvelto) → feedback-
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

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

Comment 11

3 years ago
(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.
(Assignee)

Comment 12

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

Comment 14

3 years ago
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.
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.
Flags: needinfo?(arthur.chen)
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 });
    };
  });
});
Flags: needinfo?(arthur.chen)
(Assignee)

Comment 17

3 years ago
Oh.. that's much simpler than I thought. Guess I over thought it. Thanks Arthur!
(Assignee)

Comment 18

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

Pretty sure all the tests pass now. So ready for another review.
Attachment #8489161 - Flags: review?(arthur.chen)
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!
Attachment #8489161 - Flags: review?(arthur.chen)
(Assignee)

Comment 20

3 years ago
Created attachment 8515681 [details] [review]
2nd PR for bug 1062798
Attachment #8489161 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
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?
Attachment #8515681 - Flags: review?(arthur.chen)
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.
Attachment #8515681 - Flags: review?(arthur.chen)
(Assignee)

Comment 23

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

Updated based on your previous review. Could you give this another look? 
Thanks!
Attachment #8515681 - Flags: review?(arthur.chen)
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!
Attachment #8515681 - Flags: review?(arthur.chen)
(Assignee)

Comment 25

3 years ago
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 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!
Attachment #8515681 - Flags: review+
(Assignee)

Comment 27

3 years ago
Squashed and ready to land. Thanks!
Flags: needinfo?(arthur.chen)
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/7278f26d51a387db39c87b5389bb584746a762a4
status-b2g-v2.2: --- → fixed
Keywords: checkin-needed
Target Milestone: --- → 2.1 S8 (7Nov)
Flags: needinfo?(arthur.chen)
(Reporter)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.