1023292 - split up browser_popupNotification.js

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

This test is a bit long, hopefully splitting it will help with bug 969608.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1

I did some refactoring to share more stuff through the test files, through an helper in head.js and some shared functions. This is not too far from being able to use a non-custom test runner, but I didn't go that far cause there's enough changes here and I tried to keep the structure of the tests to avoid nulifying some of them. Though it will be a good starting point to remove the custom runner in future.
For now I re-enabled all the tests in Windows, cause I don't see issues locally, let's see what Try thinks.
https://tbpl.mozilla.org/?tree=Try&rev=e20fac07815f

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1 (hg cp)

same diff, but using hg cp to copy the test...

I was expecting it to be slightly better for review, but it's not that much.

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v2

Small update to fix an issue raised by previous try
https://tbpl.mozilla.org/?tree=Try&rev=fca520825bf9

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v3

This is the version that gave the most green on Try server, but it's not perfect. The intermittent failures we had before are unlikely to be fixed.
So I'm disabling the tests on debug linux that is where I saw most common failures.
I think it's worth to land this splitting and then work on the single tests separately in a follow-up bug (or in one of the already existing bugs for these random failures), I'd like to do some more Try runs with better logging to better understand order of calls since sounds like most of the issues are due to popup events coming at unexpected times.

But I think this can land regardless and that will also help us figuring how much work we still have to do on intermittents.

The complete review may take quite some time, so I suggest to concentrate on the head file changes, new structure of a single test and the changes I made to about home to try reducing likely of some intermittents I saw on Try.
As I said the new structure is made to make it easier in future to move to a more common shared harness (like add_task), but I didn't go that far cause it required even deeper changes and may hide the intermittents we should instead figure and fix.

Gavin, feel free to forward the request.

Comment 5

[Marco Bonardo [:mak]]

PS: I'm going to do a further try run of this, to check status against current nightly.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8443284 [details] [diff] [review]
patch v3

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

::: browser/base/content/test/general/browser.ini
@@ +339,5 @@
> +skip-if = (os == "linux" && debug) || e10s # e10s - Bug ?????? - popup notification test probably confused re content process notifications etc
> +[browser_popupNotification-2.js]
> +skip-if = (os == "linux" && debug) || e10s - Bug ?????? - popup notification test probably confused re content process notifications etc
> +[browser_popupNotification-3.js]
> +skip-if = (os == "linux" && debug) || e10s - Bug ?????? - popup notification test probably confused re content process notifications etc

Isn't that missing a "#" to denote the comment?

Comment 7

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [:mak] from comment #4)
> I'd like to do some more Try runs with better
> logging to better understand order of calls since sounds like most of the
> issues are due to popup events coming at unexpected times.

See also bug 1027812.

Comment 8

[Marco Bonardo [:mak]]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> Isn't that missing a "#" to denote the comment?

oops, I edited it too fast

Comment 9

[Marco Bonardo [:mak]]

with fixed browser.ini

https://tbpl.mozilla.org/?tree=Try&rev=4cb6263cdf5d

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8443284 [details] [diff] [review]
patch v3

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

I would prefer the -N.js suffix used an underscore instead of a hyphen since that seems to be more common.

TBH I'm not really convinced that splitting this test will help with the intermittent failures.

::: browser/base/content/test/general/browser_popupNotification-3.js
@@ -660,3 @@
>      },
> -    onHidden: [
> -      function (popup) {

Was it intentional that the check is now done on the first hide instead of the second?

@@ -701,4 @@
>      },
> -    onHidden: [
> -      function (popup) {
> -      },

ditto

@@ +310,5 @@
>                        .setAttribute("src", "http://example.org/");
>      },
>      onHidden: function () {}
>    },
> +  // Popup Notifications should catch exceptions from callbacks  

Nit: trailing whitespace

::: browser/base/content/test/general/browser_popupNotification.js
@@ +19,5 @@
>    PopupNotifications.transitionsEnabled = false;
>  
> +  registerCleanupFunction(() => {
> +    PopupNotifications.buttonDelay = PREF_SECURITY_DELAY_INITIAL;
> +    PopupNotifications.transitionsEnabled = true;

Can you move this and the const inside a shared PopupNotificationHelper cleanup helper to avoid the duplication?

@@ +31,3 @@
>  }
>  
> +function* runNextTest() {

Can you also move the runNextTest function to the helper since it seems to be identical in all 5 tests.

::: browser/base/content/test/general/head.js
@@ +500,5 @@
> +    tab.linkedBrowser.loadURI(url);
> +  return deferred.promise;
> +}
> +
> +let PopupNotificationHelper = {

It seems like it may be better to move these to a new directory so you don't need to create this helper object and change blame for each of the callers of the methods. You would put these in the new head.js at the top-level. This would also make it easier to run the tests together.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #10)
> ::: browser/base/content/test/general/browser_popupNotification-3.js
> @@ -660,3 @@
> >      },
> > -    onHidden: [
> > -      function (popup) {
> 
> Was it intentional that the check is now done on the first hide instead of
> the second?

it's still the second, I delayed the time we attach the handler since that gave better results on Try regarding intermittent failures. Moreover there is no more hack-ish arrays of empty handlers.


> @@ +31,3 @@
> >  }
> >  
> > +function* runNextTest() {
> 
> Can you also move the runNextTest function to the helper since it seems to
> be identical in all 5 tests.

I could. I didn't cause it's custom runner that I think in future should be dropped in favor of using the default runner (add_task) directly. Since it's an uncommon runner I didn't want to "share" it more than it is. But since it would be namespaced into the helper object probably I was just overzealous.

> > +let PopupNotificationHelper = {
> 
> It seems like it may be better to move these to a new directory so you don't
> need to create this helper object and change blame for each of the callers
> of the methods. You would put these in the new head.js at the top-level.
> This would also make it easier to run the tests together.

I might, though it seems a bit overkill for just 4 tests?

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8443284 [details] [diff] [review]
patch v3

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

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #10)
> TBH I'm not really convinced that splitting this test will help with the
> intermittent failures.

Me either, but I think Marco's only claiming that splitting (and refactoring) is more about laying a foundation for further debugging than it is intending to fix the failures, and that makes sense to me.

I like Matt's idea about moving the tests to a new directory with a new head file, but I also don't think it's a big deal, so either way, r+ on that part from me.  Although if you envision splitting these tests even more or adding others in the near future, a new directory would be better.

::: browser/base/content/abouthome/aboutHome.js
@@ +398,2 @@
>      };
>      xhr.onload = function (event)

You might be able to replace onload and onerror with onloadend, which "gets invoked when the operation is completed for any reason": https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIXMLHttpRequestEventTarget

::: browser/base/content/test/general/head.js
@@ +480,5 @@
> +{
> +  let deferred = Promise.defer();
> +  info("Wait tab event: " + eventType);
> +  let timeout = setTimeout(
> +    () => deferred.reject(new Error("Timed out while waiting for a '" + eventType + "'' event")),

The listener should probably be removed in this case even though this case shouldn't normally happen.

Comment 13

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #12)
> (In reply to Matthew N. [:MattN] (behind on bugmail) from comment #10)
> > TBH I'm not really convinced that splitting this test will help with the
> > intermittent failures.
> 
> Me either, but I think Marco's only claiming that splitting (and
> refactoring) is more about laying a foundation for further debugging than it
> is intending to fix the failures, and that makes sense to me.

Indeed, moreover this bug is not about fixing intermittent failures, splitting the test has a value by itself, coment 0 indeed says that "hopefully" splitting may help with the intermittent failures.

Btw, I'll move the tests to their own folder since you both like the idea.

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch v4

This should address all comments, I will re-push to try as soon as it reopens...

Comment 15

[Marco Bonardo [:mak]]

https://tbpl.mozilla.org/?tree=Try&rev=24821165c87b
https://hg.mozilla.org/integration/fx-team/rev/a86527f8f58a

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a86527f8f58a

Comment 17

[Marco Bonardo [:mak]]

as a side note, this is unlikely to have solved existing intermittents (maybe reduced, we will know with time), so existing failures of browser_popupNotifications.js will now be assigned to the specific splitted test (-1 to -4). 99% is not new failures, I hope the improvements to the tests will help figuring them out though.