Permaorange in test_SitePermissions.js | testPermissionsListing - [testPermissionsListing : 10] Correct list of all permissions when Gecko 55 merges to beta on 2017-06-12

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Site Identity and Permission Panels
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: philor, Assigned: Fischer)

Tracking

55 Branch
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55+ fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
You've still got 7 weeks, the merge isn't until 2017-06-12, but when it happens, xpcshell on every platform is going to be permaorange like https://treeherder.mozilla.org/logviewer.html#?job_id=93212440&repo=try (which you should be able to repro locally by just changing /config/milestone.txt from 55a1 to 55), "test_SitePermissions.js | testPermissionsListing - [testPermissionsListing : 10] Correct list of all permissions - ["camera","cookie","desktop-notification","focus-tab-by-prompt","geo","image","indexedDB","install","microphone","popup","screen deepEqual ["camera","cookie","desktop-notification","focus-tab-by-prompt","geo","image","indexedDB","install","microphone","persistent-sto" which I suspect of being persistent-storage and that of being related to dom.storageManager.enabled, which is false on beta and release so you either have to set it to the value you expect it to have, or adjust your expectations based on the value it has.

[Tracking Requested - why for this release]: Merge bustage, closed tree, delayed b1.
(Reporter)

Updated

a year ago
Flags: needinfo?(fliu)
(Assignee)

Updated

a year ago
Assignee: nobody → fliu
Flags: needinfo?(fliu)
Priority: -- → P1
(Assignee)

Comment 1

a year ago
This bug is because the persistent-permission is only enabled on Nightly. The test would fail on build beyond Nightly because of the persistent-permission disabled. The solution would be dynamically testing the persistent-permission according to the browser.storageManager.enabled pref state.
Tracking 55+ for this permaorange.
tracking-firefox55: ? → +
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
(In reply to Fischer [:Fischer] from comment #3)
> Created attachment 8863154 [details]
> Bug 1358384 - Test the persistent-storage permission based on the
> pref-on/off state,
> 
> The persistent-storage permission is still only pref-on on Nightly so this
> patch would test it only when it is pref-on.
> 
> Review commit: https://reviewboard.mozilla.org/r/134982/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/134982/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9734bf28c507da985b97468a9d1465f332b5579f
TRY with dom.storageManager.enabled=false to simulate the Beta environment: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9f73457a68297a85e40c8c5fad91c8349bf63a0
Attachment #8863154 - Flags: review?(florian) → review?(jhofmann)

Comment 5

a year ago
mozreview-review
Comment on attachment 8863154 [details]
Bug 1358384 - Test the persistent-storage permission based on the pref-on/off state,

https://reviewboard.mozilla.org/r/134982/#review138762

This approach makes sense to me. Alternatively we could have set the pref to true in the beginning of the test but I don't see any big advantage to that.

r=me with nits fixed

::: browser/modules/test/unit/test_SitePermissions.js:9
(Diff revision 1)
>  "use strict";
>  
>  Components.utils.import("resource:///modules/SitePermissions.jsm");
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  
> +const storageManagerEnabled = Services.prefs.getBoolPref("browser.storageManager.enabled");

Nit: We normally screaming snake case consts (STORAGE_MANAGER_ENABLED)

::: browser/modules/test/unit/test_SitePermissions.js:13
(Diff revision 1)
>  
> +const storageManagerEnabled = Services.prefs.getBoolPref("browser.storageManager.enabled");
> +
>  add_task(function* testPermissionsListing() {
> -  Assert.deepEqual(SitePermissions.listPermissions().sort(),
> -    ["camera", "cookie", "desktop-notification", "focus-tab-by-prompt", "geo", "image",
> +  let expectedPermissions = ["camera", "cookie", "desktop-notification", "focus-tab-by-prompt",
> +     "geo", "image", "indexedDB", "install", "microphone", "popup", "screen"]

nit: missing a semicolon

(why doesn't ESLint catch this?)
Attachment #8863154 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
mozreview-review-reply
Comment on attachment 8863154 [details]
Bug 1358384 - Test the persistent-storage permission based on the pref-on/off state,

https://reviewboard.mozilla.org/r/134982/#review138762

> Nit: We normally screaming snake case consts (STORAGE_MANAGER_ENABLED)

Thanks updates.

> nit: missing a semicolon
> 
> (why doesn't ESLint catch this?)

Thanks updates.
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 8

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8ae98a3bc970
Test the persistent-storage permission based on the pref-on/off state, r=johannh
Keywords: checkin-needed

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8ae98a3bc970
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.