Closed Bug 1062787 Opened 5 years ago Closed 4 years ago

Use the new shared helper for loading JSON files in the smart collections code

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

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
Gabriele, could this bug taken? What should be done with this bug?
Flags: needinfo?(gsvelto)
(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)
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
Assignee: nobody → gioyik
Attached file Github PR
Attachment #8567734 - Flags: review?(gsvelto)
Is the PR ok? Is necessary a callback? Let me know if are necessary changes or is not the expected result.
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-
Attachment #8567734 - Flags: review-
Attachment #8567734 - Flags: feedback?(gsvelto)
Gabriele could you check the PR again? I did some changes.

Thanks
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
Attachment #8567734 - Flags: review-
Attachment #8567734 - Flags: feedback?(gsvelto)
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)
(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)
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)
(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)
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.
Blocks: 1231115
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.