[Collection app] Load Pinned apps and Bg Image on open

RESOLVED FIXED

Status

Firefox OS
Gaia::Everything.me
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ranbena, Assigned: ranbena)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(1 attachment)

Currently they wait for eme.load
(Assignee)

Updated

4 years ago
Assignee: nobody → ran
Blocks: 1016203
Status: NEW → ASSIGNED
Depends on: 1025844
Created attachment 8440701 [details] [review]
PR

This is a big step for Collection responsiveness
Attachment #8440701 - Flags: review?(amirn)

Comment 2

4 years ago
Comment on attachment 8440701 [details] [review]
PR

Comments on Github and the unit test is failing:
1) [collection-test/unit/view_collection_test.js] view >  renders a collection:
     ReferenceError: Contextmenu is not defined
      at HandleView (http://collection.gaiamobile.org:8080/js/view_collection.js:38:5)
      at onActivity (http://collection.gaiamobile.org:8080/js/view_collection.js:45:7)
Neither Contextmenu, ViewApp, ViewBgImage globals are loaded into view_collection.js.

Amir, I have no idea how to handle this. Can you help?
Flags: needinfo?(amirn)

Comment 4

4 years ago
I guess you should include them with require(). did you try that?
Flags: needinfo?(amirn)
Kevin, need your assistance here :/
Flags: needinfo?(kgrandon)
Oh, this is a funny one. Apparently before we were basically getting stuck processing eme.init() I think so we are now getting into a different code path in the test. If we fix the test we will have more code coverage which is probably good.

Basically you'll want to add these requirements into the test here: https://github.com/EverythingMe/gaia/blob/1025889-first-load/apps/collection/test/unit/view_collection_test.js#L6

We'll want all of the required files here: https://github.com/EverythingMe/gaia/blob/1025889-first-load/apps/collection/js/view_collection.js#L38
So Contextmenu, ViewApps, and ViewBgImage should be pulled into the test. Let me know if you have any problems with that and I'll take a look. Thanks!
Flags: needinfo?(kgrandon)
Yep, adding the requires works.
https://github.com/EverythingMe/gaia/blob/1025889-first-load/apps/collection/test/unit/view_collection_test.js#L7

I had to add a if (collection) as well, because when BaseCollection.create is stubbed, it returns undefined. Makes sense?
https://github.com/EverythingMe/gaia/blob/1025889-first-load/apps/collection/js/view_collection.js#L38
Flags: needinfo?(kgrandon)
Amir, r+ me!
Flags: needinfo?(amirn)
It's probably better to fill out the stub to have the necessary interfaces, but I'm fine with this for now if it gets you unblocked. F+.
Flags: needinfo?(kgrandon)

Updated

4 years ago
Attachment #8440701 - Flags: review?(amirn) → review+
Flags: needinfo?(amirn)
Comment on attachment 8440701 [details] [review]
PR

I'm sorry but I'm drawing back my r+ because I noticed the new change in view_collection.js - adding the 'if' for the tests to pass does not makes sense for me. I don't think we should change working code to get the tests passing but rather change the tests instead.
Attachment #8440701 - Flags: review+ → review-
There are a few options to solve this... 

1 - Return a stub collection object with the sinon.stub, this.sinon.stub(BaseCollection, 'create').returns(myCollectionStub);

2 - Stop requiring Contextmenu, etc, and use mocks for those classes out instead. There are a few different ways to do this, but the general way that we do this is by importing them and declaring them like so: https://github.com/EverythingMe/gaia/blob/1025889-first-load/apps/system/test/unit/rocketbar_test.js?pr=%2Fmozilla-b2g%2Fgaia%2Fpull%2F20573#L12

You then need to attach the test helpers: https://github.com/EverythingMe/gaia/blob/1025889-first-load/apps/system/test/unit/rocketbar_test.js?pr=%2Fmozilla-b2g%2Fgaia%2Fpull%2F20573#L22
Also, as we start having better integration test coverage with things like bug 1026778, I will probably be ok with going more that route and having less unit tests. I think we'll end up being able to cover much larger areas of the code with the integration tests, and have less fragile tests.
Comment on attachment 8440701 [details] [review]
PR

Kevin I went for your 2nd suggestion. Seems safest to me.
Attachment #8440701 - Flags: review- → review?(amirn)

Updated

4 years ago
Attachment #8440701 - Flags: review?(amirn) → review+
landed in master: https://github.com/mozilla-b2g/gaia/commit/3d54dcf597b1b427db72bbf74b0e6c714321e6f8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8440701 [details] [review]
PR

This is needed for the vertical homescreen. We've done our best effort at testing this and believe it is safe for uplift.
Attachment #8440701 - Flags: approval-gaia-v2.0?(bbajaj)

Updated

4 years ago
Attachment #8440701 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
2.0: https://github.com/mozilla-b2g/gaia/commit/cdfbd0a9d361fa351e1acb567f0d76a0bb189c76
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
You need to log in before you can comment on or make changes to this bug.