Add back/port the tests that got removed from bug 1140495

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

Trunk
Firefox 44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43+ fixed, firefox44 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Some of the tests that got removed in bug 1140495 should still be kept around as copies of them did not exist under the in-content/ directory, and they are not specific to the windowed preferences.

See https://hg.mozilla.org/mozilla-central/rev/189cf9021f8b for the files that got removed.
[Tracking Requested - why for this release]: The tests were removed in Firefox43 while on m-c (and uplifted to Aurora42). We should get these added back soon before a regression gets created and unnoticed.
jaws, are you looking for someone else to do this or are you intending to take it on?
Flags: needinfo?(jaws)
Yeah, I'm going to take this.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
Posted patch WIP Patch (obsolete) — Splinter Review
This adds back the tests that shouldn't have been removed.

All tests pass except for browser_cookies_exceptions.js, which as a weird failure at line 66, where params.tree.view is undefined for some reason that I can't pinpoint. Any ideas, Matt?
Attachment #8659898 - Flags: feedback?(MattN+bmo)
Comment on attachment 8659898 [details] [diff] [review]
WIP Patch

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

::: browser/components/preferences/permissions.js
@@ -158,5 @@
>  
>      if (!isNewPermission) {
>        this._permissionsToDelete.set(aPermission.principal.origin, aPermission);
>      }
> -

this change can be removed.
Posted patch Patch (obsolete) — Splinter Review
Gijs, do you have any idea why the browser_cookies_exception.js test is failing at the line annotated with "Test failing with params.tree.view is undefined, see bug XXX"

The original test doesn't fail here.

Try push, https://treeherder.mozilla.org/#/jobs?repo=try&revision=09e8e1eb849e
Attachment #8659898 - Attachment is obsolete: true
Attachment #8659898 - Flags: feedback?(MattN+bmo)
Attachment #8665146 - Flags: review?(gijskruitbosch+bugs)

Comment 7

4 years ago
Comment on attachment 8665146 [details] [diff] [review]
Patch

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

Discussed on vidyo - failures because of the permission manager observer that nukes the tab before the test finishes.
Attachment #8665146 - Flags: review?(gijskruitbosch+bugs)
Posted patch Patch v2Splinter Review
Thanks for your help in debugging the test failure over Vidyo.

The permission observer was fired while the test was running, and the permission observer was set up to close the preferences once it had been hit. Since test case #5 adds a permission during the test run using the permission manager, the permission observer is ran before the test has completed. This caused the tab to close and thus the tree.view getting destroyed.

In the old version of the prefs, the dialog's window.close() was either being done async and/or the references to the dialog within the test kept the window alive long enough to continue the test. However, the test was still written as a no-op since the permission observer was supposed to call cleanUp which would remove the newly added permission causing the rows to be empty at the following check anyways. But... cleanUp() wasn't called, because the permObserver had a typo that was checking for `cleanup`, not `cleanUp`. This is all fixed now.
Attachment #8665146 - Attachment is obsolete: true
Attachment #8665416 - Flags: review?(gijskruitbosch+bugs)

Comment 9

4 years ago
Comment on attachment 8665416 [details] [diff] [review]
Patch v2

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

::: browser/components/preferences/in-content/tests/browser_cookies_exceptions.js
@@ +198,5 @@
> +      let helperFunctions = {
> +        windowLoad: function(win) {
> +          let doc = win.document;
> +          let params = {
> +            doc: doc,

nit: doc,

(don't need to repeat a local variable name if the property has the same name)
Attachment #8665416 - Flags: review?(gijskruitbosch+bugs) → review+
The previous landing of the patch for this bug had two issues with the browser.ini changes. In both instances, the 'skip-if' line was not kept adjacent to their associated test, causing browser_change_app_handler.js to run on non-Windows platforms (as well as browser_healthreport.js).
https://hg.mozilla.org/mozilla-central/rev/3db46f5d82cc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Comment on attachment 8665416 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 1140495
[User impact if declined]: no impact, only additional automated tests
[Describe test coverage new/current, TreeHerder]: automated tests being run on mozilla-central
[Risks and why]: none expected
[String/UUID change made/needed]: none
Attachment #8665416 - Flags: approval-mozilla-aurora?
Comment on attachment 8665416 [details] [diff] [review]
Patch v2

Test-only change; ok to uplift to aurora.
Attachment #8665416 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.