1050253 - [ff, fa] Test failure "node.localName is undefined" in testSafeBrowsingNotificationBar.js

Status: RESOLVED FIXED

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

Description

[Andreea Matei [:AndreeaMatei]]

This fails with Beta ff and fa locales, all platforms:
http://mozmill-sandbox.blargon7.com/#/remote/report/ff9854eb4c07409123869856274b0d34
http://mozmill-sandbox.blargon7.com/#/remote/report/ff9854eb4c07409123869856274a44ce

I can reproduce this on my Linux, with ff locale, only by running the test.

Comment 1

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

Which element is that about? May be good to also add this to the summary.

Comment 2

[Andreea Matei [:AndreeaMatei]]

Yeah, I wanted to add the line in a follow up comment, was still tracking it cause it doesn't seem that obvious in the 'show more' lines. 
In the test is as this line:
http://hg.mozilla.org/qa/mozmill-tests/file/mozilla-beta/firefox/tests/remote/testSecurity/testSafeBrowsingNotificationBar.js#l118
For the button not a forgery in the notification bar. 

Points in tabs:
http://hg.mozilla.org/qa/mozmill-tests/file/mozilla-beta/firefox/lib/tabs.js#l717

Then in mozmill -> mozelement I couldn't find the correlation at those lines, seems here: https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/mozelement.js#L852
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/mozelement.js#L40

Although in the test I don't see a checkbox mentioned.

Comment 3

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

I assume that the panel is not shown, which we try to access here.

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: Screenshot from 2014-08-13 11:58:36.png

Okey I found the issue here.
After returning the node, when trying to to instantiate the new mozelemnet it goes through a factory method to check the type of node, for this it tries to get the nodes 'localName' which is undefined.
https://github.com/mozilla/mozmill/blob/2.0rc6/mozmill/mozmill/extension/resource/driver/mozelement.js#L847
The reason it's undefined here is because we assume the node is a node but it's an array, because the lookup expression returned two nodes.
https://github.com/mozilla/mozmill/blob/2.0rc6/mozmill/mozmill/extension/resource/driver/elementslib.js#L294
That's because we have the same lookup expression for both buttons, they bot have the same access key(see the attached image).

I will look why we have the duplicate in the access key, but I honestly think we should also file a bug in mozmill to log this exception better so next time we hit this we would know why it fails, and we won't waste time doing the same investigation as here.

Comment 5

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

Interesting. It indeed sounds like an access key conflict then. Also I was not aware that elementslib is returning arrays. We simply haven't used it so far. Good to know.

Comment 6

[Andrei Eftimie]

Good find Cosmin.

Please file a bug for each locale to get this access key fixed.
And file a bug against mozmill to fix the expression to return only one node if there are multiple instances (as we have for the Selector method)

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

I filed a bug 1053150 for mozmill. I couldn't reproduce the failure for "fa" locale, and it doesn't have a duplicate access key.

Comment 8

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

Can we please get this temporarily fixed in our tests. It should not block getting the locale from the blacklist. We can check for an array and take the first element, or the element directly if not an array.

Comment 9

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 1050253.patch

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Can we please get this temporarily fixed in our tests. It should not block
> getting the locale from the blacklist. We can check for an array and take
> the first element, or the element directly if not an array.
We can get this permanently fixed if we switch to label instead of access key terms of element locator.
Regarding the issue in mozmill we can't handle it in a easy way in mozmill-tests (we will have to switch 
to using node collector with multiple query for returning a single element)

Henrik what do you think about this fix? I think that using lable is much more safer because it will always be different, and also the change will be small and clean.

http://mozmill-crowd.blargon7.com/#/remote/report/53ec3148fca67aea1b1c3528f9244505
http://mozmill-crowd.blargon7.com/#/remote/report/53ec3148fca67aea1b1c3528f9247419

Comment 10

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

Comment on attachment 8498069 [details] [diff] [review]
1050253.patch

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

Looks perfect to me.

Comment 11

[Andrei Eftimie]

Nightly and Aurora have already been fixed.

We have 2 options here:
- land the fix only for Beta, Release, ESR31. (hopefully train merges will run smoothly)
- land the fix across branches to maintain parity (we'll lose our ability to catch future regressions in regard to access keys)

Comment 12

[Andrei Eftimie]

Comment on attachment 8498069 [details] [diff] [review]
1050253.patch

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

Looks fine to me.
Spot-checked a few locales and we don't seem to have any issues with the label.

Comment 13

[Andrei Eftimie]

remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/ebb8b498f87a (default)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/9ca9049c4d70 (mozilla-aurora)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/11976fc5de5d (mozilla-beta)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/a110ad907a22 (mozilla-release)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/f1763a220c1d (mozilla-esr31)

Comment 14

[Andrei Eftimie]

Forgot to mention, as discussed on IRC, we decided to land it across branches for parity.

We are working on different tests specifically aimed at Access Keys. These kind of regression will be caught by them in the future.