Closed
Bug 1062783
Opened 10 years ago
Closed 10 years ago
Replace the loadFile() function used to load JSON files with the new shared function in the homescreen app
Categories
(Firefox OS Graveyard :: Gaia::Homescreen, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: gsvelto, Assigned: gauravmittal1995, Mentored)
References
Details
(Whiteboard: [lang=js])
Attachments
(1 file, 1 obsolete file)
+++ 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
Reporter | ||
Updated•10 years ago
|
Summary: Replace the loadFile() function used to load JSON files with the new shared function → Replace the loadFile() function used to load JSON files with the new shared function in the homescreen app
Assignee | ||
Comment 1•10 years ago
|
||
Hey... I would like to work on this bug. Can you assign it to me please?
Updated•10 years ago
|
Assignee: nobody → gauravmittal1995
Comment 2•10 years ago
|
||
Thanks for taking this!
Assignee | ||
Comment 3•10 years ago
|
||
Here, How can i handle the args success, error??
Comment 4•10 years ago
|
||
(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?
Assignee | ||
Comment 5•10 years ago
|
||
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 )
Reporter | ||
Comment 6•10 years ago
|
||
(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.
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8489725 -
Flags: review?(kgrandon)
Attachment #8489725 -
Flags: review?(gsvelto)
Assignee | ||
Comment 8•10 years ago
|
||
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•10 years ago
|
||
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.
Attachment #8489725 -
Flags: review?(kgrandon)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(kgrandon)
Assignee | ||
Comment 10•10 years ago
|
||
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•10 years ago
|
||
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
Flags: needinfo?(kgrandon)
Reporter | ||
Comment 12•10 years ago
|
||
(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
Assignee | ||
Comment 13•10 years ago
|
||
@gsvelto .... i mean the ./gaia-test in the path/to/gaia/bin folder .... !!
Assignee | ||
Comment 14•10 years ago
|
||
@gsvelto .... i mean the ./gaia-test in the path/to/gaia/bin folder .... !!
Reporter | ||
Comment 15•10 years ago
|
||
(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
Status: NEW → ASSIGNED
Assignee | ||
Comment 16•10 years ago
|
||
(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?
Flags: needinfo?(kgrandon)
Comment 17•10 years ago
|
||
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.
Flags: needinfo?(kgrandon)
Comment 18•10 years ago
|
||
Comment on attachment 8489725 [details] [review] PR for Bug We should fix the tests before putting this into review.
Attachment #8489725 -
Flags: review?(gsvelto)
Comment 19•10 years ago
|
||
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•10 years ago
|
||
Thanks Julien! gauravmittal - give that a shot and see if it fixes the unit tests for you :)
Flags: needinfo?(gauravmittal1995)
Assignee | ||
Comment 21•10 years ago
|
||
(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 ??
Flags: needinfo?(gauravmittal1995) → needinfo?(kgrandon)
Comment 22•10 years ago
|
||
(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?
Flags: needinfo?(kgrandon)
Comment 23•10 years ago
|
||
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.
Attachment #8494672 -
Flags: review?(gauravmittal1995)
Assignee | ||
Updated•10 years ago
|
Attachment #8494672 -
Flags: review?(gauravmittal1995) → review+
Assignee | ||
Comment 24•10 years ago
|
||
@kevin Yes, i think this solves all the unit test problem. Thanks a lot for helping.
Comment 25•10 years ago
|
||
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.
Attachment #8489725 -
Attachment is obsolete: true
Attachment #8489725 -
Flags: review+
Comment 26•10 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•