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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Everything.me, defect)

Description

[Ran Ben Aharon [:ranbena] EverythingMe]

Currently they wait for eme.load

Comment 1

[Ran Ben Aharon [:ranbena] EverythingMe]

Attachment: PR

This is a big step for Collection responsiveness

Comment 2

[Amir Nissim (:amirn)]

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)

Comment 3

[Ran Ben Aharon [:ranbena] EverythingMe]

Neither Contextmenu, ViewApp, ViewBgImage globals are loaded into view_collection.js.

Amir, I have no idea how to handle this. Can you help?

Comment 4

[Amir Nissim (:amirn)]

I guess you should include them with require(). did you try that?

Comment 5

[Ran Ben Aharon [:ranbena] EverythingMe]

Kevin, need your assistance here :/

Comment 6

[Kevin Grandon :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!

Comment 7

[Ran Ben Aharon [:ranbena] EverythingMe]

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

Comment 8

[Ran Ben Aharon [:ranbena] EverythingMe]

Amir, r+ me!

Comment 9

[Kevin Grandon :kgrandon]

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+.

Comment 10

[Amir Nissim (: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.

Comment 11

[Kevin Grandon :kgrandon]

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

Comment 12

[Kevin Grandon :kgrandon]

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 13

[Ran Ben Aharon [:ranbena] EverythingMe]

Comment on attachment 8440701 [details] [review]
PR

Kevin I went for your 2nd suggestion. Seems safest to me.

Comment 14

[Amir Nissim (:amirn)]

landed in master: https://github.com/mozilla-b2g/gaia/commit/3d54dcf597b1b427db72bbf74b0e6c714321e6f8

Comment 15

[Kevin Grandon :kgrandon]

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.

Comment 16

[Kevin Grandon :kgrandon]

2.0: https://github.com/mozilla-b2g/gaia/commit/cdfbd0a9d361fa351e1acb567f0d76a0bb189c76