Paul Theriault [:pauljt] (no longer reading bugmail) Reporter
	Description • 12 years ago

Implement tests which ensure that an app that has been granted permissions can access the relevant API, and vice versa.

	Paul Theriault [:pauljt] (no longer reading bugmail) Reporter
	Comment 4 • 12 years ago

Work in progress in zip file above. Approach has been to develop the tests on the device, see http://allperms.creativemisuse.com for latest version.


Currently non-working tests:

* contacts-write - error in the test since I can't write with contacts readwrite permission. (the other contacts tests work)
* desktop-notification: cant detect success in an automated fashion (maybe by interacting with the service itself? not sure how though)
* embed-apps: test works but it is a total hack - what is the standard way to talk from a child mozbrowser iframe to its parent? 
* geolocation: test is broken, was working, not sure what happened.
* idle: no test yet. Just need to catch idle event i think. Not sure what the permission means though
* network-events: test broken, events never received, even with permission.
*wifi-manage: navigator.mozWifiManager isnt defined and I dont know why not...

Also, for testing without permissions, attempting to access some APIs causes the child process to be killed see bug 815510.

The file "test_all_permissions.html" takes these same tests and runs them are different app levels in mochitest, but I am getting very different results in the emulator to the device - maybe because so far on the device I am only testing all perms (certified) and no perms, not the in between state.

	Paul Theriault [:pauljt] (no longer reading bugmail) Reporter
	Comment 5 • 12 years ago

Output from current emulator tests: http://pastebin.mozilla.org/1967318

	Paul Theriault [:pauljt] (no longer reading bugmail) Reporter
	Comment 6 • 12 years ago

PS: currently tests need to be in /tests/webapi for paths to work.

	Paul Theriault [:pauljt] (no longer reading bugmail) Reporter
	Comment 7 • 12 years ago

Tests are still blocked on 815110 but here is a patch with these tests disabled.

	David Chan [:dchan] Assignee
	Comment 8 • 12 years ago

The patch doesn't apply cleanly for me. I think your editor changed the leading tab on the $(INSTALL) line to spaces, or maybe it change when I imported.

	David Chan [:dchan] Assignee
	Comment 9 • 11 years ago

I've updated the patch. Try push here
https://tbpl.mozilla.org/?tree=Try&rev=662225889506

I believe only a whitelist of B2G tests are run on try. You can run the tests on your local emulator / device. There should be 96 passes

There are a couple outstanding issues that need to be resolved, but the framework is mostly there

the browser and embed-apps test have problems on desktop builds and I'm debugging that. Remaining tests that need to be written / completed are

background-sensors
contacts
audio-channel-*
network-events
device-storage
desktop-notification
geolocation

background-senors needs to be finished. I'm trying to figure out how to put the window in the background.

contacts needs to be written

audio-channel is blocked on bug 855615

network-events doesn't seem to work and the only test on the tree is disabled bug 795716 . Though my testing seesm to show that the event is broken

device-storage tests have not been written yet

desktop-notifcation and geolocation are blocked by some problems with importing MockPermissionPrompt into the emulator/device. I need to file a bug on that


The rest of the tests were tested working in the emulator build synced 03/24 . The tests mostly follow what was described a while ago.

1. mochitest loads test_PERM.html
2. test loads framework.js
3. test creates iframe to example.org/.../shim.html
4. test checks that example.org doesn't have permissions
5. test checks that running test on example.org fails
6. test adds permissions to example.org .
7. test creates a second iframe to example.org . This is done since some webapis are intialized with the window object
8. test checks that running the test on second iframe succeeds

test_PERM.html controls the tests by sending a function to shim.html to execute. This gets around the need for needing chrome privileges to manipulate the child frame. This also means that the mochi.test app and test_PERM.html don't pollute the test environment. 

Each test defines a gData object which is used by the framework
perm (required): Array of permissions required for the test
needParentPerm (optional): Whether the parent process requires the permission as well. Otherwise AppProcessChecker will kill the parent
obj (optional): navigator object to test for
idl (optional): expected interface that obj should be
settings (optional): settings to change before running tests
verifier (optional): custom verification function to use
skip (optional): skip this test

	David Chan [:dchan] Assignee
	Comment 10 • 11 years ago

For completeness, the following permissions won't have mochitests

deprecated-hwvideo - This isn't used anywhere yet. See bug 793034
storage - This is more of a convience permission to underlying storage mechanism in firefox indexedDB-unlimited, offline-app, pin-app

backgroundservice, attention, open-remote-window - these are all gaia specific permissions and not WebAPI ones.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07
	Comment 11 • 11 years ago

David, do we need to line up reviewers for this, or is this WIP?

	David Chan [:dchan] Assignee
	Comment 12 • 11 years ago

(In reply to Geo Mealer [:geo] from comment #11)
> David, do we need to line up reviewers for this, or is this WIP?

We'll need some reviewers soon. I'm cleaning up small bugs in the tests. Latest try push at
https://tbpl.mozilla.org/?tree=Try&rev=8bf6a0914153

This switches to using bug 685652 for permissions management.

	David Chan [:dchan] Assignee
	Comment 13 • 11 years ago

This is the patch for the previous try run

Outstanding issues
1. Fix "alarms" and "permissions" permission on android
2. Fix mozapp and mozbrowser detection on b2g
3. Add remaining tests listed in previous patch
4. Fix DOMCameraManager assertion, see bug 859593

Fixes since last patch
1. Fixed idle test on desktop builds
2. Fixed permissions leakage by switching to pushPermissions. Bug 859401 may need to land first to handle PROMPT_ACTION

	Andrew Overholt [:overholt]
	Comment 14 • 11 years ago

(In reply to David Chan [:dchan] from comment #13)
> Created attachment 734941 [details] [diff] [review]
> Test suite 2 v3
> 
> This is the patch for the previous try run

Do we need reviewers here now?

	David Chan [:dchan] Assignee
	Comment 15 • 11 years ago

(In reply to Andrew Overholt [:overholt] from comment #14)
> (In reply to David Chan [:dchan] from comment #13)
> > Created attachment 734941 [details] [diff] [review]
> > Test suite 2 v3
> > 
> > This is the patch for the previous try run
> 
> Do we need reviewers here now?

Feedback would be appreciated. The tests will have to change once bug 859554 lands.

	David Chan [:dchan] Assignee
	Comment 16 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=826901600b79

This covers most of the WebAPI tests. Uncovered tests are

background-sensors
contacts
audio-channel-*
network-events
device-storage
desktop-notification
geolocation
camera

I will file a follow up to fix the remaining tests

Tests that skip certain platforms. Skipping of tests only affects the second part of the tests where the check is that the appropriate API is accessible. In those cases, a todo() is used instead of an ok()

Android
alarms
permissions
embed-app

The alarms and permissions interface are not included in Android builds
embed-app requires a working webapp runtime on Android

B2G
embed-app

The b2g mochitest setup results in the wrong appStatus due to iframe nesting. The iframe hierarchy looks as follows
- test-container app
-- mochitest iframe
--- isolated iframe used for permissions tests (example.org)
----- embed mozbrowser mozapp iframe example.org app

Since the example.org app iframe is different origin than the test-container app, it gets an appStatus of NOT_INSTALLED even with the appropriate permissions

	David Chan [:dchan] Assignee
	Comment 17 • 11 years ago

Who should I ask to review the patch?

	Andrew Overholt [:overholt]
	Comment 18 • 11 years ago

I'm CCing Andrea here who said he will take a look.

	Andrea Marchesini [:baku]
	Comment 19 • 11 years ago

Comment on attachment 752407 [details] [diff] [review]
Test suite 2 v4

Review of attachment 752407 [details] [diff] [review]:
-----------------------------------------------------------------

First of all, this is a wonderful test suite! Thanks.
I have a few questions:

1. can you check that without permissions getObj() fails?
2. maybe would be nice to have documentation about gData and its properties.

Is it fully green on try?

::: dom/permission/tests/file_framework.js
@@ +26,5 @@
> +                        });
> +
> +  this.setupParent = false;
> +  this.perms = expandPermissions(aData.perm);
> +  this.uuid = UUIDSvc.generateUUID().toString();

Why UUID and not just a simple progressive ID? I'm just wondering if this is too heavy or not...

	David Chan [:dchan] Assignee
	Comment 20 • 11 years ago

(In reply to Andrea Marchesini (:baku) from comment #19)
> Comment on attachment 752407 [details] [diff] [review]
> Test suite 2 v4
> 
> Review of attachment 752407 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> First of all, this is a wonderful test suite! Thanks.
> I have a few questions:
> 
> 1. can you check that without permissions getObj() fails?

The first run of the test attempts to getObj() without permissions. Depending on the API, this will either return null or throw an exception. Since the returned object isn't of the expected IDL type, the test fails. Once bug 859554 lands, then the logic may have to be changed to test null vs undefined.

> 2. maybe would be nice to have documentation about gData and its properties.

I'll add a description of gData to the framework file and work with sheppy to get this stuff documented on MDN.

> 
> Is it fully green on try?
> 

It is green aside from unrelated intermittent test failures. I'll resubmit after addressing your concerns.

> ::: dom/permission/tests/file_framework.js
> @@ +26,5 @@
> > +                        });
> > +
> > +  this.setupParent = false;
> > +  this.perms = expandPermissions(aData.perm);
> > +  this.uuid = UUIDSvc.generateUUID().toString();
> 
> Why UUID and not just a simple progressive ID? I'm just wondering if this is
> too heavy or not...

I don't recall why I'm using a UUID. I think using "step" would work as well. Let me change that and submit to try

	David Chan [:dchan] Assignee
	Comment 21 • 11 years ago

Try run at
https://tbpl.mozilla.org/?tree=Try&rev=54e62bf57817

I added documentation about gData in file_framework.js

UUID was changed to a progressive ID, though I think ID is only set once for current tests. The original design was to allow a test file to contain multiple related tests e.g. tests for the various contact permissions could be one file.

Local run on my linux build shows 57 passes. and 91 passes on b2g emulator with 1 todo

	David Chan [:dchan] Assignee
	Comment 22 • 11 years ago

Above try run was all green. I had to restart a couple tests due to intermittent failures.

	Andrea Marchesini [:baku]
	Comment 23 • 11 years ago

Comment on attachment 754891 [details] [diff] [review]
Test suite 2 v5

Review of attachment 754891 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

::: dom/permission/tests/file_framework.js
@@ +24,5 @@
> + *         on. The tests still verify that you can't get obj on those
> + *         platforms without permissions, however it is expected that adding
> + *         the permission still won't allow access to those objects
> + *
> + * settings (optional) Array of preference tuples 

extra space.

@@ +32,5 @@
> + *         will fail the initial test
> + *
> + * verifier (optional) Function
> + *         A function used to test whether a WebAPI is accessible or not.
> + *         The function takes a success and failure callback which both 

extra space.

	David Chan [:dchan] Assignee
	Comment 24 • 11 years ago

Fixed the extra spaces. Carrying over the r+

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/533aeba067b9

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/533aeba067b9