1062783 - Replace the loadFile() function used to load JSON files with the new shared function in the homescreen app

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

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

This function can now be replaced with the new shared helper:

https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/js/configurator.js#L22

Comment 1

[Gaurav Mittal (gauravmittal1995)]

Hey... I would like to work on this bug. Can you assign it to me please?

Comment 2

[[:fabrice] Fabrice Desré]

Thanks for taking this!

Comment 3

[Gaurav Mittal (gauravmittal1995)]

Here, How can i handle the args success, error??

Comment 4

[Kevin Grandon :kgrandon]

(In reply to gauravmittal from comment #3)
> Here, How can i handle the args success, error??

Which error specifically? Are you talking about the try/catch statements?

Comment 5

[Gaurav Mittal (gauravmittal1995)]

i meant, how to i give the parameters "success" and "errors" ( https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/js/configurator.js#L22 ) to "resolve" and "reject" ( https://github.com/mozilla-b2g/gaia/blob/master/shared/js/lazy_loader.js#L75 )

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to gauravmittal from comment #5)
> i meant, how to i give the parameters "success" and "errors" (
> https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/js/
> configurator.js#L22 ) to "resolve" and "reject" (
> https://github.com/mozilla-b2g/gaia/blob/master/shared/js/lazy_loader.js#L75
> )

You can obtain the same semantics of loadFile(name, success, errors) using LazyLoader.loadJSON(name).then(success, errors)

The first  argument to then() is invoked when the promise returned by loadJSON() is resolved and the second argument when it's rejected.

Comment 7

[Gaurav Mittal (gauravmittal1995)]

Attachment: PR for Bug

Comment 8

[Gaurav Mittal (gauravmittal1995)]

i am getting a Gu error in https://tbpl.mozilla.org/?rev=bada13a9b54bc4d07d0d6edb8ff8b9cfc00d8d38&tree=Gaia-Try ... can someone please help me understand why this is hapening?

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8489725 [details] [review]
PR for Bug

If you look in the test logs (https://tbpl.mozilla.org/php/getParsedLog.php?id=48130437&tree=Gaia-Try), you'll see the following error: 15:25:32     INFO -  JavaScript error: app://verticalhome.gaiamobile.org/js/configurator.js?time=1410819932571, line 181: ReferenceError: LazyLoader is not defined

It looks like you can just include the lazy_loader.js file within the test and we should be fine.

Comment 10

[Gaurav Mittal (gauravmittal1995)]

Ok, i found out that if i reverted all my changes, it still had a lot of failed tests!!! Almost all the tests which were failing after changing were failing before failing!

Can anyone please look into this?

Comment 11

[Kevin Grandon :kgrandon]

I think that this is due to the mockXHR and the fact that lazyloader is now returning a promise. My guess is that you would want to stub out the promise, maybe using sinon? Let me know if that helps. I might be able to pick up the testing part if this if you don't have much luck.

Have you been able to get this test running locally yet?

https://github.com/gauravmittal1995/gaia/blob/1062783/apps/verticalhome/test/unit/configurator_test.js#L85

Comment 12

[Gabriele Svelto [:gsvelto]]

(In reply to gauravmittal from comment #10)
> Ok, i found out that if i reverted all my changes, it still had a lot of
> failed tests!!! Almost all the tests which were failing after changing were
> failing before failing!

You mean the integration tests? Then there might be something wrong with your setup. The tests should be all working by default. Did you follow the instructions here to set up the tests?

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

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Running_Tests

Comment 13

[Gaurav Mittal (gauravmittal1995)]

@gsvelto .... i mean the ./gaia-test in the path/to/gaia/bin folder .... !!

Comment 14

[Gaurav Mittal (gauravmittal1995)]

@gsvelto .... i mean the ./gaia-test in the path/to/gaia/bin folder .... !!

Comment 15

[Gabriele Svelto [:gsvelto]]

(In reply to gauravmittal from comment #14)
> @gsvelto .... i mean the ./gaia-test in the path/to/gaia/bin folder .... !!

Those are the unit-tests and should all be working. Which version of Firefox are you using for running the tests? You should be running nightly to ensure it's up to date with the latest gaia, see the documentation here:

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

Comment 16

[Gaurav Mittal (gauravmittal1995)]

(In reply to Kevin Grandon :kgrandon from comment #11)
> I think that this is due to the mockXHR and the fact that lazyloader is now
> returning a promise. My guess is that you would want to stub out the
> promise, maybe using sinon? Let me know if that helps. I might be able to
> pick up the testing part if this if you don't have much luck.

Hmm, I am not sure how to stub the promise, and i wish to learn it. Can u please guide me in this?

Comment 17

[Kevin Grandon :kgrandon]

Hey - did you read through this thread? I think there might be some useful tips in there. https://groups.google.com/forum/#!topic/mozilla.dev.gaia/yn7waOIfKFQ

I'll also try to dig into the code if I have time.

Comment 18

[Kevin Grandon :kgrandon]

Comment on attachment 8489725 [details] [review]
PR for Bug

We should fix the tests before putting this into review.

Comment 19

[Julien Wajsberg [:julienw]]

Here, I think you want to stub LazyLoader to return promises.

Something like this in a setup function should work:

  var json = "your json data";
  this.sinon.stub(LazyLoader, 'loadFile').returns(Promise.resolve(json));

Grep on "sinon" in the unit tests and you should have plenty of examples :)

Comment 20

[Kevin Grandon :kgrandon]

Thanks Julien! gauravmittal - give that a shot and see if it fixes the unit tests for you :)

Comment 21

[Gaurav Mittal (gauravmittal1995)]

(In reply to Kevin Grandon :kgrandon from comment #20)
> Thanks Julien! gauravmittal - give that a shot and see if it fixes the unit
> tests for you :)

do u mean http://pastebin.mozilla.org/6605646 ?? if so .. what value of "loadFile" and var json can i use in  https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/test/unit/configurator_test.js#L81L90 ??

Comment 22

[Kevin Grandon :kgrandon]

(In reply to gauravmittal from comment #21)
> (In reply to Kevin Grandon :kgrandon from comment #20)
> > Thanks Julien! gauravmittal - give that a shot and see if it fixes the unit
> > tests for you :)
> 
> do u mean http://pastebin.mozilla.org/6605646 ?? if so .. what value of
> "loadFile" and var json can i use in 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/test/unit/
> configurator_test.js#L81L90 ??

I don't think this is quite correct because you still have the xhr sitting around. I'm not sure it's needed?

Comment 23

[Kevin Grandon :kgrandon]

Attachment: Pull request - /w test updates

Hey Gauravmittal - Since you're not so familiar with the unit tests, it's probably not the best use of your time. I think that there will be plenty of opportunities to learn the unit testing framework in the future, but I'd hate to see you spin your wheels for weeks on this one issue.

I've gone ahead and taken a look at the unit tests and made a commit which addresses the test issues. My recommendation is that you take a look, and review it (I'm delegating the review to you as I am a home screen owner), and if it looks good we can land both of our commits together. What do you think? If this looks good to you, you can change the review '?' mark to a '+', or a '-' if it still needs work.

Comment 24

[Gaurav Mittal (gauravmittal1995)]

@kevin
Yes, i think this solves all the unit test problem. Thanks a lot for helping.

Comment 25

[Kevin Grandon :kgrandon]

Comment on attachment 8489725 [details] [review]
PR for Bug

Leaving an R+ on your patch, but will obsolete in favor of landing with the unit test fix.

Comment 26

[Kevin Grandon :kgrandon]

Both commits landed in master: https://github.com/mozilla-b2g/gaia/commit/c1a82ef23c8b989fb0eb821b6abf6f2c88b3de1d

Thank you for your contribution! Looking forward to working with you on more bugs in the future.