1062829 - Use the new shared helper for loading JSON files in the FTU app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[Gabriele Svelto [:gsvelto]]

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

We can replace the custom code used to load JSON file here with the new shared helper:

https://github.com/mozilla-b2g/gaia/blob/master/apps/ftu/js/tutorial.js#L379

Comment 1

[Mathieu Rhéaume]

Attachment: Github PR for Bug 1062829 with LazyLoader.

Hi,

I'm doing more LazyLoader conversion :). I hope I don't forget anything.

I did the following.

1. Updated the /* global */ tag to pass the JSLint Test
2. Used the LazyLoader to load the JSON data.
3. We don't need to raise the fail premise as it used in the old XHR code as LazyLoader does it inside the lib function...

Ran both Marionette and UI tests both passed.

Thanks for reviewing !

Comment 2

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8490570 [details] [review]
Github PR for Bug 1062829 with LazyLoader.

As with the other bug you've been working on you'll need an FTU peer to review this. I'll give you my feedback though so feel free to ask for it again until you feel ready for review.

The code is working but I'm giving this f- because there's still some things to address, see my comments in the PR for further detail.

Comment 3

[Mathieu Rhéaume]

Attachment: Github PR for Bug 1062829 with changes from code review

Hi,

I applied the changes proposed in the PR. 

1. Using the Then() callback to return the intent instead of a new intent.
2. Removed the configRequest variable and reset verification as it's not used anymore.
3. Squashed all my commits into one.

The only thing that I'm not quite sure is the code formatting with the .Then at line 379.

Thanks for reviewing !

Comment 4

[Julien Wajsberg [:julienw]]

hey Mathieu,

you can find the list of peers for modules in [1]. For the "first time experience" application, you can see the current owner is Sam Foster, and peers include Borja, Fernando or Francisco. You can ask review from anyone of them. As Gabriele explained, he can give you feedback, but he can't do a proper review.

[1] https://wiki.mozilla.org/Modules/FirefoxOS

Comment 5

[Mathieu Rhéaume]

Hi,

I modified my current attachment to put svelto as Feedback and added Sam Foster as Review.

I will go modify some other bug that I need review from owner too.

Thanks for the list of contributors in the wiki. 

(In reply to Julien Wajsberg [:julienw] from comment #4)
> hey Mathieu,
> 
> you can find the list of peers for modules in [1]. For the "first time
> experience" application, you can see the current owner is Sam Foster, and
> peers include Borja, Fernando or Francisco. You can ask review from anyone
> of them. As Gabriele explained, he can give you feedback, but he can't do a
> proper review.
> 
> [1] https://wiki.mozilla.org/Modules/FirefoxOS

Comment 6

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8490987 [details] [review]
Github PR for Bug 1062829 with changes from code review

Looks good to me, thank you for helping with this bug.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8490987 [details] [review]
Github PR for Bug 1062829 with changes from code review

This patch makes the code much more testable (yay), but means we need to rework the tests to agree with the implementation. The tests use the MockXMLHttpRequest to supply different config data for different scenarios. Now we're using LazyLoader.getJSON() we should be able to get rid of the MockXMLHttpRequest entirely in that test file and use this.sinon.stub to supply the needed config data. 

I'm a little worried that by getting rid of the _configRequest we lose the ability to manage overlapping requests (we used to abort() them in reset method), but this was always a test artifact, so if Gaia-Try is green I'm happy - we'll just watch out for new intermittents.

Comment 8

[Mathieu Rhéaume]

Attachment: Github PR for Bug 1062829 with simon stub instaid of MockXMLHttpRequest

Hi Sam,

It is indeed simple to test stuff with sinon :).

I made the following changes :

1. Removed the MockXMLHttpRequest function, calls and the realXHR variable as it was related to the MockXMLHttpRequest function.

2. Implemented Sinon Stub for the test.

Thanks for looking into it !

Comment 9

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8492015 [details] [review]
Github PR for Bug 1062829 with simon stub instaid of MockXMLHttpRequest

Looks great, thank you!

Comment 10

[Sam Foster [:sfoster] (he/him)]

I've re-triggered Gu a few times to see if we get any whiff of intermittent failure. If that comes up green we're good to go here. 

https://tbpl.mozilla.org/?rev=1a49681ef58b8442d7f34daa7bcd1d972ec138cb&tree=Gaia-Try

Comment 11

[Sam Foster [:sfoster] (he/him)]

Calling that good. The patch is just refactoring in the FTU app - I can't see how the Gij failure in apps/calendar/test/marionette/day_view_test.js can be related. 

Merged to master: https://github.com/mozilla-b2g/gaia/commit/a416d1e2c1f82631de19ba476cb110304d0105c4
Commit: https://github.com/mozilla-b2g/gaia/commit/49ff41b8b60be0d389883634ca7fb0037fe5f37a

Comment 12

[Nigel Babu [:nigelb]]

Reverted for intermittent Gu oranges in https://github.com/mozilla-b2g/gaia/commit/5e180c2b53cc38fbba1f61d38807fe6f9380e974

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Here's the log to the intermittent Gu failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=48559342&tree=B2g-Inbound

Comment 14

[Mathieu Rhéaume]

Attachment: Github PR for Bug 1062829 with Mock call refactor in IAC Message Suite.

Hi Sam,

I investigated a bit the various gu failures and I was able to reproduce the problem on my desktop by spamming the exec button with bin/gaia-test.

It seems that the failure was a timeout in the first test of IAC Message suite.

I refactored the setup and teardown of to use the same method as the other test suite and added a call to the MockNavigatorSettings teardown method to make sure we teardown everything we setup.

I ran it about 50 times on my desktop and it didn`t trigger again.
If you could run a few times Gu on gaia-try to make sure it doens't go orange again, it would be great.

Thanks for looking into it.

Comment 15

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8492961 [details] [review]
Github PR for Bug 1062829 with Mock call refactor in IAC Message Suite.

I retriggered Gu a bunch of times, lets see how that goes. I left comments in the pull request to remove some unneeded done function arguments. Ultimately the answer to intermittents is to avoid asynchrony within tests where possible. If a setup/teardown/test doesnt need to wait to complete and reach a stable state, just remove it. 
Once we get that cleared up and the tests confirmed to pass we can call this good. I'm not r+ing only because we do need to do that clean up and see the results go green though before it can land.

Comment 16

[Mathieu Rhéaume]

Comment on attachment 8492961 [details] [review]
Github PR for Bug 1062829 with Mock call refactor in IAC Message Suite.

Hi Sam,

I updated the PR with the nits you asked.

Let's wait for gaia-try to go green and see how it goes from there.

Thanks for looking into it.

Comment 17

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8492961 [details] [review]
Github PR for Bug 1062829 with Mock call refactor in IAC Message Suite.

Comments in the PR. Gaia-Try came up green again, but I dont want to try to land this again until I'm more confident we have tackled all sources of intermittents introduced by this change. Sorry if this is becoming discouraging. Because the config is loaded as a side-effect of Tutorial.init, and many of the init and tutorial steps happen asynchronously, these tests have always been rather brittle. The LazyLoader.getJSON will be a big help, but we do need to nail down the impact of the change in each test. If you need some help figuring it out, maybe we can chat on IRC?

Comment 18

[Mathieu Rhéaume]

Comment on attachment 8492961 [details] [review]
Github PR for Bug 1062829 with Mock call refactor in IAC Message Suite.

Hi Sam,

I updated the PR following the discussion we had in IRC and recommendations from :gsvelto.

The following changes were made :

1. Implement LazyLoader GetJSON Stub to limit the async call in the IAC Call suite.
2. I had to had a .restore() stub in a previous test suite as LazyLoader GetJSON stub wasn't cleaned up correctly.

If there's anything more we could do please tell me i'll implement it.

Thanks for looking into it.

Comment 19

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8492961 [details] [review]
Github PR for Bug 1062829 with Mock call refactor in IAC Message Suite.

I like it, lets run with it. Thank you for your persistence!

Comment 20

[Sam Foster [:sfoster] (he/him)]

Merged to master: https://github.com/mozilla-b2g/gaia/commit/568fc850b42c24c649000bc3ec9e344e5e3bb6be
Commit: https://github.com/mozilla-b2g/gaia/commit/fa962141dcaab233fb06fea79475f6b8667fa74e