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)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(3 files, 4 obsolete files)

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)
Attached patch Restore launchable value (obsolete) — Splinter Review
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #8403160 - Flags: review?(fabrice)
Attachment #8403174 - Flags: review?(fabrice)
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-
Attachment #8403161 - Flags: review?(fabrice) → review+
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-
Attachment #8403160 - Attachment is obsolete: true
Attachment #8404911 - Flags: review?(fabrice)
Attachment #8403174 - Attachment is obsolete: true
Attachment #8404920 - Flags: review?(fabrice)
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+
Attachment #8404920 - Flags: review?(fabrice) → review+
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)
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+
Attachment #8405915 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
Blocks: 999653
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.