Closed Bug 815105 Opened 12 years ago Closed 11 years ago

Implement Suite 2 WebAPI testing

Categories

(Core :: Permission Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: pauljt, Assigned: dchanm+bugzilla)

References

(Depends on 1 open bug, )

Details

Attachments

(1 file, 8 obsolete files)

Implement tests which ensure that an app that has been granted permissions can access the relevant API, and vice versa.
No longer blocks: 815110
Depends on: 815110
Attached file WIP Permission Tests (obsolete) (deleted) —
Attached file WIP permissions table (obsolete) (deleted) —
Attached file WIP mochitest (obsolete) (deleted) —
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.
PS: currently tests need to be in /tests/webapi for paths to work.
Attached patch Test Suite 2 for WebAPI permission testing v1 (obsolete) (deleted) — Splinter Review
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
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.
Component: DOM: Device Interfaces → DOM: Apps
Attached patch Test suite 2 v2 (obsolete) (deleted) — Splinter Review
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: nobody → dchan+bugzilla
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.
Component: DOM: Apps → Permission Manager
David, do we need to line up reviewers for this, or is this WIP?
Flags: needinfo?(dchan+bugzilla)
(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)
Attached patch Test suite 2 v3 (obsolete) (deleted) — Splinter Review
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
Depends on: 855615, 859593, 795716
(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)
(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)
Attached patch Test suite 2 v4 (obsolete) (deleted) — Splinter Review
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
Who should I ask to review the patch?
Flags: needinfo?(overholt)
I'm CCing Andrea here who said he will take a look.
Flags: needinfo?(overholt)
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+
(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
Attached patch Test suite 2 v5 (obsolete) (deleted) — Splinter Review
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)
Above try run was all green. I had to restart a couple tests due to intermittent failures.
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+
Attached patch Test suite 2 v6 (deleted) — Splinter Review
Fixed the extra spaces. Carrying over the r+
Attachment #754891 - Attachment is obsolete: true
Attachment #755971 - Flags: review+
Keywords: checkin-needed
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.

Attachment

General

Created:
Updated:
Size: