Closed
Bug 815105
Opened 12 years ago
Closed 11 years ago
Implement Suite 2 WebAPI testing
Categories
(Core :: Permission Manager, defect)
Core
Permission Manager
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: pauljt, Assigned: dchanm+bugzilla)
References
(Depends on 1 open bug, )
Details
Attachments
(1 file, 8 obsolete files)
(deleted),
patch
|
dchanm+bugzilla
:
review+
|
Details | Diff | Splinter Review |
Implement tests which ensure that an app that has been granted permissions can access the relevant API, and vice versa.
Reporter | ||
Updated•12 years ago
|
Reporter | ||
Comment 1•12 years ago
|
||
Reporter | ||
Comment 2•12 years ago
|
||
Reporter | ||
Comment 3•12 years ago
|
||
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.
Reporter | ||
Comment 5•12 years ago
|
||
Output from current emulator tests: http://pastebin.mozilla.org/1967318
Reporter | ||
Comment 6•12 years ago
|
||
PS: currently tests need to be in /tests/webapi for paths to work.
Reporter | ||
Comment 7•12 years ago
|
||
Tests are still blocked on 815110 but here is a patch with these tests disabled.
Attachment #686526 -
Attachment is obsolete: true
Attachment #686527 -
Attachment is obsolete: true
Attachment #686528 -
Attachment is obsolete: true
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.
Updated•12 years ago
|
Component: DOM: Device Interfaces → DOM: Apps
Assignee | ||
Comment 9•12 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
Attachment #693835 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → dchan+bugzilla
Assignee | ||
Comment 10•12 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.
Updated•12 years ago
|
Component: DOM: Apps → Permission Manager
David, do we need to line up reviewers for this, or is this WIP?
Flags: needinfo?(dchan+bugzilla)
Assignee | ||
Comment 12•12 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.
Flags: needinfo?(dchan+bugzilla)
Assignee | ||
Comment 13•12 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
Attachment #731356 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Comment 14•12 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?
Flags: needinfo?(dchan+bugzilla)
Assignee | ||
Comment 15•12 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.
Flags: needinfo?(dchan+bugzilla)
Assignee | ||
Comment 16•12 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
Attachment #734941 -
Attachment is obsolete: true
Comment 18•12 years ago
|
||
I'm CCing Andrea here who said he will take a look.
Flags: needinfo?(overholt)
Comment 19•12 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...
Attachment #752407 -
Flags: review+
Assignee | ||
Comment 20•12 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
Assignee | ||
Comment 21•12 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
Attachment #752407 -
Attachment is obsolete: true
Attachment #754891 -
Flags: review?(amarchesini)
Assignee | ||
Comment 22•12 years ago
|
||
Above try run was all green. I had to restart a couple tests due to intermittent failures.
Comment 23•12 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.
Attachment #754891 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 24•12 years ago
|
||
Fixed the extra spaces. Carrying over the r+
Attachment #754891 -
Attachment is obsolete: true
Attachment #755971 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 25•12 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 26•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•