948826 - Create a UI automated test case to test permission prompts in a privileged app for contacts & device storage

Status: RESOLVED FIXED

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

Description

[Jason Smith [:jsmith]]

Per discussion in bug 853356:

We need a UI automated test case in either Marionette JS or Gaia UI Test to test permission prompts in a privileged app for contacts & device storage. See https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c214 for what needs to be done here.

Note - this could be implemented in either framework.

Comment 1

[Zac C (:zac)]

Shih-Chiang, is this the same prompt you get when you go into UI Tests app, tap API menu and then go into "getUserMedia" option?

We can write this test easily if there is a way we can do it in Gaia now but if we have to write an app in which to do it it is probably outside the ability of my team right now.

Could we easily modify UI Tests app to enable us to reach this prompt?

Comment 2

[Jason Smith [:jsmith]]

(In reply to Zac C (:zac) from comment #1)
> Shih-Chiang, is this the same prompt you get when you go into UI Tests app,
> tap API menu and then go into "getUserMedia" option?

Nope. This wouldn't be triggered in that app, as the UI Tests app is certified & this is a different permission prompt (gUM perm prompt is different than the contacts/device storage prompt).

> 
> We can write this test easily if there is a way we can do it in Gaia now but
> if we have to write an app in which to do it it is probably outside the
> ability of my team right now.

Why would this be outside of the ability of your team? Al already said he was going to do this to my understanding.

> 
> Could we easily modify UI Tests app to enable us to reach this prompt?

Nope. You would need to create a different test app in the test_apps directory that is privileged that would have the appropriate permissions (contacts & device storage). Then, you would need to access those apps to trigger the JS code to trigger the prompts, allow access to each prompt, and verify that permissions were allowed in the JS code for each prompt.

Note - Technically this could be done either Marionette JS or Gaia UI Test.

Comment 3

[Zac C (:zac)]

OK, assigning it to Al!

Comment 4

[Al Tsai [:atsai]]

Thanks for opening the bug. I'll start to work on it. :D

Comment 5

[Al Tsai [:atsai]]

(In reply to Jason Smith [:jsmith] from comment #2)
> (In reply to Zac C (:zac) from comment #1)
> > Shih-Chiang, is this the same prompt you get when you go into UI Tests app,
> > tap API menu and then go into "getUserMedia" option?
> 
> Nope. This wouldn't be triggered in that app, as the UI Tests app is
> certified & this is a different permission prompt (gUM perm prompt is
> different than the contacts/device storage prompt).
> 
> > 
> > We can write this test easily if there is a way we can do it in Gaia now but
> > if we have to write an app in which to do it it is probably outside the
> > ability of my team right now.
> 
> Why would this be outside of the ability of your team? Al already said he
> was going to do this to my understanding.

To trigger the prompt in gaia, you'll need to write a test app and use it to send a request for accessing contact info. It will make things more complicated. I prefer to separate the tests into two parts.

A) Write a marionette JS to verify the gecko event is correct. the test will land with the patch in 853356.
B) Create a bug and enhance gaia-ui-test to support such case. We'll need to accomplish these kinds of test.

The bug will solve part A only. I'll keep check with SC if we can have other better solution for it.

> > 
> > Could we easily modify UI Tests app to enable us to reach this prompt?
> 
> Nope. You would need to create a different test app in the test_apps
> directory that is privileged that would have the appropriate permissions
> (contacts & device storage). Then, you would need to access those apps to
> trigger the JS code to trigger the prompts, allow access to each prompt, and
> verify that permissions were allowed in the JS code for each prompt.
> 
> Note - Technically this could be done either Marionette JS or Gaia UI Test.

Comment 6

[Al Tsai [:atsai]]

Sorry for bringing wrong information. Please ignore Comment 5.

I'll add necessary app to "UI Test" app in testapps and use it to verify the functionality.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Al Tsai [:atsai] from comment #6)
> Sorry for bringing wrong information. Please ignore Comment 5.
> 
> I'll add necessary app to "UI Test" app in testapps and use it to verify the
> functionality.

That isn't going to work. The UI Test app is a certified app, not a privileged app. These permission prompts won't trigger in a certified app. You need to use a test app that's privileged to test this.

Comment 8

[Al Tsai [:atsai]]

(In reply to Jason Smith [:jsmith] from comment #7)
> (In reply to Al Tsai [:atsai] from comment #6)
> > Sorry for bringing wrong information. Please ignore Comment 5.
> > 
> > I'll add necessary app to "UI Test" app in testapps and use it to verify the
> > functionality.
> 
> That isn't going to work. The UI Test app is a certified app, not a
> privileged app. These permission prompts won't trigger in a certified app.
> You need to use a test app that's privileged to test this.

You are right. I'll create a separate privileged app for the case.

Comment 9

[Ivan Tsay (:ITsay)]

Hi Al,

Would you please to provide a targeted milestone for this privilege test app? Thank you!

Comment 10

[Al Tsai [:atsai]]

I'll try to finish the developing before the end of next Tuesday.

Comment 11

[Al Tsai [:atsai]]

Attachment: patch for testing scripts

Comment 12

[Askeing Yen[:askeing][:fyen]]

Comment on attachment 8351353 [details] [review]
patch for testing scripts

run 3 tests with repeat=9 on Buri 20140102040201 m-c build, and it works fine.

SUMMARY
-------
passed: 30
failed: 0
todo: 0

Comment 13

[Askeing Yen[:askeing][:fyen]]

(In reply to Askeing Yen[:askeing] from comment #12)
> Comment on attachment 8351353 [details] [review]
> patch for testing scripts
> 
> run 3 tests with repeat=9 on Buri 20140102040201 m-c build, and it works
> fine.
> 
> SUMMARY
> -------
> passed: 30
> failed: 0
> todo: 0
works fine with --type=b2g gaiatest/tests/functional/system/manifest.ini on b2gdesktop, too.

Comment 14

[Zac C (:zac)]

Comment on attachment 8351353 [details] [review]
patch for testing scripts

r-, there is loads of unused test code there.

I am a little unsure about the new test app - I am worried about who will own the app in the future.

Is it useful for other teams too, long term?

Comment 15

[Jason Smith [:jsmith]]

Comment on attachment 8351353 [details] [review]
patch for testing scripts

Adding feedback- as I don't think this is on the right path to the solution here. Let's back up and think about this again from a clean slate. Here's what you need to do:

1. Build a reduced simple privileged app that can trigger the contacts API permission prompt & device storage permission prompts

2. Put [1] out for review & get a r+

3. Land patch [1]

4. Build the relevant gaia ui test to trigger each permission prompt & validate results from the permission prompt interaction

5. Put [3] out for review & get a r+

6. Land patch [4]

Comment 16

[Al Tsai [:atsai]]

Attachment: patch for testing scripts

remove unnecessary files in uitests-privileged app
add device-storage prompt testing

Comment 17

[Jason Smith [:jsmith]]

Comment on attachment 8356966 [details] [review]
patch for testing scripts

This definitely looks like the right approach. The UI Tests app code is reduced to focus on the specific problem here a lot more.

Note - my feedback didn't really focus on the code side of things, as I'll let Zac & Askeing comment on that area.

Comment 18

[Zac C (:zac)]

Comment on attachment 8356966 [details] [review]
patch for testing scripts

r- need to break up the tests into files.
Aside from that it looks good and I can re-review asap.

Comment 19

[Askeing Yen[:askeing][:fyen]]

Comment on attachment 8356966 [details] [review]
patch for testing scripts

r+
After seperated test into four tests, it's look good to me.

Comment 20

[Al Tsai [:atsai]]

Comment on attachment 8356966 [details] [review]
patch for testing scripts

break the device_storage test into four sections

Comment 21

[Zac C (:zac)]

Comment on attachment 8356966 [details] [review]
patch for testing scripts

test_privileged_app_audio_capture_prompt.py is failing.

Is this expected? 

If it is expected then set it to `expected = fail` with a bug number so we can track it, inside the manifest file.




======================================================================
FAIL: None
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/zac/.virtualenvs/marionette_0_7_2/local/lib/python2.7/site-packages/marionette_client-0.7.2-py2.7.egg/marionette/marionette_test.py", line 143, in run
    testMethod()
  File "/home/zac/Mozilla/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/system/test_privileged_app_audio_capture_prompt.py", line 33, in test_audio_capture_prompt
    self.assertEqual(current_permission, 'allow')
TEST-UNEXPECTED-FAIL | test_privileged_app_audio_capture_prompt.py test_privileged_app_audio_capture_prompt.TestPrivilegedAppAudioCapturePrompt.test_audio_capture_prompt | AssertionError: u'prompt' != 'allow'
----------------------------------------------------------------------

Comment 22

[Al Tsai [:atsai]]

Comment on attachment 8356966 [details] [review]
patch for testing scripts

Checked w/ Developer and confirmed Jason's comment is correct. The behavior is that apps always need to ask for permission to access audio (microphone).


Change the type to prompt and resent for review

Comment 23

[Zac C (:zac)]

Comment on attachment 8356966 [details] [review]
patch for testing scripts

r+

Comment 24

[Zac C (:zac)]

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

Comment 25

[Carsten Book [:Tomcat]]

(In reply to Zac C (:zac) from comment #24)
> Merged:
> https://github.com/mozilla-b2g/gaia/commit/
> e9fe7586d1d884963d62e09530b8011196f6167d

backed this change out from gaia because it might caused bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=33154962&tree=B2g-Inbound

Comment 26

[StevenLee[:slee]]

Hi Tomcat,

I tried atsai's patch on Windows and Mac but I cannot reproduce the error. I also found that before atsai's patch, the build error problem exists.
Here are the logs.

* https://tbpl.mozilla.org/?rev=38ef8ca5644b&tree=B2g-Inbound
* https://tbpl.mozilla.org/?rev=2003bef38b9e&tree=B2g-Inbound

Could we re-land the patches since it blocks bug 853356 which is a blocking bug?

Thanks.

Comment 27

[Carsten Book [:Tomcat]]

yeah steven yeah feel free to land!

Comment 28

[StevenLee[:slee]]

(In reply to Carsten Book [:Tomcat] from comment #27)
> yeah steven yeah feel free to land!

Sure, I will talk to atsai.
Thanks.

Comment 29

[Al Tsai [:atsai]]

Attachment: resend pull request patches for the same contents

resend the pull requests

Comment 30

[Askeing Yen[:askeing][:fyen]]

Comment on attachment 8364123 [details] [review]
resend pull request patches for the same contents

r+ base on attachment 8356966 [details] [review]

Comment 31

[Al Tsai [:atsai]]

Merged 
https://github.com/altsai/gaia/commit/0214caef6fd00eb5ae5f439d911766ba4c15ed2d

Comment 32

[Florin Strugariu [:Bebe]]

This is breaking the automation test run:

http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.2/92/console

Is failing because of this.

And also the patch is missing a file:
test file: /var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.2/tests/python/gaia-ui-tests/gaiatest/tests/functional/system/test_privileged_app_device_storage_prompt.py does not exist

Comment 33

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Reverted for causing device failures:
https://github.com/mozilla-b2g/gaia/commit/9fba58cd97022041dd804961c8587a8f45950e63

Comment 34

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: console.log

Attached Jenkins console log showing errors.

Comment 35

[Florin Strugariu [:Bebe]]

Attachment: Jenkins Log

Comment 36

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8364392 [details]
Jenkins Log

I already attached the log in this case.

Comment 37

[Al Tsai [:atsai]]

I am checking the root cause on my laptop now. Sorry for making mess.

Comment 38

[Al Tsai [:atsai]]

Attachment: pr bug948826

fix bugs.

Comment 39

[Askeing Yen[:askeing][:fyen]]

Comment on attachment 8365819 [details] [review]
pr bug948826

after cherry-pick commit and ./build.sh, the uitest-privileged.gaiamobile.org is displayed under gaia/profile/webapps folder.

Comment 40

[Al Tsai [:atsai]]

Merged
https://github.com/mozilla-b2g/gaia/pull/15710

Comment 41

[Zac C (:zac)]

The rel-eng build does not seem to have picked this up as one of the test apps so all of this automation is failing.

It's ok on CI environments that build their own profiles.

Comment 42

[Al Tsai [:atsai]]

I think we need to have in rel-eng build and have it tested regularly. To keep the radar working. I'd like to disable the test from manifest until rel-eng add it to test app list.

Jason, due to I am in CNY holidays, would you mind to talk with rel-eng team to make it available for CI server? Thanks a lot.

Comment 43

[Maire Reavy [:mreavy]]

I'd like to land bug 853356 (which is the last bug needed to enable the gUM camera/video feature) ASAP. All the patches on that bug have r+, and this bug (for permission prompt tests) is now "resolved". 

jsmith  -- Are the tests from this bug in place so that bug 853356 can land?  (We've tried to land bug 853356 in the past, and it has bounced.  I'm trying to avoid another bounce.  Thanks.)

Comment 44

[Jason Smith [:jsmith]]

(In reply to Al Tsai [:atsai] from comment #42)
> I think we need to have in rel-eng build and have it tested regularly. To
> keep the radar working. I'd like to disable the test from manifest until
> rel-eng add it to test app list.
> 
> Jason, due to I am in CNY holidays, would you mind to talk with rel-eng team
> to make it available for CI server? Thanks a lot.

Let's ask Aki. I actually don't know why this app isn't included in the rel eng build.

Comment 45

[Zac C (:zac)]

(In reply to Al Tsai [:atsai] from comment #42)
> I think we need to have in rel-eng build and have it tested regularly. To
> keep the radar working. I'd like to disable the test from manifest until
> rel-eng add it to test app list.
> 
> Jason, due to I am in CNY holidays, would you mind to talk with rel-eng team
> to make it available for CI server? Thanks a lot.

Al sorry I forgot to clear your ni? but this worked on the following day's build. I don't know why.
The tests are fine now.