1025586 - MarionetteJS Tests for Smart Collections

Status: RESOLVED FIXED

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

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 8440365 [details] [review]
Github pull request

I'm pretty happy with this initial test. There's a lot more I'd like to start working on (custom collections), pinning, etc. I think integration tests can provide us a lot more value than unit testing (though there is good uses for both). Please let me know what you guys think of this initial test. 

Ran, Amir - I'd also like to spin you guys up on the framework so you can help write some of these tests. Take a look at the collection_test.js file - it's not too difficult, but it can take a while to learn how the testing framework works and such.

Comment 3

[Amir Nissim (:amirn)]

Comment on attachment 8440365 [details] [review]
Github pull request

I'm not really comfortable reviewing this since I am not familiar with the testing framework.

As for the E.me changes, if I understand correctly we need a way to override the api URL for testing, but I don't think it should be part of gaia settings since it is a "device feature" and is not customizable.

Can we find some other way to do this?
We can refactor the `api.js` constructor to receive the URL if needed.

Comment 4

[Amir Nissim (:amirn)]

(In reply to Amir Nissim (Everything.me) from comment #3)
> Comment on attachment 8440365 [details] [review]
> Github pull request
> 
> since it **NOT** is a "device feature" and is not customizable.

Comment 5

[Kevin Grandon :kgrandon]

I am fairly sure this needs to be a setting - it's the only way we can really test it from an integration test. Wherever you want to read the setting from in eme code doesn't really matter to me, sticking it in device.js made sense to me because we're already doing a get all of settings, so putting it there is going to not really slow down startup speed. (A new settingsLock may do that).

My recommendation might be to just rename device.js to config.js or similar.

Comment 6

[Amir Nissim (:amirn)]

In that case I would prefer having a general `eme.device.env` setting to indicate we are running in test mode. Something like this maybe:
https://github.com/EverythingMe/gaia/commit/8e483ede0517e4be1fa0e7dcd9d58ac8bf2237af

Maybe there is already a setting that flags testing mode?

Comment 7

[Kevin Grandon :kgrandon]

(In reply to Amir Nissim (Everything.me) from comment #6)
> Maybe there is already a setting that flags testing mode?

But we actually want to test our production code paths :) This also need to comes from settings somewhere, as we have to set it before the code runs. I'm fine with however you guys want to structure it as long as we get a nice settings hook to set the URL, and we don't impact startup time too much. I still think renaming device.js to config.js is the best approach, but whatever you guys want to do I'm fine with.

Comment 8

[Amir Nissim (:amirn)]

Kevin, please consider using this patch:
https://github.com/EverythingMe/gaia/commit/e37453c3df2152fd637aa60d12725541fdeeced3

It creates `eme.config` with an apiUrl attribute.
I moved the `readSettings` logic up to main `eme` object and then pass it to `eme.device.init`.

So, we still have only one settings lock, eme.devic contains only device information and eme.config can be overridden by settings.

Let me know what you think.

Comment 9

[Kevin Grandon :kgrandon]

Amir - sure, I think that could totally work for me! Let's get that landed and I'll rebase. Thanks!

Comment 10

[Kevin Grandon :kgrandon]

Amir - I applied the patch, and it seems to be working well. Thanks!

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 8440365 [details] [review]
Github pull request

Dale - Maybe you will want to take a look at this because we might be able to use it or something similar for search app tests.

Comment 12

[Dale Harvey (:daleharvey)]

Comment on attachment 8440365 [details] [review]
Github pull request

These look great, I think James should have final review being more familiar with the marionette code, but in general its looking great. I do think it should likely go in shared, will definitely be reusing this

Comment 13

[Kevin Grandon :kgrandon]

Received a verbal R+ from James as he was sitting next to me.

Comment 14

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/4928dde7642461180554448a50b81f5ab1c6924d

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8440365 [details] [review]
Github pull request

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/247b93347550f8ca75c3654fab8e685a07a3792b