With bug 579943 fixed for Mozmill 1.5, all Mozmill tests will have to be updated to use waitFor instead of waitForEval

Move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

This bug blocks us from transition over to Mozmill 2. Vlad or Remus, can one of you please check how many instances of waitForEval we still have in our test code? I believe it has been dropped drastically, but we should finish it up, so we make use of waitFor in any cases. Thanks.

There are 61 occurrences for default and 99 for mozilla-1.9.2 branch. This is for the whole mozmill-tests folder.

We shouldn't mind 1.9.2, but for all current branches it would be great to see the move to waitFor. With that change we would already be able to run tests with Mozmill 2 directly.

Oh, the first part we could start with are the shared modules.

https://www.pivotaltracker.com/story/show/20472259 is the link to the pivotal tracker story.

This is the initial patch for the shared modules. All branches refers to the rapid release branches. It applies cleanly for the branches.

Had to modify the patch for the release branch as it wasn't applying cleanly.

This patch addresses the refactor to waitFor() for tests/ in branches beta, aurora and nightly(default).

Comment on attachment 571954 [details] [diff] [review] patch v1 shared-modules (all branches) >+ var controller = mozmill.getBrowserController(); >+ controller.waitFor(function() { >+ return finishedFlag.state == true; >+ }, "History pages had been removed"); > } > Reword this to "History pages have been removed" >+ var that = this; >+ this._controller.waitFor(function() { >+ return that._pbTransitionItem.getNode().hasAttribute('disabled') == false; >+ }, "Transition into or out of Private Browsing mode has been made"); Reword this to "Transition for Private Browsing mode has been made" >+ var that = this; >+ this._controller.waitFor(function() { >+ return that.selectedEngine != name; >+ }, name + " has been removed"); > }, Maybe we should change this to ""Search engine '" + name + "' has been removed"" >+ this._controller.waitFor(function() { >+ return that.selectedEngine == name >+ }, "Engine " + name + "has been selected. Got " + that.selectedEngine); > }, Change "Engine " with "Search engine " Remus, you have got a few messages in here that use just "textMessage. Got: variableName". I think you should change them to use an "textMessage. Got variableName, expected @param" style. Henrik, what do you think?

Comment on attachment 573770 [details] [diff] [review] patch v1 tests (release) > // Wait until all downloads have been finished >- controller.waitForEval("subject.activeDownloadCount == 0", TIMEOUT, DELAY, dm); >+ controller.waitFor(function() { >+ return dm.activeDownloadCount == 0; >+ }, "All downloads have completed"); Please reword this to "All downloads have been finished" here and anywhere else it appears in your patch. >+ controller.waitFor(function() { >+ return engine.name != searchBar.visibleEngines[1].name; >+ }, engine.name + " has been removed"); > } Use "Search engine " + engine.name + " has been removed" >+ controller.waitFor(function() { >+ return webIDDomainLabel.getNode().value.indexOf(cert.commonName) != -1; >+ }, "Web Site label does not match Certificate Common Name"); I think a "expected - got" style message will help with debugging.

Comment on attachment 573771 [details] [diff] [review] patch v1 tests (beta, aurora, nightly) Seeing as how the only difference between this patch and the one for the release branch are only two line of code, the same review comments apply here.

Addressed all requests.

Same modifications for all patches. Unfortunately, most of the code changes in all branches might affect these patches because they affect a lot of files. There is one separate patch for beta because the other one wasn't applying cleanly. So we will need to hurry up somehow with landing this, otherwise we will need to fix these patches before applying them.

Alex, can you please make sure to reduce the time for reviews on those patches? I agree with Remus regarding the amount of time we have to spend with updating the patches because of other fixes. Thanks.

(In reply to Alex Lakatos from comment #10) > Remus, you have got a few messages in here that use just "textMessage. Got: > variableName". I think you should change them to use an "textMessage. Got > variableName, expected @param" style. Henrik, what do you think? The thing here is that you will never get the final state of the variable you check in the message string. The string gets build before the waitFor method gets executed. That means even with a change of the value it will still be the old value in the message. It will cause more confusion as leaving it out. So I would propose to drop the use of the current variable but use the expected state instead. This information will always be accurate.

Comment on attachment 577878 [details] [diff] [review] patch v2 (release) >+ controller.waitFor(function() { >+ return webIDDomainLabel.getNode().value.indexOf(cert.commonName) != -1; >+ }, "Certificate common name not matching the website label. Got " >+ + cert.commonName + ", expected " + webIDDomainLabel.getNode().value); Remus, the expected and got values have to be swapped here.

Comment on attachment 577879 [details] [diff] [review] patch v2 (beta) Remus, comment 19 applies to this patch as well. Otherwise looks fine.

Comment on attachment 577880 [details] [diff] [review] patch v2 (aurora, default) Remus, comment 19 applies to this patch as well. Otherwise looks fine.

Please keep in mind my comment 18. We are not able to show the current value, so it doesn't make any sense to add it at all. If Anthony want to comment, we should wait for him. Otherwise we should really get this patch finalized and pushed soonish.

I agree with Henrik's comment 22. Let's just report the expected value. By reporting that on error we know we got something other than the expected value. So for example: "Certificate common name not matching the website label. Got <cert.commonName>, expected <webIDDomainLabel.getNode().value>" - would be better as - "Certificate common name should be <webIDDomainLabel.getNode().value>"

I would propose to also file a new bug so we can investigate and implement a better way for waitFor which would also be able to print out the current value.

So to be clear... This bug will be to refactor waitForEval() in tests to waitFor(). The new bug will be to improve waitFor() to provide current value. Correct?

Right. The new bug should go into shared modules with the [module-refactor] whiteboard entry set.

Remus, can we get an update regarding this patch?

(In reply to Henrik Skupin (:whimboo) from comment #27) > Remus, can we get an update regarding this patch? Remus has PTO - will probably address this in Q1. If P1, I will take over

Early next year is ok, we only have to speed-up here because it's time invasive for everyone of us to keep the patch up-to-date and reviewed.

Bug 714802 has been created for handling the patch for shared modules. This patch addresses all requests and applies cleanly on all branches.

Comment on attachment 585715 [details] [diff] [review] patch v3 (all branches) I have no objections r+ Over to henrik for sr

Comment on attachment 585715 [details] [diff] [review] patch v3 (all branches) This is a patch for tests, reviewer should be me. Let's not over-burden Henrik with reviews out of scope of the Automation Services team.

Comment on attachment 585715 [details] [diff] [review] patch v3 (all branches) >--- a/tests/functional/testAwesomeBar/testFaviconInAutocomplete.js >- controller.waitForEval('subject.isOpened == true', 3000, 100, locationBar.autoCompleteResults); >+ controller.waitFor(function () { >+ return locationBar.autoCompleteResults.isOpened == true; >+ }, "Autocomplete list has been opened"); You shouldn't need to use '== true' for booleans; just return the variable for true, !variable for false. Same goes for any other waitFor() in this patch. >+++ b/tests/functional/testDownloading/testCloseDownloadManager.js >- controller.waitForEval("subject.getWindows().length == " + windowCount, >- gTimeout, 100, mozmill.utils); >+ controller.waitFor(function () { >+ return mozmill.utils.getWindows().length == windowCount; >+ }, "The Download Manager has been closed"); Make sure to use '===' instead of '==' in your evaluations. Same goes for any other waitFor() in this patch. Please make those changes so I can re-review.

Addressed all requests.

Sorry about this. Forgot about a small nit.

Comment on attachment 586043 [details] [diff] [review] patch v4 (all branches) >+++ b/tests/functional/testAwesomeBar/testSuggestHistoryBookmarks.js >+ controller.waitFor(function () { >+ return controller.window.top.StarUI._overlayLoaded; >+ }, "Overlay for the StarUI has been loaded"); Is this referring to the "Edit this Bookmark" doorhanger? If so, lets reword this message. Same in the following: >+++ b/tests/functional/testBookmarks/testAddBookmarkToMenu.js >+++ b/tests/functional/testSecurity/testGreenLarry.js >+ controller.waitFor(function () { >+ return webIDDomainLabel.getNode().value.indexOf(cert.commonName) !== -1; >+ }, "Certificate common name should be " + cert.commonName); "Found certificate common name 'cert.commonName'" >+++ b/tests/functional/testTabbedBrowsing/testCloseTab.js >- controller.waitForEval("subject.length == 5", gTimeout, 100, tabBrowser); >+ controller.waitFor(function () { >+ return tabBrowser.length === 5; >+ }, "5 tabs have been opened - got: " + tabBrowser.length); "5 tabs have been opened" >- controller.waitForEval("subject.length == 4", gTimeout, 100, tabBrowser); >+ controller.waitFor(function () { >+ return tabBrowser.length === 4; >+ }, "Tab has been closed using shortcut. Expected - 4 - got: " + tabBrowser.length); "One tab has been closed via keyboard shortcut" >- controller.waitForEval("subject.length == 3", gTimeout, 100, tabBrowser); >+ controller.waitFor(function () { >+ return tabBrowser.length === 3; >+ }, "Tab has been closed via File menu. Expected - 3 - got: " + tabBrowser.length); "One tab has been closed via File menu" >- controller.waitForEval("subject.length == 2", gTimeout, 100, tabBrowser); >+ controller.waitFor(function () { >+ return tabBrowser.length === 2; >+ }, "Tab has been closed using middle click. Expected - 2 - got: " + tabBrowser.length); "One tab has been closed via middle click" >- controller.waitForEval("subject.length == 1", gTimeout, 100, tabBrowser); >+ controller.waitFor(function () { >+ return tabBrowser.length === 1; >+ }, "Tab has been closed using the close button. Expected - 1 - got: " + tabBrowser.length); "One tab has been closed using the close button" >--- a/tests/functional/testTabbedBrowsing/testOpenInBackground.js > // Check that i+1 tabs are open and the first tab is selected >- controller.waitForEval("subject.length == " + (i + 2), TIMEOUT, 100, tabBrowser); >- controller.waitForEval("subject.selectedIndex == 0", TIMEOUT, 100, tabBrowser); >+ controller.waitFor(function () { >+ return tabBrowser.length === (i + 2); >+ }, i + 2 + " tabs have been opened - got: " + tabBrowser.length); "i+2 tabs have been opened" > // Verify that the last tab is selected: >- controller.waitForEval("subject.length == 3", TIMEOUT, 100, tabBrowser); >- controller.waitForEval("subject.selectedIndex == 2", TIMEOUT, 100, tabBrowser); >+ controller.waitFor(function () { >+ return tabBrowser.length === 3; >+ }, "Tab has been closed using the close button. Expected - 3 - got: " + tabBrowser.length); "A tab has been closed via the close button" >+++ b/tests/functional/testTabbedBrowsing/testOpenInForeground.js >+ controller.waitFor(function () { >+ return tabBrowser.length === (i + 2); >+ }, (i + 2) + " tabs have been opened - got: " + tabBrowser.length); "i+2 tabs have been opened" >+ controller.waitFor(function () { >+ return tabBrowser.length === 3; >+ }, "3 tabs have been opened - got: " + tabBrowser.length); "3 tabs have been opened" Also, this is completely up to you as it's not completely within scope of this bug, but I think all discrete numbers here should be referenced as CONSTANTS where-ever possible.

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #36) > Also, this is completely up to you as it's not completely within scope of > this bug, but I think all discrete numbers here should be referenced as > CONSTANTS where-ever possible. That should be handled separately and not hold of this patch to land. A new bug can be filed if you are eager to get this fixed before we transition to the new API.

Good point, Henrik. Filed bug 716685 to take care of that. Remus, feel free to assign to yourself or hand off to one of your other team mates.

Patch contains all requested changes. Constants will be treated in the separate bug.

Comment on attachment 587295 [details] [diff] [review] patch v5 (all branches) >+++ b/tests/functional/testSecurity/testGreenLarry.js >+ }, "Found certificate common name " + "'" + cert.commonName + "'"); You can simplify this by removing one of the concatenations: common name '" + cert.commonName + "'" >+++ b/tests/functional/testSecurity/testIdentityPopupOpenClose.js >+ }, "Identity popup has been opened"); > controller.sleep(gDelay); Separate with a newline. >+++ b/tests/functional/testTabbedBrowsing/testOpenInBackground.js >+ }, i + 2 + " tabs have been opened"); >+ controller.waitFor(function () { Separate with a newline. >+ }, "A tab has been closed via the close button"); >+ controller.waitFor(function () { Separate with a newline. >+++ b/tests/functional/testTabbedBrowsing/testOpenInForeground.js >+ }, (i + 2) + " tabs have been opened"); >+ controller.waitFor(function () { Separate with a newline. >+ }, "3 tabs have been opened"); >+ controller.waitFor(function () { Separate with a newline. Assuming these changes are made, the patch is code-correct. Considering the broadness of this patch, can you please confirm it passes a full testrun using Mozmill Crowd on all platforms with Nightly?

Updated the patch as requested. I can confirm this works on all platforms using nightly.

Do we also need to refactor the tests on 1.9.2 branch?

(In reply to Remus Pop (:RemusPop) from comment #42) > Do we also need to refactor the tests on 1.9.2 branch? Yes, same as for bug 714802.

We might get into trouble when using === or !== instead of == or !=. For example we might have a variable that has it's data stored as string and we compare it with a numeral using ===. So "2" would not equal 2 because their data types are different. What we want for sure is comparison by value. So do we need comparison by object type too?

No, it is what we want. We always know which datatype a variable has.

Remus, can you please retest this patch. I was unable to check this in due to the brasstacks outage. Now that we have a temporary dashboard I would like to get it checked in. However, since it's been a couple of weeks, we should ensure it has not bitrotted.

Asking r from Alex because Vlad is on PTO.

Comment on attachment 591468 [details] [diff] [review] patch v1 (1.9.2) >diff --git a/tests/addons/toolbar@google.com/lib/testSearchBoxAPI.js b/tests/addons/toolbar@google.com/lib/testSearchBoxAPI.js >--- a/tests/addons/toolbar@google.com/lib/testSearchBoxAPI.js >+++ b/tests/addons/toolbar@google.com/lib/testSearchBoxAPI.js Should tests/addons be updated as well? > // Wait that the Installation pane is selected after the extension has been installed >- addonsManager.controller.waitForEval("subject.manager.paneId == 'installs'", TIMEOUT, 100, >- {manager: addonsManager}); >+ addonsManager.controller.waitFor(function () { >+ return addonsManager.paneId[0] === 'installs'; >+ }, "Installation pane has been selected"); Why are we using paneId[0] here, and everywhere else in the patch? >- addonsManager.controller.waitForEval("subject.manager.paneId == 'extensions'", 1000, 100, >- {manager: addonsManager}); >+ addonsManager.controller.waitFor(function () { >+ return addonsManager.paneId[0] === 'extensions'; >+ }, "Extensions pane has been selected"); The default timeout for waitFor is 5000, not 1000. So you should specify it as a parameter here and anywhere in the patch where is different from 5000. > // editBookmarksPanel is loaded lazily. Wait until overlay for StarUI has been loaded, then close the dialog >- controller.waitForEval("subject._overlayLoaded == true", TIMEOUT, 200, controller.window.top.StarUI); >+ controller.waitFor(function () { >+ return controller.window.top.StarUI._overlayLoaded; >+ }, "Edit This Bookmark doorhanger has been loaded"); Use "'Edit This Bookmark' doorhanger has been loaded" to avoid confusion, here and anywhere in the patch, where this string is used. >- controller.waitForEval("subject.getWindows().length == " + windowCount, >- gTimeout, 100, mozmill.utils); >+ controller.waitFor(function () { >+ return mozmill.utils.getWindows().length === windowCount; >+ }, "The Download Manager has been closed via the Esc button"); ESC is a key, not a button. Change the message accordingly. >+ controller.waitFor(function () { >+ return controller.window.getComputedStyle(animateBox.getNode(), null).opacity != 0; >+ }, "The 'List all tabs' button has been lit up"); Make that !== >+ controller.waitFor(function () { >+ return controller.window.getComputedStyle(animateBox.getNode(), null).opacity == 0; >+ }, "The 'List all tabs' button has faded"); Make that === >- controller.waitForEval("subject.state == 'closed'", gTimeout, 100, allTabsPopup.getNode()); >+ controller.waitFor(function () { >+ return allTabsPopup.getNode().state === 'closed'; >+ }, "The all tabs popup has been closed"); Reword the message, or use '' to avoid confusion.

This is actually a diff between current changes and the ones in patch v1 (1.9.2) so it's easier to track the changes that were requested. When this gets r+ it will be merged with patch v1 (1.9.2). >+ addonsManager.controller.waitFor(function () { >+ return addonsManager.paneId[0] === 'installs'; >+ }, "Installation pane has been selected"); > Why are we using paneId[0] here, and everywhere else in the patch? The paneId getter returns an array so we would get a type mismatch.

Comment on attachment 593074 [details] [diff] [review] patch v1 - diff (1.9.2) You can go ahead and merge the two patches.

I'm confused with the patch naming. Which is valid for which branches? * patch v6 (all branches) * patch v1 (1.9.2) * patch v1 - diff (1.9.2)

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #51) > I'm confused with the patch naming. Which is valid for which branches? > > * patch v6 (all branches) > * patch v1 (1.9.2) > * patch v1 - diff (1.9.2) patch v1 - diff (1.9.2) contains changes that Alex requested after reviewing patch v1 (1.9.2). I made this diff so Alex could review the changes that he requested easier. So now that I have an r+ I will merge those 2 patches into what will be patch v2 (1.9.2)

This is a merge of patch v1 (1.9.2) and patch v1 - diff (1.9.2).

As of the needed change stated in bug 714802 comment 26 I have updated the patch. So first we would need to get the patch on bug 714802 for 1.9.2 landed.

Shared modules have now been landed. I will now review these patches.

Comment on attachment 587653 [details] [diff] [review] patch v6 (all branches) Patch looks fine. Landing on default.

Comment on attachment 587653 [details] [diff] [review] patch v6 (all branches) Patch fails to apply for: tests/functional/testBookmarks/testAddBookmarkToMenu.js Please update the patch and resubmit.

Fixed by removing some code which also changed in all branches.

Comment on attachment 596957 [details] [diff] [review] patch v7 (all) [landed] Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/6bc78b688de1 (default)

Comment on attachment 593829 [details] [diff] [review] patch v3 (1.9.2) Patch fails to apply for: tests/functional/restartTests/testMultipleExtensionInstallation/test2.js

I remember having problems constantly applying the patch. Nothing is modified in the code so I have no idea why this fails. I rewrote the same code and now applies cleanly, but I don't know for how long.

Sorry, forgot to add some code.

Comment on attachment 597373 [details] [diff] [review] patch v3 (1.9.2) Overall the patch looks good. Just one nit: can you please give it one more pass and make sure long lines are being wrapped? Thanks

Trimmed some lines and added some variables to break the lines into max 80 character lines.

Please note that I changed only the lines which were changed by me, although longer lines still exist in the code.

Comment on attachment 599580 [details] [diff] [review] patch v4 (1.9.2) [landed] Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/febaf02d97bd (mozilla-1.9.2)

Comment on attachment 596957 [details] [diff] [review] patch v7 (all) [landed] Missed landing the default patch across other branches. Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/acff2eef2402 (mozilla-aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/9132453f4af9 (mozilla-beta) http://hg.mozilla.org/qa/mozmill-tests/rev/ac85d28dd0dc (mozilla-release) http://hg.mozilla.org/qa/mozmill-tests/rev/9344eb9a34a9 (mozilla-esr10)

Remus, please do me a favour and confirm this has not regressed by pulling down the latest of each branch and running the entire testrun against each latest build: * Firefox 11b4 * Firefox 10.0.2 * Firefox 10.0.2ESR * Firefox 12.0a2 * Firefox 13.0a1 * Firefox 3.6.27 Thanks

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #67) > Comment on attachment 596957 [details] [diff] [review] > patch v7 (all) [landed] > > http://hg.mozilla.org/qa/mozmill-tests/rev/9344eb9a34a9 (mozilla-esr10) Actually, I had to back this changeset out because I just realized the shared modules have not yet been ported. Please do this first so I can re-land this patch (see bug 714802). Backout: http://hg.mozilla.org/qa/mozmill-tests/rev/81f8180a519b (mozilla-esr10)

There is a difference between esr10 and release branch so 1 waitForEval call remains in esr10. The reason to this is that the rest of the rapid release branches contain Vlad's waitForPanel in shared-modules along with it's call in testBookmarks/testAddToBookmarksMenu. So we have 2 options here: * copy waitForPanel in esr10 shared-modules and update the test accordingly * refactor waitForEval into a waitFor call I tend to go with the first option so we're consistent, but I would like to hear your opinion too.

(In reply to Remus Pop (:RemusPop) from comment #70) > * copy waitForPanel in esr10 shared-modules and update the test accordingly It's a new feature so we will not merge it to mozilla-esr10. > * refactor waitForEval into a waitFor call Simply update the patch so it applies cleanly on esr10 and removes all waitForEval calls. Thanks.

1 waitForEval call refactored in testBookmarks/testAddBookmarkToMenu.js and removed the TIMEOUT constant.

(In reply to Henrik Skupin (:whimboo) from comment #71) > (In reply to Remus Pop (:RemusPop) from comment #70) > > * copy waitForPanel in esr10 shared-modules and update the test accordingly > > It's a new feature so we will not merge it to mozilla-esr10. Wait, I'm confused. I would think we want all active branches to be updated so they all work with Mozmill-2.0. So right now, all branches are updated, only esr10 remains. Why would we not want this for ESR? I understand not landing new tests on ESR, but this is fixing existing code, not introducing new code.

Not sure why you are confused Anthony. Please see the patch from Remus which already includes the necessary update to kill all waitForEval calls on the esr branch. I have never said that we don't want to do that. So not sure where you get this impression from. Also waitFor is not a new feature, but the waitForPanel enhancements from Vlad which I was referring to is and that one we do not want to have on the esr branch. I hope that clarifies everything and you can successfully review the remaining patch.

Ah, sorry I missed that -- I see the difference now. Thanks for clarification Henrik.

Comment on attachment 601959 [details] [diff] [review] patch v1 (esr10) Patch fails to apply for testPreferences/testPreferredLanguage.js

Must have been something that has landed before so I've updated the patch.

Comment on attachment 604345 [details] [diff] [review] patch v2 (esr10) [landed] Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/ee31e2f27e36 (mozilla-esr10)

I believe that wraps up this bug. Will need to wait for 10.0.3esr#2 to be built so we can verify.

YAY! Thank you all a ton. Lets do the same for assertJS now.