714802 - Update Mozmill shared-modules to use waitFor instead of waitForEval

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

To move to Mozmill 2.0, waitforEval calls will have to be changed to waitFor.

Comment 1

[Remus Pop (:RemusPop)]

Attachment: patch v1 (all branches)

This is an updated patch from bug 592275 for shared-modules. It applies cleanly on all branches.

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 585675 [details] [diff] [review]
patch v1 (all branches)

r+ over to Henrik

Comment 3

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

Comment on attachment 585675 [details] [diff] [review]
patch v1 (all branches)

>+      this._controller.waitFor(function () {
>+        return mozmill.utils.getWindows().length == (windowCount - 1);
>+      }, "Download Manager has been closed");

As Anthony mentioned on the other patch for tests please use '===' and '!==' for comparisons. That applies to all other instances of the patch.

>-    this._controller.waitForEval("subject.manager.getDownloadState(subject.download) == subject.state", timeout, 100,
>-                                 {manager: this, download: download, state: state});
>+    this._controller.waitFor(function () {
>+      return this.getDownloadState(download) == state;
>+    }, "Download state has been processed");

This will not work because 'this' inside the callback will not reference the downloadManager class as you would expect, but the callback function itself. Keep an eye out for the 4th parameter of the waitFor method. Also update the message. Currently it tells nothing. It should be 'Expected download state has been set'.

>+    controller.waitFor(function () {
>+      return button.getNode().hasAttribute('disabled') == false;
>+    }, "The OK button has been enabled");

This should be a 'Save' button. Can you please re-check? Also no need to compare to false. Return the boolean value directly.

>   // Remove the pages, then block until we're done or until timeout is reached
>   browserHistory.removeAllPages();
>-  mozmill.controller.waitForEval("subject.state == true", gTimeout, 100, finishedFlag);
>+  var controller = mozmill.getBrowserController();
>+  controller.waitFor(function () {
>+    return finishedFlag.state == true;
>+  }, "History pages have been removed");

Just keep 'mozmill.controller.waitFor()'. We don't need a full controller instance here. Also please update the message to "Browsing History has been cleared".

>-    this._controller.waitForEval("subject.hasAttribute('disabled') == false", gTimeout, 100,
>-                                 this._pbTransitionItem.getNode());
>-    this._controller.waitForEval("subject.privateBrowsing.enabled == subject.state", gTimeout, 100,
>-                                 {privateBrowsing: this, state: state});
>+    var that = this;

First don't use that. If there is a need we have used 'self' so far. But in this case see my comment above. Just use the 4th parameter.

>+    this._controller.waitFor(function () {
>+      return that._pbTransitionItem.getNode().hasAttribute('disabled') == false;
>+    }, "Transition for Private Browsing mode has been made");

Which direction did the transition happen? That would be important for the message.

>+    this._controller.waitFor(function () {
>+      return that.enabled == state;
>+    }, "Private Browsing state has been changed. Expected: " + state);

The state should be enclosed in '' quotes.

>-    this._controller.waitForEval("subject.manager.selectedIndex == subject.newIndex", TIMEOUT, 100,
>-                                 {manager: this, newIndex: index});
>+    this._controller.waitFor(function () {
>+      return this.selectedIndex == index.newIndex;
>+    }, "Search engine with index " + index + " has been selected");

Please put the index value at the end under expected value.

>-    this._controller.waitForEval("subject.manager.selectedEngine != subject.removedEngine", TIMEOUT, 100,
>-                                 {manager: this, removedEngine: name});
>+    var that = this;
>+    this._controller.waitFor(function () {
>+      return that.selectedEngine != name;
>+    }, name + " has been removed");

Please the same message as for the other changes in this file. Also always put variables at the end under expected.

>+      }, "Search engines drop down open state has beeen changed");

nit: Remove one 'e'.

>+      this._controller.waitFor(function () {
>+        return self.opened == true;
>+      }, "A link has been opened in a new tab");

nit: Remove the trailing 'A'.

>+      this._controller.waitFor(function () {
>+        return self.opened == true;
>+      }, "A new tab has been opened");

Same here.

Otherwise it looks good and will be a kinda big improvement beside the fact that the eval methods are marked as deprecated. Thanks Remus! Looking forward to the updated patch.

Comment 4

[Remus Pop (:RemusPop)]

Attachment: patch v2 (all branches)

Updated the patch as requested.

Comment 5

[Remus Pop (:RemusPop)]

Attachment: patch v2 (all branches)

Sorry about this. Found a problem and updated the patch.

Comment 6

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

Comment on attachment 586962 [details] [diff] [review]
patch v2 (all branches)

>+    this._controller.waitFor(function () {
>+      return this.enabled === state;
>+    }, "Private Browsing state has been changed. Expected: " + "'" + state + "'", undefined, undefined, this);

Nit: No need to put the first "'" into a separate string.

Given that you get r=me. But please fix this nit.

Have you run tests against all platforms for the default branch? If yes can you please execute Mozmill by saving of the reports to the local disk (mozmill --report=file://filename) and attach those on this bug.

If everything is fine we can land it on default and once everything is green also on the older branches. Thanks for the work!!

Comment 7

[Remus Pop (:RemusPop)]

Attachment: patch v3 (all branches) [checked-in]

Removed all the <" + "'" +> occurrences.

Comment 8

[Remus Pop (:RemusPop)]

Attachment: Testrun report for Linux

Comment 9

[Remus Pop (:RemusPop)]

Attachment: Testrun report for MacOS

Comment 10

[Remus Pop (:RemusPop)]

Attachment: Testrun report for Win XP

I kind of not trust this results because I got different results and strange fails when reruning the testrun.

I ran this using mozmill-env.

Comment 11

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

Comment on attachment 587693 [details] [diff] [review]
patch v3 (all branches) [checked-in]

Looks great. Thanks Remus.

Comment 12

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

Landed on default for now as:
http://hg.mozilla.org/qa/mozmill-tests/rev/11bd60d82b0f

I will check tomorrows results on qa-horus and its VMs and push it to older branches once everthing is green.

Comment 13

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

Landed for other branches except 1.9.2 where it doesn't apply cleanly.

http://hg.mozilla.org/qa/mozmill-tests/rev/ad1efa87beec (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/574e084805be (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/ec79f731ef17 (release)

Remus, can you update the patch for 1.9.2 please?

Comment 14

[Remus Pop (:RemusPop)]

Attachment: patch v1 (1.9.2)

This follows the same error messages as the patch for all branches and the guidelines in the reviews.
I used == instead of === where the latter would not work.

Comment 15

[Alex Lakatos[:AlexLakatos]]

(In reply to Remus Pop (:RemusPop) from comment #14)
> Created attachment 589174 [details] [diff] [review]
> patch v1 (1.9.2)
> 
> This follows the same error messages as the patch for all branches and the
> guidelines in the reviews.
> I used == instead of === where the latter would not work.
== checks the value where as === checks for both type and value. Do we really need === here?

Comment 16

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

We want triple operators in any case.

Comment 17

[Maniac Vlad Florin (:vladmaniac)]

This bug is not fixed - it still needs the 1.9.2 branch - please be attentive about closing bugs - reopening

Comment 18

[Alex Lakatos[:AlexLakatos]]

Comment on attachment 589174 [details] [diff] [review]
patch v1 (1.9.2)

>+    this._controller.waitFor(function () {
>+      return this.paneId == id;
>+    }, "Pane has been selected. Expected pane id: '" + id + "'", undefined, undefined, this);
>   },
Should'n you use === here?

Comment 19

[Remus Pop (:RemusPop)]

Attachment: patch v2 (1.9.2)

Fixed the request.

Comment 20

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

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #17)
> This bug is not fixed - it still needs the 1.9.2 branch - please be
> attentive about closing bugs - reopening

This is not true. Once a patch has been landed on the default branch our policy is to close a bug. Check-ins for older branches are covered by whiteboard for now. Anthony wanted to setup some tracking flags for older branches. Not sure about its status.

Comment 21

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

Comment on attachment 593084 [details] [diff] [review]
patch v2 (1.9.2)

>-    this._controller.waitForEval("subject.window.paneId == subject.expectedPaneId", gTimeout, 100,
>-                                 {window: this, expectedPaneId: id});
>+    this._controller.waitFor(function () {
>+      return this.paneId[0] === id;

Why do you think this is an array? It does not fit in what we had before the change. Alex has already mentioned it, so you probably forgot to fix?

>-    this._controller.waitForEval("subject.plugin.getPluginState(subject.node, subject.value) == subject.state", gTimeout, 100,
>-                                 {plugin: this, node: node, value: value, state: enable});
>+    this._controller.waitFor(function () {
>+      this.getPluginState(node, value) === enable;

One example why such a change is important. It's very easy to read now. 

>-    this._controller.waitForEval("subject.selector.getAttribute('pane') == subject.dlg.selectedPane.getNode().id", gTimeout, 100,
>-                                 {selector: selector.getNode().selectedItem, dlg: this});
>+    this._controller.waitFor(function () {
>+      return selector.getNode().selectedItem.getAttribute('pane') === this.selectedPane.getNode().id;
>+    }, "Pane has been changed. Expected id: '" + this.selectedPane.getNode().id + "'", undefined, undefined, this);

Please trim the lines so that we keep with 80 chars as best as possible. Same applies to some other changes.

>@@ -355,18 +356,19 @@ locationBar.prototype = {
>-    this._controller.waitForEval("subject.getAttribute('focused') == 'true'",
>-                                 gTimeout, 100, this.urlbar.getNode());

While we have removed all usages of gTimeout can you please check if the declaration has also been dropped? Just do a search across the whole repository. Not sure if we have taken care of for the new branches. If there are instances to remove please file a new bug for it.

With the above fixes it looks good to be checked-in. Remus, can you please come up with a new patch today?

Comment 22

[Maniac Vlad Florin (:vladmaniac)]

> 
> This is not true. Once a patch has been landed on the default branch our
> policy is to close a bug. Check-ins for older branches are covered by
> whiteboard for now. Anthony wanted to setup some tracking flags for older
> branches. Not sure about its status.

Ok. Thanks

Comment 23

[Remus Pop (:RemusPop)]

(In reply to Henrik Skupin (:whimboo) from comment #21)
> Comment on attachment 593084 [details] [diff] [review]
> patch v2 (1.9.2)
> 
> >-    this._controller.waitForEval("subject.window.paneId == subject.expectedPaneId", gTimeout, 100,
> >-                                 {window: this, expectedPaneId: id});
> >+    this._controller.waitFor(function () {
> >+      return this.paneId[0] === id;
> 
> Why do you think this is an array? It does not fit in what we had before the
> change. Alex has already mentioned it, so you probably forgot to fix?

We have:
  return /\w+/.exec(selected.getNode().id); 
in the getter of the paneId.
regExp.exec returns an array. Do you suggest other solution here?

Regarding gTimeout, a new bug would be more suitable to remove its instances where this would be suitable because waitThenClick or waitForElement still use the constant.

Comment 24

[Alex Lakatos[:AlexLakatos]]

(In reply to Remus Pop (:RemusPop) from comment #23)
> We have:
>   return /\w+/.exec(selected.getNode().id); 
> in the getter of the paneId.
> regExp.exec returns an array. Do you suggest other solution here?
Just make that return /\w+/.exec(selected.getNode().id)[0];

Comment 25

[Remus Pop (:RemusPop)]

That means to also change the patch for tests in bug 592275

Comment 26

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

The getter method paneid is intended to return an atomic value - in this case a string - and not an array. See the doc content. As you point out this is not the case and has to be fixed. Please also check the other branches if they are also affected.

Comment 27

[Remus Pop (:RemusPop)]

Attachment: patch v3 (1.9.2)

Fixed all requests.

Comment 28

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

Comment on attachment 593825 [details] [diff] [review]
patch v3 (1.9.2)

>+    return mozmill.wm.getMostRecentWindow('').document.documentElement.id
>+           === 'unknownContentType';

If a line is too long please use a temporary variable but please don't push the operator and second operand into the next line. This applies to a couple of instances.

Otherwise it looks good now. Please do the quick update and we can check it in.

Comment 29

[Remus Pop (:RemusPop)]

Attachment: patch v4 (1.9.2)

Added variables for those broken lines.

Comment 30

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

Comment on attachment 594661 [details] [diff] [review]
patch v4 (1.9.2)

>-  controller.waitForEval("subject.getMostRecentWindow('').document.documentElement.id == 'unknownContentType'",
>-                         gTimeout, 100, mozmill.wm);
>+  controller.waitFor(function () {
>+    mostRecentWindow = mozmill.wm.getMostRecentWindow('').document.documentElement.id;
>+    return mostRecentWindow === 'unknownContentType';
>+  }, "Unknown content type dialog has been opened");

The declaration of variables always require the var or let keyword. Otherwise they will end-up in the global scope. Also in this case you do not store the 'mostRecentWindow' but the 'windowId'. Keep in mind that you can also store the document and retrieve the sub-properties in the next line, so that we get balanced lines. 

>+    this._controller.waitFor(function () {
>+      currentPaneId = selector.getNode().selectedItem.getAttribute('pane')
>+      return currentPaneId === this.selectedPane.getNode().id;

'var' comment also applies here.

Comment 31

[Remus Pop (:RemusPop)]

Attachment: patch v5 (1.9.2)

Fixed.

Comment 32

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

Comment on attachment 594682 [details] [diff] [review]
patch v5 (1.9.2)

>+  controller.waitFor(function () {
>+    var mostRecentWindow = mozmill.wm.getMostRecentWindow('');
>+    var mostRecentWindowId = mostRecentWindow.document.documentElement.id;
>+    return mostRecentWindowId === 'unknownContentType';

That's not what I meant in my last review, Remus. There is no need to create even more variables. Lets use 'var id = mozmill.wm.getMostRecentWindow('').document.documentElement.id'. We have a small block of code where we really can condense the length of the declared variable.

With this r=me.

Comment 33

[Remus Pop (:RemusPop)]

Attachment: patch v6 (1.9.2) [checked-in]

Fixed it. Hope it's fine now.
I did that in order to be super clear about stuff.

Comment 34

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

Comment on attachment 594691 [details] [diff] [review]
patch v6 (1.9.2) [checked-in]

Thanks Remus! That's great to get checked in. I will do it right away.

Comment 35

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/62aa5a8a44fd

We can call this bug done now. Wohhoooo.

Comment 36

[u279076]

Thanks for getting this done guys. Remus, please check tomorrow's 1.9.2 testrun to verify we have not introduced any regressions.

Comment 37

[Remus Pop (:RemusPop)]

There is 1 failure, in testSearchSuggestion which is covered by the refactoring.
Link http://mozmill-release.blargon7.com/#/functional/failure?branch=3.6&platform=All&from=2012-02-06&to=2012-02-07&test=%2FtestSearch%2FtestSearchSuggestions.js&func=testSearchSuggestions.js%3A%3AtestMultipleEngines
Not sure what happens here as I cannot reproduce the issue.

Comment 38

[u279076]

(In reply to Remus Pop (:RemusPop) from comment #37)
> There is 1 failure, in testSearchSuggestion which is covered by the
> refactoring.

Let's deal with this in a new bug. Remus, can you file it?

Marking this bug VERIFIED FIXED.

Comment 39

[Remus Pop (:RemusPop)]

Sure Anthony. Filed bug 727474.

Comment 40

[u279076]

> patch v3 (all branches) [checked-in]

This patch should be ported to mozilla-esr10 as well so all branches are the same.

Comment 41

[Remus Pop (:RemusPop)]

The patch is already ported to the esr10 branch. There are no waitForEval calls in shared-modules.

Comment 42

[u279076]

Actually, you are right, this was checked-in before the merge. Thanks.