874344 - Update testNewTab.js to handle the Australis build properly

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Daniela Petrovici]

This failure reproduces only on Windows:
http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c2f27d00
It is due to a lookup expression and fails here http://hg.mozilla.org/qa/mozmill-tests/file/05730582186d/lib/tabs.js#l284

Comment 1

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

A bit more information about this failure would be helpful. Which lookup string is failing here?

Comment 2

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> A bit more information about this failure would be helpful. Which lookup
> string is failing here?

This is not related to the lookup expression, but to the fact that on Windows XP double clicking on the tab strip is not opening a new tab in Australis as it used to be in all the other versions of Firefox. Double clicking on the tab strip is maximizing the window on XP too now.

The test can now be run on MAC only where the behavior to open a new tab by double clicking on the tab strip remained.

The following should be removed from the line http://hg.mozilla.org/qa/mozmill-tests/file/05730582186d/tests/functional/testTabbedBrowsing/testNewTab.js#l58
'(mozmill.isWindows && (version < "6.0")'

But I think we should confirm if this behavior is intentional first. If it is, then the test could be run on MAC only.

Comment 3

[mario garbi]

This test is skipped and depends on Bug 879752, tagging it as such.

Comment 4

[Balazs Juhasz (:balazs)]

I did some manual-test, here are my observations:

Windows XP:
- if the FF window is not maximized, the tab strip presents, on double-clicking opens a new tab
- if the FF window is maximized, the tabs "joining the title bar", double-clicking restores the window (as mentioned Daniela) - this is normal behavior of the title-bar
- if the "Menu bar" is also turned on, the tabs not "joining the title bar", double-clicking opens a new tab (no matter if FF window is maximized or not)

Windows 7: (a little more complicated)
- in Aero/Basic, with "Menu bar" turned off, the tabs "joining the title bar", double-clicking  restores/maximizing the window
- in Aero/Basic, with "Menu bar" turned on, the tabs not "joining the title bar", double-clicking doesn't do anything
- if "Use visual styles on windows and buttons" turned off in Windows 7 (getting simple grey menu), the behavior is same as described for Windows XP

Linux: (Debian 7 with Xfce)
- double-clicking on tab strip opens a new tab both if the window is maximized/restored and with Menu bar on/off
- since the test is disabled for Linux, maybe in other desktop manager the behavior is different

Comment 5

[Balazs Juhasz (:balazs)]

Sorry, I made the previous test with normal version of Nightly.
I repeated the situations with Nightly UX which contains Australis, and the behavior is what Daniela described in comment 2.

However, in Debian I still got the normal tab stirp and new-tab opening with double-click.

Comment 6

[mario garbi]

One issue with this test on Australis is the we try to get TabsOnTop propriety that has been removed. This can be verified manually by checking the preference in about:config "browser.tabs.onTop".

http://mozmill-crowd.blargon7.com/#/functional/report/26d5854914a68aa13b333f28150e0154

I'm looking into a way to fix this.

Comment 7

[mario garbi]

Attachment: australisNewTab_v2.patch

Added a patch that handles the issues presented in comment 6.

Linux report:
http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0a6b51c
http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0a716cf

Comment 8

[Andrei Eftimie]

Comment on attachment 805224 [details] [diff] [review]
australisNewTab_v2.patch

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

I don't like this approach. The test gets confusing to read, and unnecessarily complicated.
We took the approach of having code that runs on both the regular UI and Australis because we needed a heavy refactoring of NodeCollector to make it work cleanly.

In this case I feel we can just strip the non-existing functionality.
Prepare the patch to run against Australis, we will land this when Australis lands.

Comment 9

[mario garbi]

Attachment: australisNewTab_v4.patch

This strips the parts of the test that were handling a functionality that has been removed in Australis and the patch should only be landed when Australis get merged into Nightly.

Comment 10

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

(In reply to Andrei Eftimie from comment #8)
> In this case I feel we can just strip the non-existing functionality.
> Prepare the patch to run against Australis, we will land this when Australis
> lands.

So how are we going to allow people to run tests against the Australis builds? We have to ensure that all is working fine BEFORE the code lands on mozilla-central.

Comment 11

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> (In reply to Andrei Eftimie from comment #8)
> > In this case I feel we can just strip the non-existing functionality.
> > Prepare the patch to run against Australis, we will land this when Australis
> > lands.
> 
> So how are we going to allow people to run tests against the Australis
> builds? We have to ensure that all is working fine BEFORE the code lands on
> mozilla-central.

In this case we will need to handle that. 
We could also make a separate branch for Australis, but that might be lot of work to maintain.

Ok then, lets go back to using .isAustralis() to handle the UX branch.

Comment 12

[Andrei Eftimie]

Comment on attachment 805224 [details] [diff] [review]
australisNewTab_v2.patch

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

Ok, lets get this a proper review, and get this test working on both regular and UX builds.

::: tests/functional/testTabbedBrowsing/testNewTab.js
@@ +18,5 @@
>  const PREF_NEWTAB_URL = "browser.newtab.url";
>  
>  function setupModule(aModule) {
>    aModule.controller = mozmill.getBrowserController();
> +  aModule.isAustralis = utils.australis.isAustralis();

Lets make this a constant, since this can't change.
I think we can also negate this in the constant declaration itself, so we could call it NOT_AUSTRALIS, the subsequent if statements might be easier to read.

Please also add a comment referencing this bug, and a note to remove outdated functionality once Australis gets merged.

@@ +45,5 @@
>    // First, perform all tests with tabs on bottom
> +  if (!isAustralis) {
> +    tabBrowser.hasTabsOnTop = false;
> +    checkOpenTab("tabStrip");
> +  }

Include the whole block here including the comment and the next preference set and preserve the order of the calls.
We do not want to run "tabs on bottom" on Australis, we want to run the "tabs on top" tests.

@@ +56,5 @@
> +    tabBrowser.hasTabsOnTop = true;
> +    checkOpenTab("menu");
> +    checkOpenTab("shortcut");
> +    checkOpenTab("newTabButton");
> +  }

And let these calls be executed.

Comment 13

[mario garbi]

Attachment: australisNewTab_v3.patch

Updated as requested, if this looks ok I will provide full reports.

http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219a4a473

Comment 14

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 810586 [details] [diff] [review]
australisNewTab_v3.patch

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

::: tests/functional/testTabbedBrowsing/testNewTab.js
@@ +17,5 @@
>  
>  const PREF_NEWTAB_URL = "browser.newtab.url";
> +// Bug 874344 - We need to handle Australis builds as exceptions and we should
> +//              remove unaplicable code after Australis lands as Nightly
> +const NOT_AUSTRALIS = !utils.australis.isAustralis();

This should have the TODO format.

// Bug no.
// Remove after ..

Comment 15

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

Comment on attachment 810586 [details] [diff] [review]
australisNewTab_v3.patch

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

::: tests/functional/testTabbedBrowsing/testNewTab.js
@@ +17,5 @@
>  
>  const PREF_NEWTAB_URL = "browser.newtab.url";
> +// Bug 874344 - We need to handle Australis builds as exceptions and we should
> +//              remove unaplicable code after Australis lands as Nightly
> +const NOT_AUSTRALIS = !utils.australis.isAustralis();

Also I would see an empty line to separate that line, and I wouldn't negate the constant. Better would be IS_AUSTRALIS_BUILD IMO.

Comment 16

[mario garbi]

Attachment: patch_v4

Changed the constant name as suggested and updated the comment with the TODO structure.

Comment 17

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 811111 [details] [diff] [review]
patch_v4

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

::: tests/functional/testTabbedBrowsing/testNewTab.js
@@ +18,5 @@
>  const PREF_NEWTAB_URL = "browser.newtab.url";
>  
> +// Bug 874344
> +// TODO: We need to handle Australis builds as exceptions and we should
> +//       remove unaplicable code after Australis lands as Nightly

No need for TODO, I added an example in the previous review. Please also see bug 872918 where we refactored this.

Comment 18

[mario garbi]

Attachment: patch_v5

I've copied the TODO structure entirely before. Now I've understood how it should have been done and updated the code to fit the current style.

Comment 19

[Andrei Eftimie]

Comment on attachment 812523 [details] [diff] [review]
patch_v5

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

Looks good, just one small nit, please make the change and we'll get this landed.

::: tests/functional/testTabbedBrowsing/testNewTab.js
@@ +60,2 @@
>  
>    // Second, perform all tests with tabs on top

nit: Please move this comment in the above block as well.

When Australis lands we'll only need to remove these blocks as a whole.

Comment 20

[mario garbi]

Attachment: patch_v6

Made the requested changes and created reports for Linux:

http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750669a29
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a75066d488

Comment 21

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 812591 [details] [diff] [review]
patch_v6

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

This looks good now. I will land this when we have full reports to be sure we don't regress anything.

Comment 22

[mario garbi]

Attachment: australisNewTab_v7.patch

I had to update the code a little to make it work on Windows XP too. It works ok now on all platforms with Mozmill 2.0 as can be seen in the reports:

Mac with Nightly
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d090e5

Australis reports:
-Mac
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d0bb98
-Linux
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d0ad8c
-Windows
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d0ef65

Comment 23

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 813463 [details] [diff] [review]
australisNewTab_v7.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/f5cddebbe0c5 (default)

Comment 24

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

We should not forget to update the summary so it is more descriptive and tells what has been done on that bug.