Closed
Bug 1062787
Opened 10 years ago
Closed 9 years ago
Use the new shared helper for loading JSON files in the smart collections code
Categories
(Firefox OS Graveyard :: Gaia::Homescreen, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: gsvelto, Assigned: gioyik, Mentored)
References
Details
(Whiteboard: [lang=js])
Attachments
(2 files)
+++ This bug was initially created as a clone of Bug #990047 +++
This functionctionality can be replaced using the new helper:
https://github.com/mozilla-b2g/gaia/blob/b213f6de0ef9e5d9b6533c040eeb7cf26d6f5717/apps/collection/js/setup.js#L35
Assignee | ||
Comment 1•10 years ago
|
||
Gabriele, could this bug taken? What should be done with this bug?
Flags: needinfo?(gsvelto)
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Giovanny Andres Gongora Granada from comment #1)
> Gabriele, could this bug taken? What should be done with this bug?
This bug is up for the taking, if you want to work on it just ask and I'll assign it to you. The scope of this bug is to replace the code I pointed to in comment 0 using the shared helper provided by bug 990047.
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 3•10 years ago
|
||
Hi Gabriele, I want to work on this bug. Could you assign me it? so I could have it listed on Bugzilla ToDO's client?
Thanks
Updated•10 years ago
|
Assignee: nobody → gioyik
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8567734 -
Flags: review?(gsvelto)
Assignee | ||
Comment 6•10 years ago
|
||
Is the PR ok? Is necessary a callback? Let me know if are necessary changes or is not the expected result.
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8567734 [details] [review]
Github PR
This is missing quite a few bits. First of all when the promise returns successfully you should parse the returned value and invoke the populate() function as the original code does:
https://github.com/mozilla-b2g/gaia/pull/28354/files#diff-ad674cfecb7f38b343ddae3532b08f80L46
Secondly you'll have to handle errors correctly like the existing code does:
https://github.com/mozilla-b2g/gaia/pull/28354/files#diff-ad674cfecb7f38b343ddae3532b08f80L53
Ask for feedback again once you've fixed these issues.
Attachment #8567734 -
Flags: review?(gsvelto) → feedback-
Assignee | ||
Updated•10 years ago
|
Attachment #8567734 -
Flags: review-
Attachment #8567734 -
Flags: feedback?(gsvelto)
Assignee | ||
Comment 8•10 years ago
|
||
Gabriele could you check the PR again? I did some changes.
Thanks
Reporter | ||
Comment 9•10 years ago
|
||
This is not going to work, not even the function name is right. Please take your time to understand how LazyLoader.getJSON() works before updating the PR, you can find the sources here:
https://github.com/mozilla-b2g/gaia/blob/f2e9115aa3ba4b6e8bafc3c0622d6c39f08018b9/shared/js/lazy_loader.js#L75
It returns a promise and you'll need to act upon that. You can find plenty of examples of how to use that function in other gaia code. The LazyLoader unit-tests for example, here:
https://github.com/mozilla-b2g/gaia/blob/77c08e32534179c3c1fd18ed4d9eafcbee0aed09/apps/sharedtest/test/unit/lazy_loader_test.js#L47
You should also test your changes before asking for feedback again; make sure all the tests for your application you're working on are passing. See here on how to run the unit tests:
https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests#Running_unit_tests
Reporter | ||
Updated•10 years ago
|
Attachment #8567734 -
Flags: review-
Attachment #8567734 -
Flags: feedback?(gsvelto)
Assignee | ||
Comment 10•10 years ago
|
||
Hi Gabriele,
After a time testing, seems that the problem was that I didn't export the global LazyLoader, so the test failed every change. That's the reason for what I changed from getJson to load at first. Now I changed the code and lint test passed, only Gaia unit test fails "orange state", but seems that fail due to the LazyLoad change.
Those test fails:
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > Window is closed - LazyLoader is not defined
ReferenceError: LazyLoader is not defined
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > There are no collections - timeout of 10000ms exceeded
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > There one collection - timeout of 10000ms exceeded
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > There are two collections - timeout of 10000ms exceeded
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > File not found - timeout of 10000ms exceeded
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > Unknown response - timeout of 10000ms exceeded
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > JSON not well-formed - timeout of 10000ms exceeded
TEST-UNEXPECTED-FAIL | keyboard/test/unit/imes/jspinyin/jspinyin_test.js | jspinyin deactivate (save indexedDB failed) - TypeError: procFunction is undefined (app://keyboard.gaiamobile.org/js/imes/jspinyin/worker.js:15)
Could you help me with this? Additionally could you check the PR and tell me if is ok?
Flags: needinfo?(gsvelto)
Reporter | ||
Comment 11•10 years ago
|
||
(In reply to Giovanny Gongora [:gioyik] from comment #10)
> After a time testing, seems that the problem was that I didn't export the
> global LazyLoader, so the test failed every change. That's the reason for
> what I changed from getJson to load at first. Now I changed the code and
> lint test passed, only Gaia unit test fails "orange state", but seems that
> fail due to the LazyLoad change.
Yes, you've made good progress and I've left some further comments on the PR. Regarding the tests:
> TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js >
> Window is closed - LazyLoader is not defined
> ReferenceError: LazyLoader is not defined
This is the important error, as you can see the LazyLoader object is not defined. This is because the setup_test.js file is not importing the LazyLoader sources which are now required to run the test. User the `require()` function to load the LazyLoader sources in that file and the error will go away.
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 12•10 years ago
|
||
I made some changes and added require in the file but now test failed by a timeout:
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > Window is closed - timeout of 10000ms exceeded
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > There are no collections - timeout of 10000ms exceeded
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > There one collection - timeout of 10000ms exceeded
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > There are two collections - timeout of 10000ms exceeded
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > File not found - timeout of 10000ms exceeded
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > Unknown response - timeout of 10000ms exceeded
TEST-UNEXPECTED-FAIL | collection/test/unit/setup_test.js | setup.js > JSON not well-formed - timeout of 10000ms exceeded
Could you check the code in the repo and address me on why could be happening with the timeout?
Flags: needinfo?(gsvelto)
Reporter | ||
Comment 13•10 years ago
|
||
(In reply to Giovanny Gongora [:gioyik] from comment #12)
> Could you check the code in the repo and address me on why could be
> happening with the timeout?
There are still issues in your PR. As for the timeouts you have to check what happens in the test. Since the function calls your using are asynchronous in real life the test needs to be setup asynchronously and notified when everything is ready. Check sinon's documentation on how to deal with asynchronous calls in tests:
http://sinonjs.org/docs/#spies-api
I also suggest debugging the tests, or at least adding print statements, to understand how they work and how you should change them.
Flags: needinfo?(gsvelto)
Comment 14•9 years ago
|
||
Mass update: Resolve wontfix all issues with legacy homescreens.
As of 2.6 we have a new homescreen and having these issues open is confusing. All issues will block bug 1231115 so we can use that to re-visit any of these if needed.
You need to log in
before you can comment on or make changes to this bug.
Description
•