992187 - Test failure "addButton is undefined" in restartTests/testAddons_InstallAddonWithoutEULA/test1.js

Status: RESOLVED FIXED

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

Description

[Andreea Matei [:AndreeaMatei]]

Failed several times with Aurora, en-US on Windows 8 and Windos 7, so far.

Affected line:
http://hg.mozilla.org/qa/mozmill-tests/file/mozilla-aurora/firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test1.js#l69

Comment 1

[Andreea Matei [:AndreeaMatei]]

We've seen this with Aurora ru and de locales, on Linux and Windows.
http://mozmill-daily.blargon7.com/#/remote/report/91f18c799f60f33fc05d03749c4ba40e
http://mozmill-daily.blargon7.com/#/remote/report/91f18c799f60f33fc05d03749c21f2ba

Comment 2

[Andrei Eftimie]

Still failing. Current rate ~8 failures in the last month.
http://mozmill-daily.blargon7.com/#/remote/report/0bd22723d1fdbb446269f765b91b4cac

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

Filed 4 times on mac nodes with Aurora, in the same timeframe (01:06-01:09PM), tests are remote so it might worth checking how the elements is loading.

Comment 4

[Daniel Gherasim]

Failed again 5 times on Aurora.
Some lines have been changed in the tests along with bug 994040, so now the error is
'addButton is undefined'.

Most probably the line that fails is :
http://hg.mozilla.org/qa/mozmill-tests/file/f172cbe94bdf/firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test1.js#l71

Looking more into this, we may find a easy fix here.
This is what's happening in the test:
* We are accessing https://addons.mozilla.org/en-US/firefox/addon/nightly-tester-tools/ and wait for the page to load;
* Then we click the "+ Add to Firefox" button to add the addon;
* We wait for the notification to appear;

One behavior we missed in the test is that the button changes it's state after <1 s from 'Download' to '+ Add to Firefox', so with a slow connection, we might catch the first state & not trigger the propper event. 

We may want to wait for the button to be in the propper state & then click on it.

Maybe this is why it's failling.

Comment 5

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

(In reply to daniel.gherasim from comment #4)
> One behavior we missed in the test is that the button changes it's state
> after <1 s from 'Download' to '+ Add to Firefox', so with a slow connection,
> we might catch the first state & not trigger the propper event. 

This may even open a modal system-level file save dialog, which would block the testrun. Given that we haven't seen this yet, I don't think at least our system is affected here. But yes, we should fix this.

Comment 6

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #5)
> (In reply to daniel.gherasim from comment #4)
> > One behavior we missed in the test is that the button changes it's state
> > after <1 s from 'Download' to '+ Add to Firefox', so with a slow connection,
> > we might catch the first state & not trigger the propper event. 
> 
> This may even open a modal system-level file save dialog, which would block
> the testrun. Given that we haven't seen this yet, I don't think at least our
> system is affected here. But yes, we should fix this.

In firefox it doesn't do that as I can see. I tested this by slowing down my internet connection so I can click on 'Download Now', this actually has the same behavior as for '+ Add to firefox'.

Comment 7

[Daniel Gherasim]

8 failures today, all with Aurora on Mac OSX's.
We should get this investigated now, maybe skip it until we find a fix.

Comment 8

[Daniel Gherasim]

Attachment: skip.patch

Affecting our testruns a lot,
http://mozmill-release.blargon7.com/#/remote/failure?app=All&branch=All&platform=All&from=2014-04-04&to=&test=%2FrestartTests%2FtestAddons_InstallAddonWithoutEULA%2Ftest1.js&func=testInstallAddonWithEULA

Let's skip it & find a fix here.

Comment 9

[Andrei Eftimie]

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

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

Disabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/13366e4a9202 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/91eef9085e45 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/7dc8bfc3d801 (mozilla-beta)

Comment 10

[Andrei Eftimie]

Patch doesn't apply clean on the `mozilla-release` branch. Please provide a backport.

Comment 11

[Daniel Gherasim]

Attachment: skip_release.patch

Comment 12

[Andrei Eftimie]

Comment on attachment 8441222 [details] [diff] [review]
skip_release.patch

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

Disabled:
https://hg.mozilla.org/qa/mozmill-tests/rev/db8b8621c485 (mozilla-release)

Comment 13

[Andreea Matei [:AndreeaMatei]]

Daniel, please investigate this tomorrow.

Comment 14

[Daniel Gherasim]

(In reply to Andreea Matei [:AndreeaMatei] from comment #13)
> Daniel, please investigate this tomorrow.

This is the same as the old 'aElement is undefined' error as we change the code in bug 994040. 
This started to fail in 2014-04-04 & it's intermittently.

I couldn't reproduce it at all in 500 runs.

I would suggest to wait for the correct state of the 'addButton' element:
> expect.waitFor(() => {
>   var addButton = addonPage.getElement({type: "install-button"});
>   return addButton !== "undefined";
> , "Button has been loaded");

That way we can now if it's happening because we click on addButton very fast or the element doesn't get loaded at all.

The only thing it's changing when button becomes "+ Add to Firefox" is the class, so it won't interfere.

Comment 15

[Daniel Gherasim]

Attachment: fix_1.patch

Hi, some results here:

* 100 tests on mm-osx-107-1 & got no failure.
* 600 locally & still no failure.

Used in both cases the latest version of Aurora en-US & I ran just the test itself.
So I couldn't reproduce this at all. 
Not sure what could happened here, maybe some network issues, I ran the test with a slow internet speed but couldn't reproduce this issue at all.

I would unskip the test but also wait for the button to be in the correct state to click on it, so no another event could  interfere.

Comment 16

[Andrei Eftimie]

Comment on attachment 8447981 [details] [diff] [review]
fix_1.patch

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

This patch should first land on default, not directly on aurura.
Please also add a commit message.

::: firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test1.js
@@ +68,5 @@
>    });
>  
>    locationBar.waitForNotificationPanel(() => {
> +    expect.waitFor(() => addButton &&
> +                         addButton.getNode().classList.contains('add'),

I don't see waiting for the 'add' class might help us.
The button starts as "Download" and it's contents get changed via Javascript to "+ Add to Firefox". (at the same time the "add" class gets added).

I've tested this page with Javascript turned off and the link still works as expected.
So even if we click before the "add" class and the content text has changed, this still works.

I've also looked into a potential problem where maybe the button size gets down to nothing while the content gets updated, but this doesn't appear to be the case (I've recorded the window while loading the page and I've got the change an 2 consecutive frames, there's no animation or "empty" button anywhere across the timeline).

Maybe we should use isDisplayed() this checks that the element is drawn on a visible part (and should be enough if our problem might be that we click the button before/during the CSS gets applied)

@@ +69,5 @@
>  
>    locationBar.waitForNotificationPanel(() => {
> +    expect.waitFor(() => addButton &&
> +                         addButton.getNode().classList.contains('add'),
> +                   "Button is now '+ Add to Firefox'");

I wouldn't include the actual button text in the message.
Something along the lines: "Add extension to Firefox button is ready"

Comment 17

[Daniel Gherasim]

Attachment: fix_2.patch

Updated as requested.

Comment 18

[Andrei Eftimie]

Comment on attachment 8450807 [details] [diff] [review]
fix_2.patch

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

I've landed this on default:
https://hg.mozilla.org/qa/mozmill-tests/rev/7d30f6b1ab5c (default)

Please monitor the results for a few days to see if we still have failures.

Comment 19

[Andrei Eftimie]

We've had no failures on Nightly.
Works fine on Aurora as well.

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/1378c9bff2d9 (mozilla-aurora)

Comment 20

[Andrei Eftimie]

Let's get this fixed on Beta, patch works fine:
http://mozmill-crowd.blargon7.com/#/remote/report/233c96fc2ff8f1c1a44659ece21b8b2d

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/76f7df61ebaf (mozilla-beta)

Branch 30 is obsolete, we're going to have the merge today for release 31.