Closed
Bug 993326
Opened 10 years ago
Closed 10 years ago
Some apps tests don't cleanup after themselves
Categories
(Core Graveyard :: DOM: Apps, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla31
People
(Reporter: marco, Assigned: marco)
References
Details
Attachments
(3 files, 4 obsolete files)
749 bytes,
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
23.78 KB,
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
21.85 KB,
patch
|
marco
:
review+
|
Details | Diff | Splinter Review |
In particular: - some tests don't restore the DOMApplicationRegistry::allAppsLaunchable value; - one test sets the "dom.mozApps.auto_confirm_install" pref to false in its cleanup function, but it doesn't need to do so because it uses SpecialPowers.autoConfirmAppInstall; - some tests don't correctly cleanup the "browser.mozApps.installer.dry_run" pref (indeed this caused problems with another test I've written in the past)
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #8403160 -
Flags: review?(fabrice)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8403161 -
Flags: review?(fabrice)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8403174 -
Flags: review?(fabrice)
Comment 4•10 years ago
|
||
Comment on attachment 8403160 [details] [diff] [review] Restore launchable value Review of attachment 8403160 [details] [diff] [review]: ----------------------------------------------------------------- Instead, we should turn SpecialPowers.setAllAppsLaunchable() into something like SpecialPowers.pushPrefEnv() and automatically revert the state at the end of test runs.
Attachment #8403160 -
Flags: review?(fabrice) → review-
Updated•10 years ago
|
Attachment #8403161 -
Flags: review?(fabrice) → review+
Comment 5•10 years ago
|
||
Comment on attachment 8403174 [details] [diff] [review] Correctly cleanup the dry_run pref Review of attachment 8403174 [details] [diff] [review]: ----------------------------------------------------------------- In these too, use pushPrefsEnv() instead.
Attachment #8403174 -
Flags: review?(fabrice) → review-
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8403160 -
Attachment is obsolete: true
Attachment #8404911 -
Flags: review?(fabrice)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8403174 -
Attachment is obsolete: true
Attachment #8404920 -
Flags: review?(fabrice)
Comment 8•10 years ago
|
||
Comment on attachment 8404911 [details] [diff] [review] Automatically restore apps launchable value Review of attachment 8404911 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed if needed. Also, push to try before landing ::: testing/specialpowers/content/specialpowersAPI.js @@ +1036,5 @@ > launchable: launchable > }; > + > + let launchable = this._sendSyncMessage("SPWebAppService", message); > + if (!this._originalAllAppsLaunchable) { I think you want (this._originalAllAppsLaunchable !== undefined) here.
Attachment #8404911 -
Flags: review?(fabrice) → review+
Updated•10 years ago
|
Attachment #8404920 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 9•10 years ago
|
||
I've removed the _originalAllAppsLaunchable variable because in the end we always want to restore the property to false. It was also causing problems (because it was correctly set in setAllAppsLaunchable, but then it was undefined in flushAllAppsLaunchable).
Attachment #8404911 -
Attachment is obsolete: true
Attachment #8405915 -
Flags: review?(fabrice)
Assignee | ||
Comment 10•10 years ago
|
||
I have modified this patch a little bit to avoid modifying the indentation too much and used SpecialPowers.autoConfirmAppInstall instead of the internal function that was fiddling with the browser UI. Using autoConfirmAppInstall, we don't need to set the dry_run pref anymore in these tests, so I've updated the commit message too. I think I don't need another review here because the patch is basically the same. Try run with all the patches applied is green.
Attachment #8404920 -
Attachment is obsolete: true
Attachment #8405916 -
Flags: review+
Assignee | ||
Comment 11•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a9940b233673
Updated•10 years ago
|
Attachment #8405915 -
Flags: review?(fabrice) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fc7f692314b https://hg.mozilla.org/integration/mozilla-inbound/rev/0918894c1c03 https://hg.mozilla.org/integration/mozilla-inbound/rev/2bea886b9aac
Flags: in-testsuite+
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2fc7f692314b https://hg.mozilla.org/mozilla-central/rev/0918894c1c03 https://hg.mozilla.org/mozilla-central/rev/2bea886b9aac
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•