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

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

Comment on attachment 585675 [details] [diff] [review] patch v1 (all branches) r+ over to Henrik

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.

Updated the patch as requested.

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

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!!

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

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.

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.

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?

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.

(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.

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

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?

Fixed the request.

(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 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?

> > 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

(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];

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.

Fixed all requests.

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.

Added variables for those broken lines.

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.

Fixed.

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.

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

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.

Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/62aa5a8a44fd We can call this bug done now. Wohhoooo.

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

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.

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.

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

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