Lookup class does no longer expose the `expression` property

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: Daniela Petrovici, Assigned: Daniela Petrovici)

Tracking

({regression})

Details

(Whiteboard: [mozmill-2.0+][ateamtrack: p=mozmill q=2013q2 m=2] [sprint2013-30])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
This was found while testing bug 860659. 

The Lookup method from elementslib in Mozmill 2.0 (https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/elementslib.js#L435) does not expose the expression used for finding the element. 

In mozmill 1.5.21 we were using it as described here: https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/elementslib.js#L301
Keywords: regression
Summary: Lookup method from elementslib does not expose the expression used for finding the element → Lookup class does no longer expose the `expression` property
Whiteboard: [mozmill-2.0?]
(Assignee)

Comment 1

5 years ago
Created attachment 740719 [details] [diff] [review]
patch v1.0

Created patch to expose the lookup expression. Tested with the minimized test case in bug 860659. 

Also, test testPopups/testPopupsBlocked.js will work with this change, but in testPopups/testPopupsAllowed.js we are checking for an element that does not exist at line hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testPopups/testPopupsAllowed.js#l46 (this is bug 864375).
Assignee: nobody → dpetrovici
Status: NEW → ASSIGNED
Attachment #740719 - Flags: review?(andreea.matei)
Comment on attachment 740719 [details] [diff] [review]
patch v1.0

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

Looks good and in addition with my fix for bug 864375, we solve the lookup and ID issues :)
Attachment #740719 - Flags: review?(andreea.matei) → review+
Keywords: checkin-needed
Andreea, you are not a peer for Mozmill, so any patch needs a review from myself, Dave or Clint. Removing checkin keyword.
Keywords: checkin-needed
Attachment #740719 - Flags: review?(hskupin)
Comment on attachment 740719 [details] [diff] [review]
patch v1.0

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

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +71,5 @@
>  
>  var Lookup = function (document, expression) {
> +  var elem = createInstance("Lookup", expression, elementslib.Lookup(document, expression), document);
> +  elem.expression = expression;
> +  return elem;

We should not define the expression property in mozelement but in elementslib similar to the code for Mozmill 1.5. If we do this here it will just be a wallpaper fix.

Also please add a js test beside the other ones in the elementslib subfolder.
Attachment #740719 - Flags: review?(hskupin) → review-
(Assignee)

Comment 5

5 years ago
Based on the discussion in Ask an expert: elementslib was changed to return the node directly. My understanding is that mozelement is the place where we should add the *.expression value. Henrik, can you please look over the patch again and tell me if I changed the correct method?
Comment on attachment 740719 [details] [diff] [review]
patch v1.0

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

Please re-request review in the future.
Attachment #740719 - Flags: review- → review?(hskupin)
Comment on attachment 740719 [details] [diff] [review]
patch v1.0

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

Please re-request review in the future.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +71,5 @@
>  
>  var Lookup = function (document, expression) {
> +  var elem = createInstance("Lookup", expression, elementslib.Lookup(document, expression), document);
> +  elem.expression = expression;
> +  return elem;

I re-checked the code we have now and as we talked in our meeting today, this should be the right place to add this property for backward compatibility. But there is one thing we should do here, which is not to assign the expression parameter but elem._locator. That property stores the information how to access a specific element via the selected locatorType. It should be safer to access this property given that it could have been changed by createInstance. Beside that please add a comment before which mentions that this only exists for backward compatibility. Further please add a blank line before the return statement.

Beside that the test is still required.
Attachment #740719 - Flags: review?(hskupin) → review-
(Assignee)

Comment 8

5 years ago
Created attachment 745082 [details] [diff] [review]
patch v1.1

Modified the Lookup function in mozelement to use elem._locator for the elem.expression. Added comment about backwards compatibility and an empty line before return.

Added the test for lookup expression.
Attachment #740719 - Attachment is obsolete: true
Attachment #745082 - Flags: review?(hskupin)
Attachment #745082 - Flags: review?(dave.hunt)
Comment on attachment 745082 [details] [diff] [review]
patch v1.1

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

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +71,5 @@
>  
>  var Lookup = function (document, expression) {
> +  var elem = createInstance("Lookup", expression, elementslib.Lookup(document, expression), document);
> +
> +  // Used to maintain backwards compatibility

Please add the bug number as prefix as we always did in the past (see disabled tests)

::: mutt/mutt/tests/js/elementslib/lookup_expression.js
@@ +9,5 @@
> +  controller = mozmill.getBrowserController();
> +}
> +
> +var test = function () {
> +  controller.open(TEST_FOLDER + "singlediv.html");

Why do we have to open a test page? The check below only operates on chrome nodes.

@@ +22,5 @@
> +  var searchPopup = new elementslib.Lookup(controller.window.document,
> +                                         searchBar.expression +
> +                                         '/anon({"anonid":"searchbar-textbox"})' +
> +                                         '/[0]/anon({"anonid":"searchbar-engine-button"})' +
> +                                         '/anon({"anonid":"searchbar-popup"})');

I wouldn't make it so complicated. For the base element use `/id("main-window")`, get its expression. If that is identical to the formerly specified locator we are fine.
Attachment #745082 - Flags: review?(hskupin)
Attachment #745082 - Flags: review?(dave.hunt)
Attachment #745082 - Flags: review-
(Assignee)

Comment 10

5 years ago
Created attachment 745097 [details] [diff] [review]
patch v1.2

Modified based on review.
Attachment #745082 - Attachment is obsolete: true
Attachment #745097 - Flags: review?(hskupin)
Comment on attachment 745097 [details] [diff] [review]
patch v1.2

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

::: mutt/mutt/tests/js/elementslib/lookup_expression.js
@@ +5,5 @@
> +var setupModule = function () {
> +  controller = mozmill.getBrowserController();
> +}
> +
> +var test = function () {

Give this method a name so we can distinct it from other possible tests in the future.

@@ +9,5 @@
> +var test = function () {
> +  var expression = '/id("main-window")';
> +  var element = new elementslib.Lookup(controller.window.document, expression);
> +  assert.equal(element.expression, expression,
> +               "The locator has been exposed by Lookup method");

Please change locator to expression. That's what publicly facing. Also 'by the Lookup'.

::: mutt/mutt/tests/js/elementslib/tests.ini
@@ +1,4 @@
>  [DEFAULT]
>  type = javascript
>  
> +[lookup_expression.js]

You missed to rename the file as I have mentioned in my last review.
Attachment #745097 - Flags: review?(hskupin) → review-
(Assignee)

Comment 12

5 years ago
Created attachment 745140 [details] [diff] [review]
patch v1.3

(In reply to Henrik Skupin (:whimboo) from comment #11)
> > +[lookup_expression.js]
> 
> You missed to rename the file as I have mentioned in my last review.

I am sorry, but I did not see this in your last review. Can you please tell me to what should I rename the test?

Apart from the name, I have made the requested changes.
Attachment #745097 - Attachment is obsolete: true
Attachment #745140 - Flags: review?(hskupin)
(In reply to Daniela Petrovici from comment #12)
> > > +[lookup_expression.js]
> > 
> > You missed to rename the file as I have mentioned in my last review.
> 
> I am sorry, but I did not see this in your last review. Can you please tell
> me to what should I rename the test?

Strange. Seems like the comment didn't came through. What I tried to add was that the test should not only exist for the expression check but lookup elements in detail. Make the test module re-usable for other bugs.
Comment on attachment 745140 [details] [diff] [review]
patch v1.3

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

r=me with the file name change. Which I will do before it's landing so we can have this fix in the next RC.
Attachment #745140 - Flags: review?(hskupin) → review+
Landed on master:
https://github.com/mozilla/mozmill/commit/8d4e017b7374051c6499df944698732d7e2718d2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-2.0?] → [mozmill-2.0?][ateamtrack: p=mozmill q=2013q2 m=2]

Updated

5 years ago
Whiteboard: [mozmill-2.0?][ateamtrack: p=mozmill q=2013q2 m=2] → [mozmill-2.0?][ateamtrack: p=mozmill q=2013q2 m=2] [sprint2013-30]
Whiteboard: [mozmill-2.0?][ateamtrack: p=mozmill q=2013q2 m=2] [sprint2013-30] → [mozmill-2.0+][ateamtrack: p=mozmill q=2013q2 m=2] [sprint2013-30]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.