Open Bug 811141 Opened 7 years ago Updated 7 years ago

Implement Suite 1 and Suite 3 of WebAPI testing

Categories

(Core :: Permission Manager, defect)

defect
Not set

Tracking

()

People

(Reporter: dchanm+bugzilla, Assigned: dchanm+bugzilla)

References

()

Details

Attachments

(1 file, 6 obsolete files)

Tracking but for implementing the installer permissions testing required for overall WebAPI testing.
Attached patch Test suite 1+3 for webapis (obsolete) — Splinter Review
These tests work on nightly. There are some eventhandling bugs that needed to be fixed for testing on b2g.

Last run showed 492 tests passed. 2 for testing whether the document gets the correct permissions. The other 490 are divided between app install, app uninstall and all permutations of requesting a single permissions X across the web, app, privileged and certified contexts.
Assignee: nobody → dchan+bugzilla
Attached patch Test suite 1+3 for webapis v2 (obsolete) — Splinter Review
fix typo is test path
Attachment #680871 - Attachment is obsolete: true
Attached patch Test suite 1+3 for webapis v3 (obsolete) — Splinter Review
Update the patch to also test the case of a manifest with all and no permissions. Tests now also check that no extra permissions are set above those requested in manifest.

Count of tests run is 10,616
Attachment #680877 - Attachment is obsolete: true
Attached patch Test suite 1+3 for webapis v4 (obsolete) — Splinter Review
This patch works on the latest Nightly build with 610 tests passing. Calls to is/ok were consolidated per app vs the old method of calling is/ok for each permission check. There are some outstanding issues that prevent it from working on b2g emulator/device, but it doesn't affect the correctness.

The tests check the following.
- Check that PermissionsInstaller.permissionsTable matches the gData table in the test
-- Same permissions
- Check that PermissionsInstaller.expandPermissions expands permissions to the same as "expected" in gData
- Checks that permissions for web content are intially UNKNOWN_ACTION
- Checks that installing an app with permission X installs the correct permissions at correct access level
- Checks permissions install for manifest which requests every permission
- Checks permissions install for manifest which requests no permission

If permissions are changed or added to PermissionsInstaller.permissionsTable then some of these tests will fail.

Remaining work to do
- Fix test for b2g emulator / device
Attachment #681789 - Attachment is obsolete: true
Attachment #685386 - Flags: feedback?(ddahl)
Comment on attachment 685386 [details] [diff] [review]
Test suite 1+3 for webapis v4

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

Looks like the right approach to me.

::: dom/apps/tests/test_install.html
@@ +14,5 @@
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +</div>
> +<pre id="test">
> +<script type="application/javascript;version=1.7" src="install_permissions.js"></script>

Just curious: Should the version be 1.8? is there a reason to set this to 1.7?

@@ +149,5 @@
> +function createPrincipal(aURI, aType) {
> +  switch(aType) {
> +    case "web":
> +      var uri = Services.io.newURI(aURI, null, null);
> +      // dchan - is this the correct princpal to get for web content?

I would ask geekboy about this next line

@@ +224,5 @@
> +    default:
> +      perms.push(aPerm);
> +    break;
> +  }
> +  

nit: does a pink space indicate a tab char in splinter reviews?

@@ +248,5 @@
> +function install(aURI, aCallback) {
> +  var req = window.navigator.mozApps.install(aURI);
> +  req.onsuccess = function _onsuccess() {
> +    ok(true, "Successfully installed");
> +    // dchan: install is async, is there a better way?

no, this is the way to do it

::: dom/apps/tests/webapp.sjs
@@ +28,5 @@
> +  // avoid confusing cache behaviors
> +  response.setHeader("Cache-Control", "no-cache", false);
> +
> +  response.setHeader("Content-Type", "application/x-web-app-manifest+json", false);
> +  response.write(JSON.stringify(manifest));

I have never written an sjs file, but this looks ok to me.:)
Attachment #685386 - Flags: feedback?(ddahl) → feedback+
Attached patch Test suite 1+3 for webapis v5 (obsolete) — Splinter Review
I cleaned up some of the extra spaces and addressed the feedback from ddahl. I also removed some tests since they were extraneous (ok on install / uninstall).

The tests don't work on emulator due to multiple issues
1. importing PermissionsInstaller.jsm will fail in emulator mochitest since tests are run out of process
2. There is no way to fake a user accept for prompt events. See bug 813814

I will file a bug to fix 1. At this time, the tests pass on Nightly Firefox (364 in total)
Attachment #685386 - Attachment is obsolete: true
Comment on attachment 686784 [details] [diff] [review]
Test suite 1+3 for webapis v5

:gwagner

Are you the correct person to review this patch? The testcase is designed to check for any changes to how permissions are installed for each type of app.

Adding, removing or modifying access levels/substitutions in PermissionsInstaller.jsm will result in test failure. There are ~120 app installs, ~40 checks for default permissions, ~40 checks for permissions expansion.
Attachment #686784 - Flags: review?(anygregor)
Attached patch Test suite 1+3 for webapis v6 (obsolete) — Splinter Review
Fixed up the tests to account for new permissions and removals. 337 tests in total now

Try push
https://tbpl.mozilla.org/?tree=Try&rev=c443b2c2b482

However I believe the tests may fail on android. If so, I will disable from running on android and file a bug to fix that.
Attachment #686784 - Attachment is obsolete: true
Attachment #686784 - Flags: review?(anygregor)
Attachment #689474 - Flags: review?(jonas)
It would help if I attached the actual patch
Attachment #689474 - Attachment is obsolete: true
Attachment #689474 - Flags: review?(jonas)
Attachment #689486 - Flags: review?(jonas)
Comment on attachment 689486 [details] [diff] [review]
Test suite 1+3 for webapis v7

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

It's somewhat annoying that we'll have to keep this big table in sync with the table in the PermissionsInstaller :(

Especially since this doesn't actually test a whole lot other than that the permissions installer works. I.e. it doesn't test that any of the APIs actually honor the values in the permissions database.

Could we perhaps read the data from the table in the permissioninstaller instead?

All in all, we should be aware that this doesn't actually test anything other than the PermissionsInstaller code. I.e. this doesn't actually test that any APIs are behaving like they should. That is fine, it just means that we still have a lot of tests to write.
(In reply to Jonas Sicking (:sicking) from comment #10)
> Comment on attachment 689486 [details] [diff] [review]
> Test suite 1+3 for webapis v7
> 
> Review of attachment 689486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's somewhat annoying that we'll have to keep this big table in sync with
> the table in the PermissionsInstaller :(
> 
> Especially since this doesn't actually test a whole lot other than that the
> permissions installer works. I.e. it doesn't test that any of the APIs
> actually honor the values in the permissions database.
> 
> Could we perhaps read the data from the table in the permissioninstaller
> instead?
> 

There is a part that imports PermissionInstaller to see if any permissions have changed.

> All in all, we should be aware that this doesn't actually test anything
> other than the PermissionsInstaller code. I.e. this doesn't actually test
> that any APIs are behaving like they should. That is fine, it just means
> that we still have a lot of tests to write.

The intended scope of our tests may have been too large. The 3 goals of the test suite were
- Check default permission DB for app/content with no permissions
- Catch any changes to the permissions table
- Catch any changes to how "expandPermissions" works

The second and third goal require maintaining the giant table. However as you mentioned, it isn't ideal. Also the tests don't run in a reasonable amount of time if we have to iterate over all permission permutations. 

A test for all and no permissions was suggested earlier. That may be sufficient to cover most scenarios. It wouldn't catch the scenario of an app asking for permissions X but also being granted Y as well (ignoring additional / substitute / access / channel modifications)

The current tests are also too slow for CI due to sheer amount of apps installed / uninstalled.

Geo: What are your thoughts on reducing the scope to only test all / no permissions?
Flags: needinfo?(gmealer)
David, if we need to reduce scope to make the test suites reasonable to run, I think that's an OK thing. We just need to understand what's being tested.

Re: testing PermissionsInstaller, keep in mind we have suite 2 in Bug 815105 that should be testing permission behavior. The idea was between the three suites to get the whole chain of permissions handling from the install downwards. So testing the installer was appropriate here.

Re: testing against the table, what would we be testing if we read the table at runtime? Aren't we testing that the runtime table matched design-time specs? Ultimately you maintain things like big golden tables because those match the specifications. We can decide it's not worth it, of course.
Flags: needinfo?(gmealer)
Er, correcting above, *suite 2...that should be testing API behavior
So, from my point of view, this patch test the following:

* That the PermissionsInstaller code accurately puts data into the permissions
  database when an app is installed.
* That the table in the PermissionsInstaller contains the data that we expect.

It does *not* test the following:

* That any of the APIs behave as appropriate based on data from the manifest.

The last thing is IMO the most important thing to test. But it's also the thing that is the most work to test, so it's understandable that we're not attacking that here. But we should be aware that we're not testing any of the critical checks here.


I'm not super exited about having a test for the full table in the PermissionsInstaller. It just seems like something that will be a pain to maintain for now. Especially since we're making changes to that table fairly often still. So it'll just create busy-work to keep the two tables in sync.


The first bulletpoint above is useful though. But we can test that without testing the full table. So just pick 3-4 permissions of different type (with and without access field, etc) and make sure that they get installed correctly. That should ensure that the PermissionsInstaller code is correct.
Comment on attachment 689486 [details] [diff] [review]
Test suite 1+3 for webapis v7

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

Minusing based on that. Basically all you need to do is to reduce the size of the table here so that it doesn't try to cover every single permission.
Attachment #689486 - Flags: review?(jonas) → review-
> The last thing is IMO the most important thing to test. But it's also the
> thing that is the most work to test, so it's understandable that we're not
> attacking that here. But we should be aware that we're not testing any of
> the critical checks here.

That is the purpose of the test suite I am working on in bug 815105. I have working tests (for most APIs) but I am struggling getting loading and assigning permissions correctly all working. (I'm blocked on 816847, so I will go back to this after completeing the audit stuff in 777602)
expandPermission has some test coverage in bug 808734, though this doesn't account for the new permissions and "channel". The test could easily be modified to read permissions from PermissionsInstaller and generate a single manifest with all the permissions. This would cover all the possible expansion / substitution cases. PermissionsInstaller would also have to expose AllPossiblePermissions to account for new changes such as channel

The total number of tests would be reduced to 7
- webcontent
- webapp all + none
- privileged all + none
- certified all + none
Up to you guys what you want to test where. I just don't think that the approach in the patch of trying to test the whole table (i.e. the second bullet in comment 14) is very beneficial.

Other than that I'm happy to leave this up to you.
David, the approach you outline in comment 17 sounds fine.

Jonas, re: comment 14, we're definitely clear that these aren't the API tests, and that those are critical. 

Part of the reason to separate these concerns out was to make the path to the API tests more straightforward, as well as get some isolation for easier debugging if we do end up with failures in the chain somewhere.
Component: DOM: Device Interfaces → DOM: Apps
Component: DOM: Apps → Permission Manager
You need to log in before you can comment on or make changes to this bug.