Closed Bug 1094151 Opened 9 years ago Closed 9 years ago

Write a test to install & launch a packaged app from the Marketplace

Categories

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::UI Tests
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

Details

Attachments

(2 files, 5 obsolete files)

marketplace5
46 bytes, text/x-github-pull-request
Details | Review
marketplace6
46 bytes, text/x-github-pull-request
jlorenzo
: review+
Bebe
: superreview+
Details | Review 
Bug 989562 handled the case to install and launch (and delete) a packaged app.

Originally, it tried to do this, using Marketplace.
Because running automated tests locally is more reliable, it was decided to do this locally.

But iiuc, there is still need for a gaia-ui test for installing (and deleting?) a packaged app from the Marketplace.

In this pull request, I basically have the code that does such a thing: https://github.com/mozilla-b2g/gaia/pull/25090

It wasn't working reliably, but with the hack from bug 1086680, comment 18, I think it could be made working reliably.

Let me know if this is something that is still wanted or not.
This is in the "Manual vs Automated Smoketests" fwiw.
QA Whiteboard: [fxosqa-auto-backlog+]
QA Whiteboard: [fxosqa-auto-backlog+] → [fxosqa-auto-s4]
I wanted to use the Dev Marketplace for this, but installing apps from there doesn't work atm. I filed bug 1100648 for that.
Zac, I think this test should use the "Dev" Marketplace app, right? Not the official Marketplace app. Then, in that case, this test needs to wait for bug 1100648 to get resolved.
Depends on: 1100648
Flags: needinfo?(zcampbell)
Hrm, no idea why this failed on tbpl try:
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=f96af362d1a8
ImportError: No module named marketplace.app
It is there in the pull request.
Martijn, you probably need to put __init__.py file into the marketplace folder to initialise it as a python package.
Flags: needinfo?(zcampbell)
Ok, yeah, I might have forgotten to git add that.
I'm going to wait on this, because I first need to have bug 1100648 fixed.
QA Whiteboard: [fxosqa-auto-s4] → fxosqa-auto-points=8, fxosqa-auto-from-s4, fxosqa-auto-backlog+
Attached file marketplace2 (obsolete) — 
Ok, I've got it working now and I think this is more or less ready for review.

I'll add some comments to the PR to explain some more.
Attachment #8524165 - Attachment is obsolete: true
Attachment #8529109 - Flags: review?(jlorenzo)
Krupa, we hould be using "Dev" Marketplace app for running automated tests, right?
Flags: needinfo?(krupa.mozbugs)
Attached file marketplace3 (obsolete) — 
On tryserver desktop, there were failures (Smart Collections permission dialog interfering), which I didn't see locally.
I added a print() statement to find out what's going on.
Attachment #8529109 - Attachment is obsolete: true
Attachment #8529109 - Flags: review?(jlorenzo)
Ok, I added the print() statement in the wrong place :(
The log only shows the output from the timeout failure. It doesn't show anything from before that.
I guess I have to invoke the print() statement from inside that wait statement or something to get it shown in the treeherder log, which is less than nice.
Attached file marketplace4 (obsolete) — 
Ok, this tries to fix the issue in comment 9 by adding this line:
self.apps.set_permission('Smart Collections', 'geolocation', 'deny')
..like it was done at:
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/tests/functional/homescreen/test_homescreen_delete_app.py#16

Apparently, this geolocation dialog pops up more often in certain gaia-ui tests, because I see it more often. But it's not clear to me, why these happen with these tests.
Attachment #8529160 - Attachment is obsolete: true
Attachment #8530994 - Flags: review?(jlorenzo)
(In reply to Martijn Wargers [:mwargers] (QA) from comment #12)
> Treeherder try worked:
> http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/382/

Jenkins run (11 times) also passed.
On IRC it was mentioned that there are https://github.com/mozilla/marketplace-tests-gaia 
Those are gaia-ui-tests that are split from gaia-ui tests for some reason. From what I understand, those work the same as gaia-ui tests.
Perhaps this test should belong in that repository?
Flags: needinfo?(andrei.hutusoru)
Comment on attachment 8530994 [details] [review]
marketplace4

It globally looks good to me. Some nits and questions left on the PR.
Attachment #8530994 - Flags: review?(jlorenzo) → review-
For packaged apps to install from dev or stage, you need to make sure that

a) certs are enabled. for 2.2, that is in Developer -> Use marketplace reviewer certs is checked.
b) the app being installed has installs_allowed_from set to *

I'd use dev since it is a good place to catch regressions earlier in the workflow. Thanks!
Flags: needinfo?(krupa.mozbugs)
Comment on attachment 8530994 [details] [review]
marketplace4

Ok, updated the patch with your review comments as good as I could.
Attachment #8530994 - Flags: review- → review?(jlorenzo)
Comment on attachment 8530994 [details] [review]
marketplace4

We're getting close. One last major issue to me, I think we should put the name and the manifest_url as a class attribute of MarketplaceDev (see more details in the PR).
Attachment #8530994 - Flags: review?(jlorenzo) → review-
Comment on attachment 8530994 [details] [review]
marketplace4

Ok, updated the pull request and tried to address your review comments as good as possible.
Attachment #8530994 - Flags: review- → review?(jlorenzo)
Comment on attachment 8530994 [details] [review]
marketplace4

A couple of small nits, once they are fixed, I'm good to go!
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #20)
> Comment on attachment 8530994 [details] [review]
> marketplace4
> 
> A couple of small nits, once they are fixed, I'm good to go!

I updated the small nits (and retested locally on b2g desktop).
Flags: needinfo?(andrei.hutusoru)
Comment on attachment 8530994 [details] [review]
marketplace4

LGTM!
Attachment #8530994 - Flags: review?(jlorenzo) → review+
Attachment #8530994 - Flags: review?(gmealer)
Andrei, on IRC it was mentioned that there are https://github.com/mozilla/marketplace-tests-gaia 
Those are gaia-ui-tests that are split from gaia-ui tests for some reason. From what I understand, those work the same as gaia-ui tests.
Perhaps this test should belong in that repository?
I don't know how or when these tests are run, fwiw.
Wouldn't it be better to just add those to the main gaia-ui repository? But just not run them as part of the gaia-ui-tests or something? Things get scattered otherwise, this way.
Flags: needinfo?(gmealer)
Flags: needinfo?(andrei.hutusoru)
(In reply to Martijn Wargers [:mwargers] (QA) from comment #23)
> Andrei, on IRC it was mentioned that there are
> https://github.com/mozilla/marketplace-tests-gaia 
> Those are gaia-ui-tests that are split from gaia-ui tests for some reason.
> From what I understand, those work the same as gaia-ui tests.
> Perhaps this test should belong in that repository?
> I don't know how or when these tests are run, fwiw.
> Wouldn't it be better to just add those to the main gaia-ui repository? But
> just not run them as part of the gaia-ui-tests or something? Things get
> scattered otherwise, this way.

Difference is that the marketplace-tests-gaia repo is owned by a different group, and implements their test plan. It should be concentrating very specifically on exercising the marketplace (and probably its payment system, but I haven't looked into them).

I'm a big believer in keeping repo ownership separated they have different concerns for maintenance, execution, etc., so I don't think we should mix them in.

As for our side, implementing a couple of tests that touch marketplace makes sense, since we do touch it as part of our smoketests to know we can install an app. But really, we're testing "installation from Marketplace," with an emphasis on installation. So that's a basic Gaia concern. We shouldn't go any deeper.
Flags: needinfo?(gmealer)
(In reply to krupa raj[:krupa] from comment #16)
> For packaged apps to install from dev or stage, you need to make sure that
> 
> a) certs are enabled. for 2.2, that is in Developer -> Use marketplace
> reviewer certs is checked.
> b) the app being installed has installs_allowed_from set to *
> 
> I'd use dev since it is a good place to catch regressions earlier in the
> workflow. Thanks!

I'm afraid I couldn't disagree with this more. We're not in the business of testing Marketplace changes with this suite, so we need to isolate "Gaia breakage" from "Marketplace breakage."

That means we should be using the most stable version of the Marketplace available so that we know test failures indicate either A) our side broke or B) the *stable* version changed protocol in a way that means we need to update. We absolutely should not be automating against a version under live development, especially for a regression test suite.

Is Stage typically kept on the same version as production and kept stable? If so, we should be testing to that. Otherwise, we need a production mirror somewhere (with only a very few test apps, of course).

NI'ing Krupa for info on the last bit.
Flags: needinfo?(krupa.mozbugs)
Comment on attachment 8530994 [details] [review]
marketplace4

LGTM, codewise, but please see my https://bugzilla.mozilla.org/show_bug.cgi?id=1094151#c25 regarding using the dev version of Marketplace.

We should resolve that conversation, but in the meantime it probably makes sense to get this test in place.
Attachment #8530994 - Flags: review?(gmealer) → review+
(In reply to Geo Mealer [:geo] from comment #25)
> (In reply to krupa raj[:krupa] from comment #16)
> > For packaged apps to install from dev or stage, you need to make sure that
> > 
> > a) certs are enabled. for 2.2, that is in Developer -> Use marketplace
> > reviewer certs is checked.
> > b) the app being installed has installs_allowed_from set to *
> > 
> > I'd use dev since it is a good place to catch regressions earlier in the
> > workflow. Thanks!
> 
> I'm afraid I couldn't disagree with this more. We're not in the business of
> testing Marketplace changes with this suite, so we need to isolate "Gaia
> breakage" from "Marketplace breakage."
> 
> That means we should be using the most stable version of the Marketplace
> available so that we know test failures indicate either A) our side broke or
> B) the *stable* version changed protocol in a way that means we need to
> update. We absolutely should not be automating against a version under live
> development, especially for a regression test suite.
> 
> Is Stage typically kept on the same version as production and kept stable?
> If so, we should be testing to that. Otherwise, we need a production mirror
> somewhere (with only a very few test apps, of course).
> 
> NI'ing Krupa for info on the last bit.

If the purpose is to test gaia breakage, then you should use Marketplace prod. Stage is more stable than dev but gets updated often 3 times a week. This also makes it easier since you don't have to worry about dev/stage certs.

I can have a separate test which runs on dev to have coverage for marketplace. Thanks!
Flags: needinfo?(krupa.mozbugs)
Sounds good, re: splitting concerns. That also gives us isolated data against each others' suites, which I would think would be helpful.

Martijn expressed a concern in-person that if we test against prod, we're going to raise the download count on the app we test against to the point where it might overwhelm stats/scoreboards/etc. That's probably a legitimate concern depending on how much other traffic we get.

Any comment on that?
Flags: needinfo?(krupa.mozbugs)
please use https://marketplace.firefox.com/app/test-webapi-permissions-9?src=search which is a test app we have in prod.

I have filed https://bugzilla.mozilla.org/show_bug.cgi?id=1112731 to make sure that we ignore the install count bump that app might get.
Flags: needinfo?(krupa.mozbugs)
(In reply to krupa raj[:krupa] from comment #29)
> please use
> https://marketplace.firefox.com/app/test-webapi-permissions-9?src=search
> which is a test app we have in prod.

I assume this is a packaged app, right?
Flags: needinfo?(andrei.hutusoru) → needinfo?(krupa.mozbugs)
(In reply to Martijn Wargers [:mwargers] (QA) from comment #30)
> (In reply to krupa raj[:krupa] from comment #29)
> > please use
> > https://marketplace.firefox.com/app/test-webapi-permissions-9?src=search
> > which is a test app we have in prod.
> 
> I assume this is a packaged app, right?

yes.
Flags: needinfo?(krupa.mozbugs)
Comment on attachment 8530994 [details] [review]
marketplace4

Thanks, that makes it clear.
The test now makes use of the official Marketplace app and searches for 'test-webapi-permissions :packaged' and installs the first search-item app that is there.

Unfortunately, that means this pull request needs to be rereviewed.
Attachment #8530994 - Flags: review?(jlorenzo)
Attachment #8530994 - Flags: review?(gmealer)
Attachment #8530994 - Flags: review+
Comment on attachment 8530994 [details] [review]
marketplace4

I took the time to read the entire diff again as it was hard to see what changed since last week. I don't see any issue. Looks good to me.
Attachment #8530994 - Flags: review?(jlorenzo) → review+
Comment on attachment 8530994 [details] [review]
marketplace4

LGTM, thanks for the changes!
Attachment #8530994 - Flags: review?(gmealer) → review+
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #33)
> I took the time to read the entire diff again as it was hard to see what
> changed since last week. I don't see any issue. Looks good to me.

Sorry about that, I should have made clear what I changed.
Mostly, it was removing some cruft that I needed to launch the Dev app.
Kicked of a Jenkins testrun with 20 repeats to make sure no intermittent failures are lurking: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/470/
Attached file marketplace5 (obsolete) — 
I had to make a new branch to get the repo update to be able to kick of a Jenkins test run, because Marionette had been updated, because I don't know how to get the remote marketplace4 branch updated to master.
Attachment #8530994 - Attachment is obsolete: true
Attachment #8539367 - Flags: review+
Attachment #8539367 - Flags: feedback+
Nothing changed with marketplace5 pull request, it's just latest master and the patch applied from marketplace4 (which applied cleanly).
(In reply to Martijn Wargers [:mwargers] (QA) from comment #37)
> Created attachment 8539367 [details] [review]
> marketplace5
> 
> I had to make a new branch to get the repo update to be able to kick of a
> Jenkins test run, because Marionette had been updated, because I don't know
> how to get the remote marketplace4 branch updated to master.

Because Jenkins adhoc runs didn't work with the previous marketplace4 pull request, I had to update. I've now kicked off a new adhoc Jenkins run:
http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/482/
Ok, all Jenkins runs passed, this is ready to be merged.
I merged it, but it had to be backed out (thanks, Ryan!), because it caused failures. These could also be seen on the try run: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=674789001d33
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/gaia-try/sha512/769369b947df199191db4e2db75d461d3f007af141b71dcc04c6009cc7712b99d74ee0b8085c937cd2d8967dccae92726c47a048e18d210ee0918390eec92d7a
Traceback (most recent call last):
File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/marionette_test.py", line 268, in run
testMethod()
File "/builds/slave/test/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/browser/test_browser_bookmark.py", line 41, in test_browser_bookmark
homescreen.wait_for_app_icon_present(self.bookmark_title)
File "/builds/slave/test/gaia/tests/python/gaia-ui-tests/gaiatest/apps/homescreen/app.py", line 47, in wait_for_app_icon_present
Wait(self.marionette, timeout=timeout).until(lambda m: self.installed_app(app_name))
File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/wait.py", line 143, in until
cause=last_exc)
TimeoutException: TimeoutException: Timed out after 11.3 seconds

I'm totally at a loss why my pull request a failure in there.
This test is not even supposed to be run on b2g desktop!
Argh, never mind, it has to do something with the change I made in wait_for_app_icon_present().
Comment on attachment 8539367 [details] [review]
marketplace5

Ok, the only thing I changed in here is, reverting the api chance in wait_for_app_icon_present.
So those other failures shouldn't be happening anymore.
I changed the timeout to 30s, but perhaps it's not needed at all, because the app that now is used for installing is much smaller than the previous app, so downloading it should take at most a couple of seconds.
Attachment #8539367 - Flags: review?(gmealer)
Attachment #8539367 - Flags: review+
Attachment #8539367 - Flags: feedback+
That teaches me to thing that even though my test wasn't actually run on treeherder, it can still cause failures!
(In reply to Martijn Wargers [:mwargers] (QA) from comment #43)
> Comment on attachment 8539367 [details] [review]
> marketplace5
> 
> Ok, the only thing I changed in here is, reverting the api chance in
> wait_for_app_icon_present.
> So those other failures shouldn't be happening anymore.
> I changed the timeout to 30s, but perhaps it's not needed at all, because
> the app that now is used for installing is much smaller than the previous
> app, so downloading it should take at most a couple of seconds.

Before I review this, I don't understand some of the issue.

You say it's not supposed to run on B2G Desktop but it did? Isn't that a problem?

I also compared this to the pull request I did yesterday, and I don't see the change. They both request a 60 sec timeout at the test level, and just pass it through at the API level. Maybe it stealth updated when you pushed the new one? 

But either way, what's timing out at 11.3 seconds?
(In reply to Geo Mealer [:geo] from comment #45)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #43)
> > Comment on attachment 8539367 [details] [review]
> > marketplace5
> > 
> > Ok, the only thing I changed in here is, reverting the api chance in
> > wait_for_app_icon_present.
> > So those other failures shouldn't be happening anymore.
> > I changed the timeout to 30s, but perhaps it's not needed at all, because
> > the app that now is used for installing is much smaller than the previous
> > app, so downloading it should take at most a couple of seconds.
> 
> Before I review this, I don't understand some of the issue.
> 
> You say it's not supposed to run on B2G Desktop but it did? Isn't that a
> problem?

It didn't run on b2g desktop, but I changed something that caused another test to fail.

I apparently forgot to update the pull request, because it still looked the same.
Attached file marketplace5 — 
Ok, apparently, I pointed to the wrong pull request somehow. This has the change in wait_for_app_icon_present.
Attachment #8539367 - Attachment is obsolete: true
Attachment #8539367 - Flags: review?(gmealer)
Attachment #8539537 - Flags: review?(gmealer)
Comment on attachment 8539537 [details] [review]
marketplace5

Hrm, never mind. Still failing. I'll have to investigate when I get home.
Attachment #8539537 - Flags: review?(gmealer)
Ok, I've now updated the pull request, I added this check:--- a/tests/python/gaia-ui-tests/gaiatest/apps/homescreen/app.py
+++ b/tests/python/gaia-ui-tests/gaiatest/apps/homescreen/app.py
@@ -151,7 +151,8 @@ class Homescreen(Base):
 
     def installed_app(self, app_name):
         for root_el in self.marionette.find_elements(*self._homescreen_all_icons_locator):
-            if root_el.text == app_name and root_el.get_attribute('data-app-state') == 'ready':
+            if root_el.text == app_name and (root_el.get_attribute('data-app-state') == 'ready' or
+                'bookmark' in root_el.get_attribute('class')):
                 return self.InstalledApp(self.marionette, root_el)

This should make treeherder try results stay green: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=9d1dedc24eb9
Attached file marketplace6 — 
The test_browser_bookmark was fixed in the patch, but I saw some other failures, for which I'm pretty sure, aren't caused by this pull request.
Just to be sure, I updated the tree and created this new pull request.
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=1edfeaf9e9c2
Comment on attachment 8540464 [details] [review]
marketplace6

Treeherder try is all green now.
Attachment #8540464 - Flags: superreview?(florin.strugariu)
Attachment #8540464 - Flags: review?(jlorenzo)
Comment on attachment 8540464 [details] [review]
marketplace6

Looks good to me.
Attachment #8540464 - Flags: review?(jlorenzo) → review+
Comment on attachment 8540464 [details] [review]
marketplace6

Started http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/511/
Attachment #8540464 - Flags: superreview?(florin.strugariu) → superreview+
https://github.com/mozilla-b2g/gaia/commit/ad929ad5f3d18b6b6be739e6b4ef3d70383e4764
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
QA Whiteboard: fxosqa-auto-points=8, fxosqa-auto-from-s4, fxosqa-auto-backlog+ → fxosqa-auto-points=8, fxosqa-auto-from-s4, fxosqa-auto-backlog+, [fxosqa-auto-s7]
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.