1038070 - Test failure 'Link 1 title is visible for the tab' in testTabbedBrowsing/testBackgroundTabScrolling.js

Status: RESOLVED FIXED

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

Description

[Daniel Gherasim]

Module:    functional
Test:      testTabbedBrowsing/testBackgroundTabScrolling.js
Function:  testScrollBackgroundTabIntoView
Failure:   "Link 1 title is visible for the tab"
Branches:  NIGHTLY
Platforms: ALL

http://mozmill-daily.blargon7.com/#/functional/report/233c96fc2ff8f1c1a44659ece20d04d4

Not sure if the first failure in report is related, running a testrun now to check.

I'll provide a pushlog & skip patch right away.

Comment 1

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

Again, I miss the pushlog link between the builds when it has been started. Please always tell us about that when you file a P1 regression bug. It's easy to retrieve from the reports list.

Comment 2

[Daniel Gherasim]

I said in Comment 1 that I'll provide the pushlog right away. 
Here it is:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cb75d6cfb004&tochange=e1a037c085d1

I see bug 1014313 as a candidate here.
Skip patch creating now.

Comment 3

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

We want to update the test, so it starts with index 4 given that a new entry has been injected:
http://hg.mozilla.org/qa/mozmill-tests/file/9b85f3c5db29c82279f6c1fb26a547a92794886f/firefox/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js#l95

Comment 4

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

Or even better find the appropriate entries with nodecollector, so we do not fail again in the future if entries are getting changed.

Comment 5

[Daniel Gherasim]

Attachment: skip.patch

Here is a skip patch.
We'll get into fixing this as you asked.

Comment 6

[Andrei Eftimie]

Comment on attachment 8455208 [details] [diff] [review]
skip.patch

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

Disabled:
https://hg.mozilla.org/qa/mozmill-tests/rev/47ebec26edd4 (default)

(I updated the commit message to also include the bug number)

Comment 7

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

Well, I would have seen more value in increasing the initial index on this bug, to fix the problem and make use of nodecollector in a follow-up. Skipping tests is really the last resort, especially if the fix is so obvious. Please try to avoid that in the future.

Comment 8

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) [away 07/19 - 08/01] from comment #7)
> Well, I would have seen more value in increasing the initial index on this
> bug, to fix the problem and make use of nodecollector in a follow-up.
> Skipping tests is really the last resort, especially if the fix is so
> obvious. Please try to avoid that in the future.

We've done this in the past and it has bit us back more than once if the initial seemingly "obvious" fix didn't work as expected. There was always pressure from you to skip the test ASAP even if we had a fix in the works as to not have failures once the daily testruns start.

The statement that "Skipping tests is really the last resort" is conflicting with previous statements that we should have the testruns "clean" of failures (by disabling tests until they are fixed).

If this sentiment has changed we should really talk about it on the mailing list or in a  meeting, not in a comment on a P1 regression that needs to be disabled ASAP.

Comment 9

[Daniel Gherasim]

I will work on this.

Comment 10

[Daniel Gherasim]

Attachment: v1.patch

Changed to use nodeCollector.

Comment 11

[Daniel Gherasim]

Attachment: v1.1.patch

Comment 12

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8460094 [details] [diff] [review]
v1.1.patch

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

Just a small change needed.

::: firefox/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
@@ +95,3 @@
>  
> +  // Check that the correct title is shown for all tabs
> +  for (var tab in nodeCollector.elements) {

Please use forEach here with aTab.

Comment 13

[Daniel Gherasim]

Attachment: v2.patch

Comment 14

[Andrei Eftimie]

Comment on attachment 8460219 [details] [diff] [review]
v2.patch

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

I'd like to improve the checks a bit for readability.

::: firefox/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
@@ +98,5 @@
> +    var node = aTab.getNode();
> +    expect.waitFor(() => {
> +      return (aIndex === 0 && node.label === "Open link in a new tab") ||
> +             (aIndex === countTabs - 1  && node.label === "2") ||
> +             (node.label === "1");

This is a bit hard to follow.
Can we do a switch or some if clauses and do specific checks for the first element, the last element and defaults to the rest of the elements.

Comment 15

[Daniel Gherasim]

Attachment: v2.1.patch

Changed to make the same checks as before in the test.

Comment 16

[Daniel Gherasim]

Attachment: v2.2.patch

Comment 17

[Andrei Eftimie]

Comment on attachment 8461582 [details] [diff] [review]
v2.2.patch

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

::: firefox/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
@@ +90,5 @@
>    }, "The all tabs popup should have been opened");
>  
> +  // Select all opened tabs
> +  var nodeCollector = new domUtils.nodeCollector(allTabsPopup.getNode());
> +  nodeCollector.queryNodes(".alltabs-item :not(:first-child)");

I don't think this works as you'd expect.
Given that there is a space in the selector, you actually get all non-first-child descendants of .alltabs-item

Comment 18

[Daniel Gherasim]

Attachment: v2.3.patch

Fixed that.

Comment 19

[Andrei Eftimie]

Comment on attachment 8464001 [details] [diff] [review]
v2.3.patch

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

::: firefox/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
@@ +97,4 @@
>  
> +  // Check that the correct title is shown for all tabs
> +  nodeCollector.nodes.forEach((aTab, aIndex) => {
> +    if (aIndex !== countTabs - 1) {

I'm not fond of this `if` clause for every step when we can check this item before the loop and loop only over the rest of the items.

Please check the last element before the loop. Do a pop on the array then you only have the second check in the loop.

Comment 20

[Daniel Gherasim]

Attachment: v2.4.patch

Comment 21

[Andrei Eftimie]

Comment on attachment 8465441 [details] [diff] [review]
v2.4.patch

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

Just 2 small nits and we can land this.

::: firefox/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
@@ +98,3 @@
>  
> +  // Check that the correct title is shown for the last tab
> +  expect.equal(nodeCollector.nodes[nodeCollector.nodes.length - 1].label, "2",

For readability please leave the length in a separate variable as you've had it before.

@@ +102,5 @@
> +
> +  nodeCollector.nodes.pop();
> +
> +  // Check that the correct title is shown for rest of the tabs
> +  nodeCollector.nodes.forEach((aTab, aIndex) => {

We don't need the index anymore.

Comment 22

[Daniel Gherasim]

Attachment: v2.5.patch

Comment 23

[Andrei Eftimie]

Comment on attachment 8465463 [details] [diff] [review]
v2.5.patch

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

r+

https://hg.mozilla.org/qa/mozmill-tests/rev/abeb8d51df89 (default)

Comment 24

[Andrei Eftimie]

We'll also need to backport this to Aurora.

Comment 25

[Daniel Gherasim]

This can be backported to aurora safely.

http://mozmill-crowd.blargon7.com/#/functional/report/533154d2e18fa05bbdf2eaada8c4a552
http://mozmill-crowd.blargon7.com/#/functional/report/533154d2e18fa05bbdf2eaada8c49db9

Comment 26

[Andreea Matei [:AndreeaMatei]]

Transplanted: http://hg.mozilla.org/qa/mozmill-tests/rev/f15a836eb1ef (aurora)