Closed Bug 1191746 Opened 5 years ago Closed 5 years ago

Port marionette tests from verticalhome to new homescreen, where appropriate

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
FxOS-S5 (21Aug)

People

(Reporter: cwiiis, Assigned: cwiiis)

References

Details

(Whiteboard: [systemsfe])

Attachments

(7 files)

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
Whiteboard: [systemsfe]
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).
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.
Keywords: leave-open
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.
Attachment #8646986 - Flags: review?(gmarty)
Comment on attachment 8646986 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-tests > mozilla-b2g:master

Flagging Kevin for a quick look-over too.
Attachment #8646986 - Flags: feedback?(kevingrandon)
Comment on attachment 8646986 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-tests > mozilla-b2g:master

Looks good to me!
Attachment #8646986 - Flags: review?(gmarty) → review+
Comment on attachment 8646986 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-tests > mozilla-b2g:master

Looks good to me also. Thanks.
Attachment #8646986 - Flags: feedback?(kevingrandon) → feedback+
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.
Attachment #8646986 - Flags: review+
Keywords: checkin-needed
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
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.
Attachment #8647524 - Flags: review?(gmarty)
Comment on attachment 8647524 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-layout > mozilla-b2g:master

All good!
Attachment #8647524 - Flags: review?(gmarty) → review+
Attachment #8646986 - Flags: review+
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.
Attachment #8648087 - Flags: review?(gmarty)
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.
Flags: needinfo?(chrislord.net)
Comment on attachment 8648087 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-hosted-app-tests > mozilla-b2g:master

LGTM!
Attachment #8648087 - Flags: review?(gmarty) → review+
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.
Flags: needinfo?(chrislord.net)
Ok! Let's just make sure that when you start working on making the new homescreen localizable, we use l20n.js for that :)
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)
Attachment #8649195 - Flags: review?(gmarty)
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 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.
Attachment #8649294 - Flags: review?(gmarty)
Blocks: 1195340
Attachment #8649195 - Flags: review?(gmarty) → review+
Attachment #8649294 - Flags: review?(gmarty) → review+
Remaining app tests merged: https://github.com/mozilla-b2g/gaia/commit/d277f2be513ea4d9e28a131dbe6a516e3ebf0d9b

Just bookmarks to go...
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.
Attachment #8650558 - Flags: review?(gmarty)
Comment on attachment 8650558 [details] [review]
[gaia] Cwiiis:bug1191746-new-homescreen-marionette-bookmark-tests > mozilla-b2g:master

Looks good to me!
Attachment #8650558 - Flags: review?(gmarty) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S5 (21Aug)
Attachment #8651092 - Flags: review?(gmarty)
Comment on attachment 8651092 [details] [review]
[gaia] Cwiiis:bug1191746-test-breakage-followup > mozilla-b2g:master

LGTM!
Attachment #8651092 - Flags: review?(gmarty) → review+
Depends on: 1197489
You need to log in before you can comment on or make changes to this bug.