Closed Bug 1025889 Opened 6 years ago Closed 6 years ago

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


(Firefox OS Graveyard ::, defect)

Not set


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

Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed


(Reporter: ranbena, Assigned: ranbena)




(1 file)

Currently they wait for eme.load
Assignee: nobody → ran
Depends on: 1025844
Attached file PR
This is a big step for Collection responsiveness
Attachment #8440701 - Flags: review?(amirn)
Comment on attachment 8440701 [details] [review]

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 (
      at onActivity (
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)
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:

We'll want all of the required files here:
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.

I had to add a if (collection) as well, because when BaseCollection.create is stubbed, it returns undefined. Makes sense?
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)
Attachment #8440701 - Flags: review?(amirn) → review+
Flags: needinfo?(amirn)
Comment on attachment 8440701 [details] [review]

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:

You then need to attach the test helpers:
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]

Kevin I went for your 2nd suggestion. Seems safest to me.
Attachment #8440701 - Flags: review- → review?(amirn)
Attachment #8440701 - Flags: review?(amirn) → review+
landed in master:
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 8440701 [details] [review]

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)
Attachment #8440701 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
You need to log in before you can comment on or make changes to this bug.