1127244 - Test failure 'Notification popup state has been opened' in /testDownloading/testDownloadStates.js

Status: RESOLVED INVALID

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Daniela Domnici]

Module:   testDownloadPanel
Test:     /testDownloading/testDownloadStates.js
Error:    Notification popup state has been opened
Branch:   Nightly(38.0a1)
Platform: Linux Ubuntu

Reports: http://mozmill-daily.blargon7.com/#/remote/failure?app=Firefox&branch=All&platform=All&from=2015-01-22&to=2015-01-29&test=%2FtestDownloading%2FtestDownloadStates.js&func=testDownloadPanel

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

This reproduces 100% on production and staging nodes, it doesn't locally. The reason is the display resolution on production nodes being to small, and if the notification is not fully displayed the test fails, at least this is what I see visually when I ran the test. Anyway since it fails constantly we should disable the tests on linux.
Daniela could you do that? Also after we file new bugs we have to investigate at least if it's reproducible and if it needs to be skipped.

Comment 3

[Daniela Domnici]

Attachment: patch_V1

The bug is reproductible on production and staging machines.As Cosmin said, the problem is the resolution, if the notification is not fully displayed the test fails.

Created the skip patch.The skip patch also applies to aurora branch.

Comment 4

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8556478 [details] [diff] [review]
patch_V1

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

https://hg.mozilla.org/qa/mozmill-tests/rev/ef96e375ad8e (beta)

Comment 5

[Andreea Matei [:AndreeaMatei]]

Oh, sorry, just default is affected here actually, the downloads refactor only got to default.

https://hg.mozilla.org/qa/mozmill-tests/rev/bb5cf2b9cd8b (backout beta)
https://hg.mozilla.org/qa/mozmill-tests/rev/5c6379600c20 (default)

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Cosmin Malutan, [:cosmin-malutan] from comment #2)
> The reason is the display resolution on production nodes being to small, and
> if the notification is not fully displayed the test fails, at least this is
> what I see visually when I ran the test. Anyway since it fails constantly we
> should disable the tests on linux.

Can you please check what exactly fails here? Is it a click? Or is the panel not displayed because the window is too narrow? A resolution of 1024x768 should have been sufficient for all tests. So I'm really curious to hear more details.

Comment 7

[Teodor Druta]

Following Cosmin's information if this is failing because the firefox window is too small, we don't have any choice here but to resize the window in setupModule and clean after us in teardownModule. We shouldn't skip a test because of this.

Comment 8

[Teodor Druta]

Attachment: fixtstdownloadstates.patch

This is my proposed fix for this issue. I think that changing the resolution to a higher one to all the ubuntu nodes is not a good one, also we shouldn't skip a fully functional test because of system resolution.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8558553 [details] [diff] [review]
fixtstdownloadstates.patch

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

Lets maximize the window instead. In teardown you can revert that and you will get the original size back.

Beside all that we have way more skipped tests like the home button test, which have the same problem. I would favor to unskip all of them with the final approach on this bug.

f+ related to the idea of maximizing the window.

Comment 10

[Teodor Druta]

Attachment: fixdownloadstates.patch

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Lets maximize the window instead. In teardown you can revert that and you
> will get the original size back.
> 
> Beside all that we have way more skipped tests like the home button test,
> which have the same problem. I would favor to unskip all of them with the
> final approach on this bug.
> 
> f+ related to the idea of maximizing the window.

I have added two new methods to the BaseWindow class, because I think we could use this methods in other tests as well where we have this problem.
Also I think this should be applied to all the platforms not just linux. We still have to see this failure on other platforms, not just on ubuntu, but this will not mean that this failure will not appear in the future on windows and os x. This can be reproduced manually on all platforms by resizing the browser window.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8559064 [details] [diff] [review]
fixdownloadstates.patch

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

::: lib/ui/base-window.js
@@ +134,5 @@
> +
> +  /**
> +   * Restores the window value after it has been maximized
> +   */
> +  restore: function BaseWindow_restore() {

restoreSize maybe will describe better. maximize it's suggestive enough but here can mean that you restore the window from the history or something else but the size.

Comment 12

[Teodor Druta]

Attachment: fixdownloadstatesv2.patch

Updated the restore method name to restoreSize

Comment 13

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8559157 [details] [diff] [review]
fixdownloadstatesv2.patch

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

lgtm.

I think we can use this as an alternative fix until bug 1084289 gets fix for bug 1071515.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8559157 [details] [diff] [review]
fixdownloadstatesv2.patch

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

r=me with the name change, and the bug filed against Firefox if its a real problem.

::: firefox/tests/remote/testDownloading/testDownloadStates.js
@@ +25,5 @@
>    prefs.setPref(PREF_PANEL_SHOWN, true);
> +
> +  // Maximize the browser window because the download panel
> +  // fails to open on smaller window sizes
> +  aModule.browserWindow.maximize();

If this is a bug in Firefox please file it! The description sounds like that, and the panel should always open when the button is clicked, independent from the window size. Please add the filed bug reference to the comment.

@@ +34,5 @@
>    downloads.removeAllDownloads();
>    aModule.downloadsPanel.close({force: true});
> +
> +  // Restore the browser window size to its previous pre-maximizing values
> +  aModule.browserWindow.restoreSize();

I think the comment is not necessary here.

::: lib/ui/base-window.js
@@ +135,5 @@
> +  /**
> +   * Restores the window size after it has been maximized
> +   */
> +  restoreSize: function BaseWindow_restoreSize() {
> +    this.controller.window.restore();

I'm not happy with the method name. Lets simply reflect the original name and make it transparent to the test.

Comment 15

[Teodor Druta]

Attachment: Screenshot from 2015-02-05 13:52:15.png

(In reply to Henrik Skupin (:whimboo) from comment #14)
> If this is a bug in Firefox please file it! The description sounds like
> that, and the panel should always open when the button is clicked,
> independent from the window size. Please add the filed bug reference to the
> comment.

In this specific case is not a firefox bug, it's just that when the window width is too small the toolbar panel buttons will group themselves under a popup menu (please see attached screenshot).

Comment 16

[Teodor Druta]

Attachment: fixdownloadstatesv3.patch

(In reply to Henrik Skupin (:whimboo) from comment #14)
> ::: firefox/tests/remote/testDownloading/testDownloadStates.js
> @@ +25,5 @@
> >    prefs.setPref(PREF_PANEL_SHOWN, true);
> > +
> > +  // Maximize the browser window because the download panel
> > +  // fails to open on smaller window sizes
> > +  aModule.browserWindow.maximize();
> 
> If this is a bug in Firefox please file it! The description sounds like
> that, and the panel should always open when the button is clicked,
> independent from the window size. Please add the filed bug reference to the
> comment.

Added a more appropriate comment to reflect the real issue.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Teodor Druta from comment #15)
> In this specific case is not a firefox bug, it's just that when the window
> width is too small the toolbar panel buttons will group themselves under a
> popup menu (please see attached screenshot).

So it's indeed the same as for the home button, which is the test we also had to disable because of that. So once this patch landed and works, make sure to also unskip the other skipped tests due to this problem. Hopefully we can get the underlying Firefox bug fixed, so windows are wider by default.

Comment 18

[Andreea Matei [:AndreeaMatei]]

https://hg.mozilla.org/qa/mozmill-tests/rev/f3d36b1ec9bc (default)
Unskip:
https://hg.mozilla.org/qa/mozmill-tests/rev/3e2f65a4b9e9 (default)

This test is only affected in nightly, so we'll close. In the other bug we need this fix in other branches too.
Please add the unskip in the fix patch from now on.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the added library methods are not complete and we want to get this backed out. Both maximze and restore would have to wait until the window reached its target size. But they don't and that's why lots of tests are failing. 

Teodor, not sure if you will be able to get this fixed today. If yes its wonderful, otherwise lets back all out and skip this test. Thanks.

Comment 20

[Teodor Druta]

Attachment: waitforresize.patch

(In reply to Henrik Skupin (:whimboo) from comment #19)
> So the added library methods are not complete and we want to get this backed
> out. Both maximze and restore would have to wait until the window reached
> its target size. But they don't and that's why lots of tests are failing. 
> 
> Teodor, not sure if you will be able to get this fixed today. If yes its
> wonderful, otherwise lets back all out and skip this test. Thanks.

This has more to do with the toolbar buttons transition from accessible dropdown tree -> to visibile toolbar buttons with icons and the window resize on slower machines. That's why I didn't catch this initially it's because I tested on my local machines which are way faster than the production ones.

This is a follow-up patch for this, which will handle all the failures due to the transition to maximize of the window, tested with the affected tests both locally and on a production node.

Comment 21

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8564063 [details] [diff] [review]
waitforresize.patch

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

Just a nit, please ask from Henrik too.

::: lib/ui/base-window.js
@@ +167,5 @@
> +                 "Callback has been specified");
> +
> +    var resized = false;
> +
> +    function onResize() { resized = true; }

Please remove the line above

Comment 22

[Teodor Druta]

Attachment: waitforresize.patch

Fixed.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8564071 [details] [diff] [review]
waitforresize.patch

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

::: lib/ui/base-window.js
@@ +131,5 @@
>    maximize: function BaseWindow_maximize() {
> +    this.waitForResize(() => {
> +      // Handle the accessible tree nav bar button
> +      var navBarBtn = findElement.ID(this.controller.window.document, "nav-bar-overflow-button");
> +      var btnDisplay = utils.isDisplayed(this.controller, navBarBtn);

BaseWindow has totally no idea about any browser window element. So this cannot be done this way.

@@ +143,5 @@
> +          var currDisplay = utils.isDisplayed(this.controller, navBarBtn);
> +          return !currDisplay;
> +        }, "Nav bar accessible tree nav bar button is not displayed");
> +      }
> +    });

Personally I don't feel that the above is very stable due to possible screen dimensions. And I'm a bit vague if we should get something like that landed.

Btw. any checks for elements might have to come from the test and not from any library. Only the test knows which element it is looking for.

@@ +177,5 @@
> +                     "Window has been resized");
> +    }
> +    finally {
> +      this.controller.window.removeEventListener("resize", onResize);
> +    }

This looks like a fine method for our BaseClass.

Comment 24

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #23)
> Personally I don't feel that the above is very stable due to possible screen
> dimensions. And I'm a bit vague if we should get something like that landed.
> 

I checked this thoroughly, In my tests this acted stable.

> Btw. any checks for elements might have to come from the test and not from
> any library. Only the test knows which element it is looking for.

What if we will add this check to the BrowserWindow class?

> @@ +177,5 @@
> > +                     "Window has been resized");
> > +    }
> > +    finally {
> > +      this.controller.window.removeEventListener("resize", onResize);
> > +    }
> 
> This looks like a fine method for our BaseClass.

Unfortunately, this will not fully fix the problem, if we really don't want the above navbar-overflow-button check, then we should update affected tests, with a waitThenClick() instead of click() everywhere it applies.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Teodor Druta from comment #24)
> (In reply to Henrik Skupin (:whimboo) from comment #23)
> > Personally I don't feel that the above is very stable due to possible screen
> > dimensions. And I'm a bit vague if we should get something like that landed.
> 
> I checked this thoroughly, In my tests this acted stable.

You do not know how test machines behave. This can perfectly work at your machine but there are hundreds of other factors around which could cause this to fail.

> > Btw. any checks for elements might have to come from the test and not from
> > any library. Only the test knows which element it is looking for.
> 
> What if we will add this check to the BrowserWindow class?

Also the browser window class does not know which element to check for. It's totally in responsibility of the test.

> > This looks like a fine method for our BaseClass.
> 
> Unfortunately, this will not fully fix the problem, if we really don't want

I haven't said that this should be the only change. It's the only one which we can do for the BaseWindow class.

> the above navbar-overflow-button check, then we should update affected
> tests, with a waitThenClick() instead of click() everywhere it applies.

This wont help because waitThenClick() waits for the element to exist, but not to be displayed. What you may want is utils.isDisplayed().

Comment 26

[Teodor Druta]

Unfortunately I won't be able to come with a better solution than the previous one for this issue.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozmill tests have been superseded by Marionette tests.