Update Mozmill shared-modules to use waitFor instead of waitForEval

VERIFIED FIXED

Status

VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: remus.pop, Assigned: remus.pop)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lib][mozmill-test-refactor])

Attachments

(5 attachments, 8 obsolete attachments)

19.83 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
60.52 KB, text/plain
Details
67.19 KB, text/plain
Details
41.79 KB, text/plain
Details
25.07 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
To move to Mozmill 2.0, waitforEval calls will have to be changed to waitFor.
(Assignee)

Comment 1

7 years ago
Created attachment 585675 [details] [diff] [review]
patch v1 (all branches)

This is an updated patch from bug 592275 for shared-modules. It applies cleanly on all branches.
Attachment #585675 - Flags: review?(vlad.mozbugs)
Comment on attachment 585675 [details] [diff] [review]
patch v1 (all branches)

r+ over to Henrik
Attachment #585675 - Flags: review?(vlad.mozbugs)
Attachment #585675 - Flags: review?(hskupin)
Attachment #585675 - Flags: review+
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.
Attachment #585675 - Flags: review?(hskupin) → review-
(Assignee)

Comment 4

7 years ago
Created attachment 586953 [details] [diff] [review]
patch v2 (all branches)

Updated the patch as requested.
Attachment #585675 - Attachment is obsolete: true
Attachment #586953 - Flags: review?(hskupin)
(Assignee)

Comment 5

7 years ago
Created attachment 586962 [details] [diff] [review]
patch v2 (all branches)

Sorry about this. Found a problem and updated the patch.
Attachment #586953 - Attachment is obsolete: true
Attachment #586953 - Flags: review?(hskupin)
Attachment #586962 - Flags: review?(hskupin)
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!!
Attachment #586962 - Flags: review?(hskupin) → review+
(Assignee)

Comment 7

7 years ago
Created attachment 587693 [details] [diff] [review]
patch v3 (all branches) [checked-in]

Removed all the <" + "'" +> occurrences.
Attachment #586962 - Attachment is obsolete: true
Attachment #587693 - Flags: review?(hskupin)
(Assignee)

Comment 8

7 years ago
Created attachment 587695 [details]
Testrun report for Linux
(Assignee)

Updated

7 years ago
Attachment #587695 - Attachment description: Report for Linux → Testrun report for Linux
(Assignee)

Comment 9

7 years ago
Created attachment 587696 [details]
Testrun report for MacOS
(Assignee)

Comment 10

7 years ago
Created attachment 587697 [details]
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 on attachment 587693 [details] [diff] [review]
patch v3 (all branches) [checked-in]

Looks great. Thanks Remus.
Attachment #587693 - Flags: review?(hskupin) → review+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing on older branches]
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?
Whiteboard: [needs landing on older branches] → [needs patch for 1.9.2 branch]
(Assignee)

Comment 14

7 years ago
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.
Attachment #589174 - Flags: review?(alex.lakatos)
(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?
We want triple operators in any case.
Blocks: 692775
This bug is not fixed - it still needs the 1.9.2 branch - please be attentive about closing bugs - reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
Attachment #589174 - Flags: review?(alex.lakatos) → review-
(Assignee)

Comment 19

7 years ago
Created attachment 593084 [details] [diff] [review]
patch v2 (1.9.2)

Fixed the request.
Attachment #589174 - Attachment is obsolete: true
Attachment #593084 - Flags: review?(alex.lakatos)
Attachment #593084 - Flags: review?(hskupin)
Attachment #593084 - Flags: review?(alex.lakatos)
Attachment #593084 - Flags: review+
(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.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
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?
Attachment #593084 - Flags: review?(hskupin) → review-
> 
> 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
(Assignee)

Comment 23

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

Comment 25

7 years ago
That means to also change the patch for tests in bug 592275
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.
(Assignee)

Comment 27

7 years ago
Created attachment 593825 [details] [diff] [review]
patch v3 (1.9.2)

Fixed all requests.
Attachment #593084 - Attachment is obsolete: true
Attachment #593825 - Flags: review?(hskupin)
(Assignee)

Updated

7 years ago
Blocks: 592275
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.
Attachment #593825 - Flags: review?(hskupin) → review+
(Assignee)

Comment 29

7 years ago
Created attachment 594661 [details] [diff] [review]
patch v4 (1.9.2)

Added variables for those broken lines.
Attachment #593825 - Attachment is obsolete: true
Attachment #594661 - Flags: review?(hskupin)
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.
Attachment #594661 - Flags: review?(hskupin) → review-
(Assignee)

Comment 31

7 years ago
Created attachment 594682 [details] [diff] [review]
patch v5 (1.9.2)

Fixed.
Attachment #594661 - Attachment is obsolete: true
Attachment #594682 - Flags: review?(hskupin)
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.
Attachment #594682 - Flags: review?(hskupin) → review-
(Assignee)

Comment 33

7 years ago
Created attachment 594691 [details] [diff] [review]
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.
Attachment #594682 - Attachment is obsolete: true
Attachment #594691 - Flags: review?(hskupin)
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.
Attachment #594691 - Flags: review?(hskupin) → review+
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/62aa5a8a44fd

We can call this bug done now. Wohhoooo.
Whiteboard: [needs patch for 1.9.2 branch]
Whiteboard: [mozmill-refactor]
Attachment #587693 - Attachment description: patch v3 (all branches) → patch v3 (all branches) [checked-in]
Attachment #594691 - Attachment description: patch v6 (1.9.2) → patch v6 (1.9.2) [checked-in]
Thanks for getting this done guys. Remus, please check tomorrow's 1.9.2 testrun to verify we have not introduced any regressions.
Whiteboard: [mozmill-refactor] → [mozmill-test-refactor]
(Assignee)

Comment 37

7 years ago
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.
(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.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 39

7 years ago
Sure Anthony. Filed bug 727474.
> patch v3 (all branches) [checked-in]

This patch should be ported to mozilla-esr10 as well so all branches are the same.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 41

7 years ago
The patch is already ported to the esr10 branch. There are no waitForEval calls in shared-modules.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Actually, you are right, this was checked-in before the merge. Thanks.
Status: RESOLVED → VERIFIED
Blocks: 744007
Component: Mozmill Shared Modules → Mozmill Tests
Whiteboard: [mozmill-test-refactor] → [lib]
Whiteboard: [lib] → [lib][mozmill-test-refactor]
You need to log in before you can comment on or make changes to this bug.