860659 - elementslib.Lookup() fails if element in reduceLookup() does not exist yet

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Andrei Eftimie]

elementslib.Lookup() fails when using XPath(?) to find elements

Affected tests:

> /testFormManager/testDisableFormManager.js
> /testPopups/testPopupsAllowed.js
> /restartTests/testPreferences_masterPassword/test1.js

Comment 1

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

The failure message would be kinda helpful here.

Comment 2

[Andrei Eftimie]

Failure is (as noted in the summary):
> Expression \"id(\"main-window\")\" returned null. Anonymous == false"

Where the expression varies by test, but is an xpath like structure.
(I'm guessing the way Mozmill IDE would have generated it?)

Comment 3

[cmtalbert]

Need a minimized test case and more information for determining whether this should block or not.

Comment 4

[Daniela Petrovici]

Attachment: minimized test case

Both tests from testPopups folder are failing now, not just the testPopups/testPopupsAllowed
Test /testFormManager/testDisableFormManager.js still fails, but this is an easy fix. We are using a Lookup expression to get to the element although it has an ID. Changing the Lookup expression with the ID is working properly
Restart tests are working properly now

testPopups:
- The minimized testcase passes when removing this line from tabs.js library: http://hg.mozilla.org/qa/mozmill-tests/file/ee95d0ef804f/lib/tabs.js#l367 (So, this is the line due to which the test is failing)
The "panel.expression" in the above line is null. Adding "elem.expression = expression;" before returning from getElements function (http://hg.mozilla.org/qa/mozmill-tests/file/ee95d0ef804f/lib/tabs.js#l317) fixes this issue.
Apart from the above issue, the Lookup function does not seem to work for the element, even if we add its expression. Mozmill 1.5 has a different Lookup fuunction and does not rely on any code that resembles the one in 2.0. Also the Lookup.prototype.getNode from mozmill-1.5 (https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/elementslib.js#L428) is the same as the Lookup method from mozmill 2.0, but it is never called by our test in the step that fails. I am not sure if this helps.

NOTE: I have added the minimized test case for testPopupsAllowed.

Comment 5

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

(In reply to Daniela Petrovici from comment #4)
> has an ID. Changing the Lookup expression with the ID is working properly
> Restart tests are working properly now

We are not going to workaround a problem, Daniela. If lookup calls are failing this is a bad regression, which has to be fixed. Also this is not a minimized testcase because it still contains a lot of references to other (even) mozmill-tests related libraries. I can remember we told this already a while ago.

Comment 6

[Daniela Petrovici]

Attachment: minimized test case

> (In reply to Daniela Petrovici from comment #4)
> > has an ID. Changing the Lookup expression with the ID is working properly
> > Restart tests are working properly now
> 
> We are not going to workaround a problem, Daniela. If lookup calls are
> failing this is a bad regression, which has to be fixed. 
I did not mean to say that we can workaround the problem, sorry if I was not clear in the comment. I just wanted to add all the results of my investigation to this bug.

> Also this is not a minimized testcase because it still contains a lot of references to other
> (even) mozmill-tests related libraries. I can remember we told this already
> a while ago.
You are right, thank you. I changed the test now to not make references to any mozmill-test related libraries.

Comment 7

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

Where do we fail in this test? Keep in mind that .getNode() currently doesn't exist in Mozmill 2.0.

Comment 8

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> Where do we fail in this test? Keep in mind that .getNode() currently
> doesn't exist in Mozmill 2.0.

The test fails in the last line:   
var button = new elementslib.Lookup(controller.window.document, panel.expression + elemStr);
If you remove it, the test will pass. 

Also no error appears when using tabs.getNode().

Comment 9

[Daniela Petrovici]

Attachment: minimized test case

Made a mistake at line 24, but it doesn't affect the result. I corrected it now.

Comment 10

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

I have absolutely no idea what panel.expression should be. Where are you getting this from? What do you think it should contain? This is also not part of our current tests in mozmill-tests.

Given that the other lookup strings work, what are the detailed failures as reported by Andrei originally?

Comment 11

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> I have absolutely no idea what panel.expression should be. Where are you
> getting this from? 
This is from the getTabPanelElement method from tabs library (http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/tabs.js#l367) and that the test uses here: http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testPopups/testPopupsAllowed.js#l46

> What do you think it should contain? 
In mozmill 1.5.21, we were setting this.expression = expression inside the Lookup methods from elementslib (https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/elementslib.js#L301). 

So, panel.expression should be the Lookup expression for the panel element. 
At the moment it is null since we are not setting it in the Lookup method from mozmill 2.0.

Also, as stated in comment #4, giving the correct value to panel.expression still does not fix the issue.

> This is also not
> part of our current tests in mozmill-tests.
As mentioned above, we are getting this from the tabs library (getTabPanelElement).

> Given that the other lookup strings work, what are the detailed failures as
> reported by Andrei originally?
The detailed failures are below:
1) testPopups (both tests are failing with the same error)
ERROR | Test Failure | {
  "exception": {
    "stack": [
      "reduceLookup@resource://mozmill/driver/elementslib.js:530", 
      "Lookup@resource://mozmill/driver/elementslib.js:549", 
      "Lookup@resource://mozmill/driver/mozelement.js:73", 
      "tabBrowser_getTabPanelElement@resource://mozmill/stdlib/securable-module.js -> file:///home/danielapetrovici/Documents/860659/22/mozmill-tests/lib/tabs.js:367", 
      "testPopUpBlocked@resource://mozmill/modules/frame.js -> file:///home/danielapetrovici/Documents/860659/22/mozmill-tests/tests/functional/testPopups/testPopupsBlocked.js:49", 
      "Runner.prototype.wrapper@resource://mozmill/modules/frame.js:634", 
      "Runner.prototype.runTestModule@resource://mozmill/modules/frame.js:704", 
      "Runner.prototype.runTestFile@resource://mozmill/modules/frame.js:602", 
      "runTestFile@resource://mozmill/modules/frame.js:749", 
      "Bridge.prototype._execFunction@resource://jsbridge/modules/Bridge.jsm:140", 
      "Bridge.prototype.execFunction@resource://jsbridge/modules/Bridge.jsm:147", 
      "@resource://jsbridge/modules/Server.jsm:33", 
      ""
    ], 
    "message": "Expression \"{\"value\":\"popup-blocked\"}\" returned null. Anonymous == false", 
    "fileName": "resource://mozmill/driver/elementslib.js", 
    "name": "Error", 
    "lineNumber": 530
  }
}
2) testFormManager/testDisableFormManager.js
ERROR | Test Failure | {
  "exception": {
    "stack": [
      "reduceLookup@resource://mozmill/driver/elementslib.js:530", 
      "Lookup@resource://mozmill/driver/elementslib.js:549", 
      "Lookup@resource://mozmill/driver/mozelement.js:73", 
      "testToggleFormManager@resource://mozmill/modules/frame.js -> file:///home/danielapetrovici/Documents/860659/22/mozmill-tests/tests/functional/testFormManager/testDisableFormManager.js:58", 
      "Runner.prototype.wrapper@resource://mozmill/modules/frame.js:634", 
      "Runner.prototype.runTestModule@resource://mozmill/modules/frame.js:704", 
      "Runner.prototype.runTestFile@resource://mozmill/modules/frame.js:602", 
      "runTestFile@resource://mozmill/modules/frame.js:749", 
      "Bridge.prototype._execFunction@resource://jsbridge/modules/Bridge.jsm:140", 
      "Bridge.prototype.execFunction@resource://jsbridge/modules/Bridge.jsm:147", 
      "@resource://jsbridge/modules/Server.jsm:33", 
      ""
    ], 
    "message": "Expression \"id(\"main-window\")\" returned null. Anonymous == false", 
    "fileName": "resource://mozmill/driver/elementslib.js", 
    "name": "Error", 
    "lineNumber": 530
  }
}

Comment 12

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

(In reply to Daniela Petrovici from comment #11)
> > I have absolutely no idea what panel.expression should be. Where are you
> > getting this from? 
> This is from the getTabPanelElement method from tabs library
> (http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/tabs.js#l367) and

Hm, I was searching the whole test folder but pycharm didn't show me that instance. 

> > What do you think it should contain? 
> In mozmill 1.5.21, we were setting this.expression = expression inside the
> Lookup methods from elementslib
> (https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/
> extension/resource/modules/elementslib.js#L301). 
> 
> So, panel.expression should be the Lookup expression for the panel element. 
> At the moment it is null since we are not setting it in the Lookup method
> from mozmill 2.0.

If we do not expose this property at all in 2.0 we should file a bug and get it fixed. 

> Also, as stated in comment #4, giving the correct value to panel.expression
> still does not fix the issue.

This can be handled here once the dependent (and to file) bug has been fixed.

>     "message": "Expression \"id(\"main-window\")\" returned null. Anonymous
> == false", 

This should never return null, unless we do not have a browser window active. Does a sleep at the beginning of the test help?

Comment 13

[Daniela Petrovici]

Attachment: minimized test case

(In reply to Henrik Skupin (:whimboo) from comment #12)
> If we do not expose this property at all in 2.0 we should file a bug and get
> it fixed. 

Logged bug 864268 for this issue.

> This should never return null, unless we do not have a browser window
> active. Does a sleep at the beginning of the test help?

I have retested this from scratch today and using the minimized test case. 

When adding the elem.expression in http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/tabs.js#l317 - test fails.

But when adding the panel.expression in this new minimized test case, the test passes.

So, I think that fixing bug 864268 will fix this issue too.

Comment 14

[Daniela Petrovici]

Attachment: minimized test case for DisableFormManager error

Also added the minimized test case for /testFormManager/testDisableFormManager.js
This is not related to the bug 864268.

The minimized test case fails in the last line (when searching for the Loopup expression). The test already has a sleep of 5 seconds added just before trying to find the element. I have also added a sleep before opening the page. 

The error is still 
"message": "Expression \"id(\"main-window\")\" returned null. Anonymous == false", 

but the browser window is active when the error is returned.

Comment 15

[Daniela Petrovici]

Attachment: minimized test case for DisableFormManager in mozmill 1.5

This minimized test (/testFormManager/testDisableFormManager.js) is failing in mozmill 1.5.21 and the only difference between this one and the one above is the usage of getNode(). The Lookup method in 2.0 is the same as Lookup.prototype.getNode in 1.5.21 which is also failing.

The reason of the test not failing in 1.5.21 is the fact that we are not actually trying to find the element except when in line: 
http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testFormManager/testDisableFormManager.js#l65 where we use Lookup.prototype.exists which calls getNode inside a try-catch block and we verify that the element does not exist. 

With mozmill 2.0 we are trying to find the element in line http://hg.mozilla.org/qa/mozmill-tests/file/2d39f465b1a5/tests/functional/testFormManager/testDisableFormManager.js#l56 where we expect it exists. Since the element should not exist, the test is returning a failure.

I think this might be related to bug 761600

Comment 17

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

Attachment: testcase

Comment 18

[Daniela Petrovici]

Attachment: patch v1.0

Based on comment #5 in bug 865627 we should not do the fix in mozelement, but in elementslib.
The issue was that we were throwing an error (from reduceLookup method) in case the element was not found on the page even if we were expecting it to not exist.

I have changed the Lookup method so that it will return the element or null in case the element was not found. Also, as far as I can tell from the rest of the methods (Link, ID), we want to throw an error only if we don't have a locator as parameter, not if we do not find the element.

Comment 19

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

Comment on attachment 746866 [details] [diff] [review]
patch v1.0

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

This patch is not satisfying because it wall-papers the underlying issue and hides all various other kinds of exceptions which could be thrown due to a broken lookup string. So please try to understand the whole method first before working on a patch. If questions exist you know where to ask.

As what I can see is that we do not have to encapsulate this line at all into a try/catch. Instead lines 505-508 are pretty useless and should be removed instead. There is no need to fire an exception at all if the element doesn't exist.

Beside that I want to see a test included in a patch when you want to ask for review. That's practice for Mozmill related code.

I would like to get Clint's feedback too given that he also knows a lot about all that code.

Comment 20

[Daniela Petrovici]

Attachment: patch v2.0

Made the changes based on the previous review.

Comment 21

[Daniela Petrovici]

Comment on attachment 749788 [details] [diff] [review]
patch v2.0

Actually removing those line does not entirely fixes the issue. I will add a new minimized test case for which this will fail.

Comment 22

[Daniela Petrovici]

Attachment: patch v3.0

(In reply to Henrik Skupin (:whimboo) from comment #19)
> This patch is not satisfying because it wall-papers the underlying issue and
> hides all various other kinds of exceptions which could be thrown due to a
> broken lookup string.

The previous patch was not working for a Lookup expression like: "/id('mainWindow')/id('mainPopupSet')". It was because we were continuously trying to reduce the expression although we did not find an element with "mainWindow". We should exit from the reduceLookup method when we don't find the element.

So if we want to:
- not throw an exception if the element does not exist 
and
- throw an exception if the lookup expression is wrong,
then I thought of still using try/catch, but throwing the exception only if there is a different issue than the element not found.

Please tell me if this is not the correct approach.

NOTE: The test for which the previous patch was not working is in the patch.

The report for functional runs is http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c27d2e2f

Comment 23

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

Comment on attachment 750455 [details] [diff] [review]
patch v3.0

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

::: mozmill/mozmill/extension/resource/driver/elementslib.js
@@ +531,5 @@
> +  } catch (e) {
> +    if (e != exception)
> +      throw exception;
> +  }
> +  return returnedValue;

I don't understand why we have to use such a logic. Please have a look at the MDN page for Array.reduce():

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Array/Reduce

I think we can perfectly make use of 'previousValue' and if it was null we directly abort.

Otherwise we can fire specific errors if the lookup string is broken like SyntaxError. That way we could filter the not found exception.

::: mutt/mutt/tests/js/elementslib/lookup.js
@@ +18,5 @@
> +  controller.waitForPageLoad();
> +
> +  var textbox = new elementslib.Lookup(controller.tabs.activeTab,
> +                                       '/id("main-window")/id("mainPopupSet")');
> +  expect.ok(!textbox.exists(), 'Element via Lookup does not exist');

Please keep both tests. The first for the top main-window element and a second for the popupset. Also a nice one would be with an invalid lookup string.

Comment 24

[Daniela Petrovici]

Attachment: patch v4.0

Modified patch based on feedback. I have used parent as this is the previousValue here and checked if it was null/undefined.

Also, left both mutt tests and added one for invalid_lookup - it now contains the test for one invalid lookup expression. Since it will return a known fail I have added it to a different file so that the "expected=fail" could be added to the manifest file for it.

Comment 25

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

Comment on attachment 751019 [details] [diff] [review]
patch v4.0

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

::: mozmill/mozmill/extension/resource/driver/elementslib.js
@@ +446,5 @@
>    var reduceLookup = function (parent, exp) {
> +    // Abort in case the parent was not found
> +    if (!parent) {
> +      return false;
> +    }

That's fine but please add a jsdoc comment which explains that this is the previousValue of the reduce callback.

::: mutt/mutt/tests/js/elementslib/invalid_lookup.js
@@ +6,5 @@
> +  controller = mozmill.getBrowserController();
> +}
> +
> +var testInvalidExpression = function () {
> +  var element = new elementslib.Lookup(controller.window.document, '/id(main-window)');

Please move this into the lookup.js file. I don't see a reason for a separate test module.

::: mutt/mutt/tests/js/elementslib/lookup.js
@@ +21,5 @@
> +                                          '/id("main-window")');
> +  var popupSet = new elementslib.Lookup(controller.tabs.activeTab,
> +                                        '/id("main-window")/id("mainPopupSet")/id("test")');
> +  expect.ok(!mainWindow.exists(), 'Element via Lookup does not exist');
> +  expect.ok(!popupSet.exists(), 'Element via Lookup does not exist');

Please do not mix-up element creation and tests. Do it after each other and please work with blank line.

Comment 26

[Daniela Petrovici]

Attachment: patch v5.0

Modified patch based on feedback

Comment 27

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

This should be definitely a blocker given that it is the cause of a lot of trouble in our current set of Mozmill tests.

Comment 28

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

Comment on attachment 751610 [details] [diff] [review]
patch v5.0

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

Daniela, if you have questions about something added in this review, please let us know on IRC. I really want to get this patch in ASAP and the cycles necessary so far have been taken a lot of time. Thanks.

::: mozmill/mozmill/extension/resource/driver/elementslib.js
@@ +445,5 @@
>  
> +  /**
> +   * Reduces the lookup expression
> +   * @param {Object} parent
> +   *        previousValue of the reduce callback

Instead of Object could we use Node? Or of which type is it?

@@ +455,3 @@
>    var reduceLookup = function (parent, exp) {
> +    // Abort in case the parent was not found
> +    if (!parent) {

Out of interest, what value does parent have in the first iteration?

::: mutt/mutt/tests/js/elementslib/lookup.js
@@ +14,5 @@
>  }
> +
> +var testElementNotExists = function () {
> +  controller.open("http://www.mozilla.org");
> +  controller.waitForPageLoad();

Why do we have to open this page? We don't want external sites pulled in. Please remove it.

@@ +18,5 @@
> +  controller.waitForPageLoad();
> +
> +  var mainWindow = new elementslib.Lookup(controller.tabs.activeTab,
> +                                          '/id("main-window")');
> +  expect.ok(!mainWindow.exists(), 'Element via Lookup does not exist');

Why 'controller.tabs.activeTab'? We want to work on chrome scope. So simply try to get an element with the id 'main-windo'.

@@ +21,5 @@
> +                                          '/id("main-window")');
> +  expect.ok(!mainWindow.exists(), 'Element via Lookup does not exist');
> +
> +  var popupSet = new elementslib.Lookup(controller.tabs.activeTab,
> +                                        '/id("main-window")/id("mainPopupSet")');

Same as above here for the second id.

@@ +26,5 @@
> +  expect.ok(!popupSet.exists(), 'Element via Lookup does not exist');
> +}
> +
> +var testInvalidLookupExpression = function () {
> +  assert.throws(function () {

As said on various other places, unit tests should not assert. This has to be an expect.throws() call.

@@ +28,5 @@
> +
> +var testInvalidLookupExpression = function () {
> +  assert.throws(function () {
> +    var element = new elementslib.Lookup(controller.window.document, '/id(main-window)');
> +  }, undefined, "Invalid lookup expression failed");

reduceLookup() should throw SyntaxError instances if the expression is invalid. That way we could even check for the correct type here.

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/SyntaxError

Comment 29

[Daniela Petrovici]

Attachment: patch v6.0

(In reply to Henrik Skupin (:whimboo) from comment #28)
> Instead of Object could we use Node? Or of which type is it?
We can, but I dumped "typeof parent" and it returns "object", so I think it is better to use Object.

> Out of interest, what value does parent have in the first iteration?
Parent is [object XULDocument] in the first iteration

Comment 30

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

(In reply to Daniela Petrovici from comment #29)
> > Instead of Object could we use Node? Or of which type is it?
> We can, but I dumped "typeof parent" and it returns "object", so I think it
> is better to use Object.

It's not always that easy to determine the correct type of an object. We could keep it as Object and rename the parameter to parentNode.

> > Out of interest, what value does parent have in the first iteration?
> Parent is [object XULDocument] in the first iteration

Thanks. So it fully explains how it works.

Comment 31

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

Comment on attachment 752747 [details] [diff] [review]
patch v6.0

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

::: mozmill/mozmill/extension/resource/driver/elementslib.js
@@ +445,5 @@
>  
> +  /**
> +   * Reduces the lookup expression
> +   * @param {Object} parent
> +   *        previousValue of the reduce callback

parentNode please and update the description to reference that.

@@ +480,1 @@
>                          strings.vslice(exp, '[', ']') + ' ||');

The indentation is off here.

@@ +498,1 @@
>                             strings.vslice(exp, '(', ')') + '  ||');

... and here.

Has this passed internal review or haven't you requested that? I'm wondering about that.

::: mutt/mutt/tests/js/elementslib/lookup.js
@@ +15,5 @@
> +
> +var testElementNotExists = function () {
> +  var mainWindow = new elementslib.Lookup(controller.window.document,
> +                                          '/id("main-windo")');
> +  expect.ok(!mainWindow.exists(), 'Element via Lookup does not exist');

The message should be better so it reflects why it not exists.

@@ +18,5 @@
> +                                          '/id("main-windo")');
> +  expect.ok(!mainWindow.exists(), 'Element via Lookup does not exist');
> +
> +  var popupSet = new elementslib.Lookup(controller.window.document,
> +                                        '/id("main-windo")/id("mainPopupSet")');

Could we add a third case here where we make use of main-window but mainPopupSe? With that we should have covered each path for id. If necessary we can later add additional checks for classes and anonid specifiers.

Comment 32

[Daniela Petrovici]

Attachment: patch v7.0

Updated patch. Sorry about not requesting a more thorough internal review previously, only the implementation was checked, but not the final patch. I have requested it now.

Comment 33

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

Comment on attachment 752764 [details] [diff] [review]
patch v7.0

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

Can we please have a quick back and forth discussion on IRC before more and more patches are getting uploaded? I would like to see an end here.

::: mozmill/mozmill/extension/resource/driver/elementslib.js
@@ +445,5 @@
>  
> +  /**
> +   * Reduces the lookup expression
> +   * @param {Object} parentNode
> +   *        previousValue of the reduce callback

I mentioned that the description should also be updated. To safe us one more possible review cycle please use 'Parent node (previousValue of the formerly executed reduce callback)'

@@ +447,5 @@
> +   * Reduces the lookup expression
> +   * @param {Object} parentNode
> +   *        previousValue of the reduce callback
> +   * @param {String} exp
> +   *        Evaluated expression

The expression is getting evaluated in the method. So please use 'Lookup expression for the parents child node'

@@ +449,5 @@
> +   *        previousValue of the reduce callback
> +   * @param {String} exp
> +   *        Evaluated expression
> +   *
> +   * @returns {Object} Result of the evaluation

It returns the node found by the given expression.

::: mutt/mutt/tests/js/elementslib/lookup.js
@@ +15,5 @@
> +
> +var testElementNotExists = function () {
> +  var mainWindow = new elementslib.Lookup(controller.window.document,
> +                                          '/id("main-windo")');
> +  expect.ok(!mainWindow.exists(), 

nit: trailing whitespace.

@@ +16,5 @@
> +var testElementNotExists = function () {
> +  var mainWindow = new elementslib.Lookup(controller.window.document,
> +                                          '/id("main-windo")');
> +  expect.ok(!mainWindow.exists(), 
> +            'Lookup failed due to an element that does not exist');

It's not the lookup which failed here. The check here proofs that the element does not exist. Please be clear in the wording.

@@ +20,5 @@
> +            'Lookup failed due to an element that does not exist');
> +
> +  var popupSet = new elementslib.Lookup(controller.window.document,
> +                                        '/id("main-windo")/id("mainPopupSet")');
> +  expect.ok(!popupSet.exists(), 'Lookup failed due to the first node not existing');

Same here.

@@ +24,5 @@
> +  expect.ok(!popupSet.exists(), 'Lookup failed due to the first node not existing');
> +
> +  popupSet = new elementslib.Lookup(controller.window.document,
> +                                    '/id("main-window")/id("mainPopupSe")');
> +  expect.ok(!popupSet.exists(), 'Lookup failed due to the second node not existing');

And here.

Comment 34

[Daniela Petrovici]

Attachment: patch v8.0

Made changes per review.

Comment 35

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

Comment on attachment 752803 [details] [diff] [review]
patch v8.0

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

I think that looks great now. Lets get a review from Clint too, given that it changes the lookup code a lot.

Comment 36

[cmtalbert]

Comment on attachment 752803 [details] [diff] [review]
patch v8.0

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

This looks ok. r+

Comment 37

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

Landed on master:
https://github.com/mozilla/mozmill/commit/d0ec481962cea83d9e977199f6ae5354dddc230a