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

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: AndreeaMatei, Assigned: Andrei Eftimie)

Tracking

unspecified
All
Linux
Dependency tree / graph

Firefox Tracking Flags

(firefox27 disabled, firefox28 fixed)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Assignee: nobody → andreea.matei
Status: NEW → ASSIGNED
status-firefox27: --- → affected
Priority: -- → P1
(Reporter)

Updated

5 years ago
Whiteboard: [mozmill-test-failure]
(Reporter)

Comment 1

5 years ago
Created attachment 806600 [details] [diff] [review]
skipTest.patch

Disabling test.
Attachment #806600 - Flags: review?(andrei.eftimie)
(Assignee)

Comment 2

5 years ago
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.
Attachment #806600 - Flags: review?(andrei.eftimie)
Attachment #806600 - Flags: review+
Attachment #806600 - Flags: checkin+
(Assignee)

Updated

5 years ago
status-firefox27: affected → disabled
(Assignee)

Updated

5 years ago
Assignee: andreea.matei → andrei.eftimie
(Assignee)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
Probably bug 916534
Are we using frames in our testcase for this mozmill test? Seems like that bug is related to those.
(Assignee)

Comment 6

5 years ago
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
(Assignee)

Comment 7

5 years ago
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?
Flags: needinfo?(hskupin)
Flags: needinfo?(dave.hunt)
(Assignee)

Comment 8

5 years ago
Created attachment 811092 [details] [diff] [review]
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.
Attachment #811092 - Flags: feedback?(hskupin)
Attachment #811092 - Flags: feedback?(dave.hunt)
Attachment #811092 - Flags: feedback?(andreea.matei)
Flags: needinfo?(hskupin)
Flags: needinfo?(dave.hunt)
(Reporter)

Comment 9

5 years ago
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.
Attachment #811092 - Flags: feedback?(andreea.matei) → feedback+
(Assignee)

Comment 10

5 years ago
Created attachment 811963 [details] [diff] [review]
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.
Attachment #811092 - Attachment is obsolete: true
Attachment #811092 - Flags: feedback?(hskupin)
Attachment #811092 - Flags: feedback?(dave.hunt)
Attachment #811963 - Flags: review?(hskupin)
Attachment #811963 - Flags: review?(dave.hunt)
Attachment #811963 - Flags: review?(andreea.matei)
(Assignee)

Updated

5 years ago
Depends on: 922040
(Reporter)

Comment 11

5 years ago
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.
Attachment #811963 - Flags: review?(hskupin)
Attachment #811963 - Flags: review?(dave.hunt)
Attachment #811963 - Flags: review?(andreea.matei)
(Assignee)

Comment 12

4 years ago
Created attachment 826675 [details] [diff] [review]
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
Attachment #811963 - Attachment is obsolete: true
Attachment #826675 - Flags: review?(hskupin)
Attachment #826675 - Flags: review?(dave.hunt)
Attachment #826675 - Flags: review?(andreea.matei)
(Assignee)

Updated

4 years ago
status-firefox28: --- → disabled
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.
Attachment #826675 - Flags: review?(hskupin)
Attachment #826675 - Flags: review?(dave.hunt)
Attachment #826675 - Flags: review?(andreea.matei)
(Assignee)

Comment 14

4 years ago
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?
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.
(Assignee)

Comment 16

4 years ago
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)
(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.
(Assignee)

Comment 18

4 years ago
Created attachment 8336046 [details] [diff] [review]
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
Attachment #826675 - Attachment is obsolete: true
Attachment #8336046 - Flags: review?(hskupin)
Attachment #8336046 - Flags: review?(andreea.matei)
(Assignee)

Comment 19

4 years ago
(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 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.
Attachment #8336046 - Attachment is obsolete: true
Attachment #8336046 - Flags: review?(hskupin)
Attachment #8336046 - Flags: review?(andreea.matei)
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.
(Assignee)

Comment 22

4 years ago
(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.
(Assignee)

Comment 23

4 years ago
Created attachment 8336607 [details] [diff] [review]
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
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].
(Assignee)

Comment 25

4 years ago
Created attachment 8336663 [details]
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.
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.
(Assignee)

Updated

4 years ago
Depends on: 835739
(Assignee)

Comment 27

4 years ago
This has been fixed with landing of bug 835739.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
status-firefox28: disabled → fixed
You need to log in before you can comment on or make changes to this bug.