1191746 - Port marionette tests from verticalhome to new homescreen, where appropriate

Status: RESOLVED FIXED

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

Description

[Chris Lord [:cwiiis]]

homescreen needs to be well-tested, like the existing verticalhome. We should port over the marionette tests, where appropriate.

Where it isn't appropriate:
- Search-bar tests
- App-grouping tests
- Divider tests
- Collections tests

Comment 1

[Chris Lord [:cwiiis]]

Of the 48 current marionette tests in verticalhome, I think 25 of them are applicable to the new homescreen, 2 of those 25 require features in the new homescreen (blacklist and RTL), and at least a further 3 won't pass due to incorrect behaviour in the new homescreen (after porting, of course - none of them will pass as is due to the huge differences between the two apps).

For reference, these tests are:
app_appcache_pending_test.js
app_blacklist_test.js
app_failed_icon_fetch_test.js
app_hosted_failed_icon_fetch_test.js
app_hosted_install_test.js
app_hosted_use_cached_icon_test.js
app_packaged_fail_test.js
app_packaged_install_test.js
app_packaged_pending_test.js
app_packaged_resume_test.js
app_packaged_resume_update_test.js
app_packaged_role_hidden_test.js
app_packaged_update_test.js
app_pending_update_test.js
app_retry_icon_fetch_test.js
app_short_name_test.js
app_uninstall_pending_test.js
app_uninstall_test.js
app_unrecoverable_error_test.js
bookmark_edit_test.js
bookmark_invalid_favicon_test.js
bookmark_test.js
bookmark_uninstall_test.js
grid_layout_test.js
localization_test.js

The other tests are all testing collections, app-grouping or search.

I'm going to start porting them, we may want to do this as multiple commits and leave this bug open until all 25 are tackled (or are omitted with a very good reason as to why).

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1191746-new-homescreen-marionette-tests > mozilla-b2g:master

Comment 3

[Chris Lord [:cwiiis]]

It was a fair amount of work getting this one working, so assuming they're all going to be non-trivial, I'm electing to do this in chunks, but under this one bug to reduce churn. Adding leave-open keyword.

Comment 4

[Chris Lord [:cwiiis]]

Comment on attachment 8646986 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-tests > mozilla-b2g:master

The test works basically the same as the verticalhome one, a little simplified.

We can't currently get icons by manifest URL due to some marionette weirdness, so using a getIconByName method. Likelihood is we'll have to modify gaia-app-icon later to help with marionette testing, but I'll deal with it when the time comes.

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 8646986 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-tests > mozilla-b2g:master

Flagging Kevin for a quick look-over too.

Comment 6

[Guillaume Marty [:gmarty]]

Comment on attachment 8646986 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-tests > mozilla-b2g:master

Looks good to me!

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8646986 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-tests > mozilla-b2g:master

Looks good to me also. Thanks.

Comment 8

[Chris Lord [:cwiiis]]

Comment on attachment 8646986 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-tests > mozilla-b2g:master

Adding my own r+ so autolander can land this... Hopefully this works.

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1191746-new-homescreen-marionette-layout > mozilla-b2g:master

Comment 10

[Chris Lord [:cwiiis]]

Comment on attachment 8647524 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-layout > mozilla-b2g:master

Just one test, grid_layout (renamed to just layout). Next one will be a batch of app tests.

Comment 11

[Chris Lord [:cwiiis]]

Merged localization_test: https://github.com/mozilla-b2g/gaia/commit/8e0d33beca9beff0878b175a4949b54c051bbd27

Comment 12

[Guillaume Marty [:gmarty]]

Comment on attachment 8647524 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-layout > mozilla-b2g:master

All good!

Comment 13

[Chris Lord [:cwiiis]]

layout test merged: https://github.com/mozilla-b2g/gaia/commit/b1f5804a38fecbffad857ef317bff5325c38e5b7

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1191746-new-homescreen-marionette-hosted-app-tests > mozilla-b2g:master

Comment 15

[Chris Lord [:cwiiis]]

Comment on attachment 8648087 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-hosted-app-tests > mozilla-b2g:master

This ports the hosted app tests to new homescreen. This fixes a few related bugs in gaia-app-icon and adds the code necessary in homescreen to re-download failed app icons on connection change.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Is it possible to use this opportunity to not remove mozL10n.get use from homescreen?

The only place where you do this now is in:

./apps/homescreen//test/marionette/lib/homescreen.js:  l10n: function(key) {
./apps/homescreen//test/marionette/localization_test.js:    var expected = home.l10n('cancel-action');

and what you should be testing instead is data-l10n-id of that element, not it's text.


Also, we're moving apps to use shared/js/l20n.js instead of shared/js/l10n.js and the main difference is the removal of this synchronous .get.
I don't see any use of l10n in the new homescreen, and I'm not sure if it's because it doesn't need l10n or it's not there yet, but if you do plan to use l10n in a new app, I'd recommend using l20n.js.

Comment 17

[Guillaume Marty [:gmarty]]

Comment on attachment 8648087 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-hosted-app-tests > mozilla-b2g:master

LGTM!

Comment 18

[Chris Lord [:cwiiis]]

Merged: https://github.com/mozilla-b2g/gaia/commit/415825fb53bdac5513461f139ee378cee34c612e

Zibi, at the moment this is basically a straight port of the tests that exist. I'd prefer to do any non-trivial improvements/changes in follow-up bugs. Feel free to make it block bug 1191745 and I'll deal with it once the porting is done.

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok! Let's just make sure that when you start working on making the new homescreen localizable, we use l20n.js for that :)

Comment 20

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1191746-new-homescreen-marionette-packaged-app-tests > mozilla-b2g:master

Comment 21

[Chris Lord [:cwiiis]]

Comment on attachment 8649195 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-packaged-app-tests > mozilla-b2g:master

This ports all app_packaged_* tests.

The following needed to be fixed alongside:
- Accessibility support in gaia-app-icon/homescreen
- Packaged app role changing

All tests remain functionally equivalent (though app_packaged_update now tests for pixel differences in the icon rather than a changed URL, as the API we use to get the app icon doesn't return a URL)

Comment 22

[Chris Lord [:cwiiis]]

For reference, what's left:

app_blacklist_test.js
app_retry_icon_fetch_test.js
app_short_name_test.js
app_uninstall_pending_test.js
app_uninstall_test.js
app_unrecoverable_error_test.js
bookmark_edit_test.js
bookmark_invalid_favicon_test.js
bookmark_test.js
bookmark_uninstall_test.js

Ends up app_failed_icon_fetch_test.js was just a duplicate of app_hosted_failed_icon_fetch_test.js, so there are only 24 tests in total (unless I discover further redundant tests).

Comment 23

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1191746-new-homescreen-marionette-remaining-app-tests > mozilla-b2g:master

Comment 24

[Chris Lord [:cwiiis]]

Comment on attachment 8649294 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-remaining-app-tests > mozilla-b2g:master

The rest of the app tests, just the bookmark tests to go.

Comment 25

[Chris Lord [:cwiiis]]

Merged packaged app tests: https://github.com/mozilla-b2g/gaia/commit/ced11b5401352fa970cdbc986d45c55ed8cf5fac

Comment 26

[Chris Lord [:cwiiis]]

Remaining app tests merged: https://github.com/mozilla-b2g/gaia/commit/d277f2be513ea4d9e28a131dbe6a516e3ebf0d9b

Just bookmarks to go...

Comment 27

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1191746-new-homescreen-marionette-bookmark-tests > mozilla-b2g:master

Comment 28

[Chris Lord [:cwiiis]]

Comment on attachment 8650558 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-bookmark-tests > mozilla-b2g:master

Port of the bookmark tests.

This also includes a rejigging of the initial icon-loading code - ended up it wasn't necessary, but it should load bookmarks a bit quicker now.

An odd thing, I needed to set the pref for the homescreen to get datastores to work, but doing so also breaks app uninstalling, so I've separated the client_options for the bookmarks test. Everything works fine on an actual device, not sure what's going on there - likelihood is when the homescreen is the default we can remove that.

Comment 29

[Guillaume Marty [:gmarty]]

Comment on attachment 8650558 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-bookmark-tests > mozilla-b2g:master

Looks good to me!

Comment 30

[Chris Lord [:cwiiis]]

Merged: https://github.com/mozilla-b2g/gaia/commit/5d2a5aacd936fd732ca8a9913a798e0392f94a53

And we're done!

I've noticed a few bugs creeping in as I port these marionette tests, I've filed several polish bugs and made them block bug 1191745, will get to addressing these. Guillaume and Alberto have already started on pin-the-web work related to the homescreen.

Comment 31

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1191746-test-breakage-followup > mozilla-b2g:master

Comment 32

[Guillaume Marty [:gmarty]]

Comment on attachment 8651092 [details] [review]
[gaia] Cwiiis:bug1191746-test-breakage-followup > mozilla-b2g:master

LGTM!

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merged: https://github.com/mozilla-b2g/gaia/commit/09e233d1476e8745f192f13259d7b16662bb2e7e

Thanks for the fix!