917771 - Test failure "The next result has been selected - '0' should not equal '0'" in testFindInPage

Status: RESOLVED FIXED

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

Description

[Andreea Matei [:AndreeaMatei]]

Started to fail today on Nightly, with all locales, on Ubuntu so far.
I can reproduce locally, I see the findbar has been moved up and no text gets inserted there.

I'm going to create a skip patch if I don't see the fix really quick.

Comment 1

[Andreea Matei [:AndreeaMatei]]

Attachment: skipTest.patch

Disabling test.

Comment 2

[Andrei Eftimie]

Comment on attachment 806600 [details] [diff] [review]
skipTest.patch

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

Skip looks good.
Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/49c80d86df3f (default)

OSX does not seem to be affected.
We should find the responsible changeset.

Comment 3

[Andrei Eftimie]

Mozilla central pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1d27c4c9871f&tochange=ab4ccf3d6b60

This also fails on Aurora, but the test has been skipped there in bug 909227

Comment 4

[Andrei Eftimie]

Probably bug 916534

Comment 5

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

Are we using frames in our testcase for this mozmill test? Seems like that bug is related to those.

Comment 6

[Andrei Eftimie]

A more restricted pushlog, this one on inbound:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=76f9cced8105&tochange=15b2fe123070

Mike de Boer — Backed out 3 changesets (bug 893446, bug 893011, bug 869543) Backed out changeset 452ca8b779f3 (bug 893446) Backed out changeset e1bf70be67ec (bug 893011) Backed out changeset 60865db5f5dc (bug 869543)

This actually looks more like backing out bug 869543

Comment 7

[Andrei Eftimie]

Well I've tried changing the lookup expression to match the new (actually old) position.
Basically http://hg.mozilla.org/qa/mozmill-tests/file/580ec7cca527/tests/functional/testFindInPage/testFindInPage.js#l24
into:
>  '/anon({"class":"browserContainer"})/[1]' +

But I haven't been able to make it work.
I've tried lots of variants of the Lookup expression without success.

Then I've noticed that this test fails on 2.0
Even prior to any recent changes it fails. We didn't caught this probably because it was skipped. (Than again, the skip only landed in 11th Sept we should have seen it before. I don't remember seeing this fail).

TLDR:
- test fails in 2.0 consistently (with versions that pass in 1.5)
- recent changes don't allow an easy fix for 1.5

Should we try to refactor the Lookup expressions into NodeCollector directly for 2.0?

Comment 8

[Andrei Eftimie]

Attachment: WIP 1.patch

This is a potential fix, I've refactored the Lookup expressions into NodeCollector queries.

This is still WIP as it currently fails in mozmill 1.5:
> "The root element has been specified. - got 'undefined'"

The other change that was needed was to move the queries _after_ we open the findBar, as the findBar is injected into the DOM when it is invoked.

I'll look into fixing this for 1.5, hopefully the fix will be easy/quick and not require a bug workaround.

Comment 9

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 811092 [details] [diff] [review]
WIP 1.patch

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

I like more using nodeCollector than the Lookup expressions which always fail when the child order changes. But we need to figure out why it's failing with this on 1.5.

Comment 10

[Andrei Eftimie]

Attachment: 2.patch

Works on both mozmill 2.0 and mozmill 1.5

Here are reports under 1.5:
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a7503f5780
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a7503d90eb

The failure in windows is a regression in Firefox, I can reproduce that manually.

I'll shortly have a blocker opened that describes the regression.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 811963 [details] [diff] [review]
2.patch

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

Looks good, thanks for the update. But until the dependency gets fixed I cannot check in this, so please add the r? flags then.

Comment 12

[Andrei Eftimie]

Attachment: 3.patch

Our blocker has been fixed!

Updated patch (small update to setting a new root to nodeCollector with nc.root instead of creating a new instance).

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/ad464590da4913b3dafe381be13c9109
http://mozmill-crowd.blargon7.com/#/functional/report/ad464590da4913b3dafe381be13e75dd
http://mozmill-crowd.blargon7.com/#/functional/report/ad464590da4913b3dafe381be1409efe

Comment 13

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

Comment on attachment 826675 [details] [diff] [review]
3.patch

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

Why do we need a full transition over to nodeCollector? I really don't understand this. Also this work is done by another bug. Please explain.

Comment 14

[Andrei Eftimie]

I've noted in comment 7 that while indeed the position has been changed back, we should have been able to revert just that.

However that approached failed. I wasn't able to get the elements.
Given that we already want to remove Lookup expressions, I've tried that and lo and behold it worked perfectly.

We were blocked by another bug which made the specific test failed which has now been fixed (bug  922040)

I don't really understand the need to take all the review flags out.
Shouldn't we fix this test?

Comment 15

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

I don't understand why it is not possible to simply revert the changes we did to retrieve the elements again.

Also if we want to use nodeCollector it should be done correctly and move out all of the element ids to the ui library. But please not that way.

So please leave the lookup expressions here and make it work.

Comment 16

[Andrei Eftimie]

I am unable to make this work with Lookup expressions.
We always fail in retrieving the findbar element.
(I've tried to retrieve the element in numerous ways, and always failed)

Lets try making some headway.
I'll create a UI library for the Findbar (I think it should live in toolbars.lib)

Comment 17

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

(In reply to Andrei Eftimie from comment #16)
> I am unable to make this work with Lookup expressions.
> We always fail in retrieving the findbar element.
> (I've tried to retrieve the element in numerous ways, and always failed)

You never give example code. How should someone help in such cases? Please try to be more verbose also in terms of failure and exception output, or logs.

Comment 18

[Andrei Eftimie]

Attachment: 4.patch

I've created a new UI Class for the findBar in the toolbars.js lib

I've looked through the "Refactor Lookup Expressions" bugs and there wasn't one specific for the findBar.

Reports
=======

Mozmill 2.0
http://mozmill-crowd.blargon7.com/#/functional/report/456bebe92845279408c15c03e80c4cce

Mozmill 1.5
http://mozmill-crowd.blargon7.com/#/functional/report/456bebe92845279408c15c03e811c457

Comment 19

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> You never give example code. How should someone help in such cases? Please
> try to be more verbose also in terms of failure and exception output, or
> logs.

I did explain what I tried to do (maybe with not enough details in comment 7).

Well here is the basic gist of it

Changing the relevant code to:
> aModule.containerString = expression + '/anon({"class":"browserSidebarContainer"})' +
>                           '/anon({"class":"browserContainer"})/[1]' +
>                           '/anon({"anonid":"findbar-container"})' +
>                           '/anon({"anonid":"findbar-textbox-wrapper"})';
-- the only change is from [0] to [1], DOM inspector sees the same thing

Gives in mozmill 1.5:
> "message": "Timeout exceeded for waitForElement Lookup: /id(\"main-window\")/id(\"tab-view-deck\")/[0]/id(\"content-deck\")/id(\"browser\")/id(\"appcontent\")/id(\"content\")/anon({\"anonid\":\"tabbox\"})/anon({\"anonid\":\"panelcontainer\"})/{\"id\":\"panel-3-1\"}/anon({\"class\":\"browserSidebarContainer\"})/anon({\"class\":\"browserContainer\"})/[1]/anon({\"anonid\":\"findbar-container\"})/anon({\"anonid\":\"findbar-textbox-wrapper\"})"

And in mozmill 2.0:
> ERROR | Test Failure | {
>   "exception": {
>     "message": "Expression \"[1]\" returned null. Anonymous == false", 
>     "lineNumber": 489, 
>     "name": "SyntaxError", 
>     "fileName": "resource://mozmill/driver/elementslib.js"
>   }
> }


I don't find it worth it to dwell on this to much.
I've already spent much more time then it would have been worth it trying to fix
the Lookup expression. Which is already deprecated.

Why not fix both things? Update the code to use nc and make the test work again.

Comment 20

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

Comment on attachment 8336046 [details] [diff] [review]
4.patch

I don't want to repeat it again, but the work done here has already been done and was only waiting for a final polish. I have mentioned bug 835739 a couple of times and it has been forgotten again. This patch does NOT relate to this bug, Andrei.

Comment 21

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

Also for examples I would like to see real code which can directly be executed. I don't want to have to copy and paste all of it into a new mozmill test.

Comment 22

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> I don't want to repeat it again, but the work done here has already been
> done and was only waiting for a final polish. I have mentioned bug 835739 a
> couple of times and it has been forgotten again.

I am sorry, but you have not mentioned bug 835739.

You did say `Also this work is done by another bug.` in comment 13.
That was a bit vague for me to find it.

> Also for examples I would like to see real code which can directly be executed. I
> don't want to have to copy and paste all of it into a new mozmill test.

I'll upload a patch that doesn't work if that's what you mean.

> This patch does NOT relate to this bug, Andrei.

I disagree.
We have a mozmill-test failure.
We want to fix it ASAP.
This patch fixes the said failure, thus this patch is relevant.

Now that you have mentioned bug 835739, yes, there is a duplication of effort.
Maybe we'll mark this as dependant to the said bug.

I don't really care how or who gets to fix things, as long as we move forward.

Comment 23

[Andrei Eftimie]

Attachment: does_not_work.patch

This passes in mozmill 1.5
This fails in mozmill 2.0
>  "message": "Expression \"[2]\" returned null. Anonymous == false",

**Use this only with the latest Nightly since very recently there's a new element beofre the <findbar> so the relevant code had to be changed from [1] to [2]

I'm uploading this as per comment 21

Comment 24

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

You are not changing the relevant code from [1] to [2] but from [0] to [2]. It's clear that this will not work. If only a single node is located before the findbar under the browserContainer the index has to be [1] and not [2].

Comment 25

[Andrei Eftimie]

Attachment: dom-inspector-nodes.png

(In reply to Henrik Skupin (:whimboo) from comment #24)
> You are not changing the relevant code from [1] to [2] but from [0] to [2].
> It's clear that this will not work. If only a single node is located before
> the findbar under the browserContainer the index has to be [1] and not [2].

Have you even read what I've written?

Here is a screenshot from the latest Nightly with DOM inspector.
The findbar is the 3rd child. AKA [2] in a zero-based index environment.

This has changed _very_ recently, until a few days ago the patch would have had [0] changed into [1]. Now we have to change it into [2] (see attached screenshot).

It fails in mozmill 2.0 either way.

Comment 26

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

Hm, so I was indeed on a version which was two days old. I thought I updated yesterday. Anyway, I can see this statuspanel node now. A quick check revealed that the findbar node is not ALWAYS available, but gets dynamically injected when it is requested. So it's clear why the node cannot be found in your example code.

Comment 27

[Andrei Eftimie]

This has been fixed with landing of bug 835739.