831714 - Replace mozmill.utils.waitFor with assert/expect.waitFor in remaining Mozmill tests

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P3)

Description

[Daniela Petrovici]

This is a follow-up bug based on bug 793092 where we have replaced all instances of controller.waitFor with assert/expect.waitFor. 

There are remaining instances of mozmill.utils.waitFor in the existing Mozmill tests which should also be replaced.

For instance on default branch:
a) /lib
b) /tests/addons/[e-mail]/lib/selenium.js

The instances should be replaced with assert/expect.waitFor.

Comment 1

[Daniela Petrovici]

Attachment: patch v1.0

Changed mozmill.utils.waitFor with assert/expect.waitFor. Reports for Windows:
- Addons: http://mozmill-crowd.blargon7.com/#/addons/report/72e15dc943833b8fcba70aeb51370d76
- Functional: http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb5137537e and http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51377fd9
- Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/72e15dc943833b8fcba70aeb5137a8fa

Comment 2

[Daniela Petrovici]

Attachment: patch v1.1

Forgot to add the bug no. in the commit message for the patch

Comment 3

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 705830 [details] [diff] [review]
patch v1.1

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

Here are my notes, but I would like Henrik or Dave's opinion on using assert/expect as well.

::: lib/addons.js
@@ +192,4 @@
>      var spec = aSpec || { };
>      var timeout = (spec.timeout == undefined) ? TIMEOUT : spec.timeout;
>  
> +    assert.waitFor(function() {

Need space here between function and '('

@@ +264,4 @@
>        this._controller.click(button);
>  
>        // Click the button and wait until menu has been opened
> +      assert.waitFor(function() {

Same here.

@@ +279,4 @@
>        // Make sure the menu has been closed
>        this._controller.keypress(menu, "VK_ESCAPE", {});
>  
> +      expect.waitFor(function() {

Same here.

@@ +1524,2 @@
>     return !!addonInfo;
>   });

Please add a message here.

::: lib/places.js
@@ +8,4 @@
>   * has to be used e.g. for alert boxes and other commonDialog instances.
>   */
>  
> + // Include required modules

No need for the whitespace at the beginning.

@@ +110,4 @@
>      BookmarkHTMLUtils.importFromURL(BOOKMARKS_RESOURCE, true);
>  
>      // Wait for it to be finished
> +    expect.waitFor(function () {

I think assert will be more appropriate here, if we fail it's from inside the observer where we change the flag so we do not successfully import the default bookmarks.

@@ +139,4 @@
>  
>      // Remove the pages, then block until we're done or until timeout is reached
>      browserHistory.removeAllPages();
> +    expect.waitFor(function () {

Same here.

Comment 4

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

Comment on attachment 705830 [details] [diff] [review]
patch v1.1

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

::: lib/addons.js
@@ +283,2 @@
>          return menu.getNode() && menu.getNode().state === "closed";
>        }, "Menu of utils button has been closed.");

This should be an assert IMO. If not we will have a focus problem here.

@@ +957,3 @@
>        return this.selectedSearchFilter.getNode() === filter.getNode();
>      }, "Search filter '" + filter.getNode().value + "' has been set", timeout,
>         undefined, this);

I think this should also be an assert because following operations will depend on the correct filter.

@@ +1492,2 @@
>      return finished;
>    }, "Addon " + aAddonId + " has been enabled");

Same here. It's an explicit call to disable the addon. When the method returns it should be clear it is disabled or not.

::: lib/places.js
@@ +8,4 @@
>   * has to be used e.g. for alert boxes and other commonDialog instances.
>   */
>  
> + // Include required modules

Hm, which white-space?

@@ +110,4 @@
>      BookmarkHTMLUtils.importFromURL(BOOKMARKS_RESOURCE, true);
>  
>      // Wait for it to be finished
> +    expect.waitFor(function () {

Agree for using assert here.

@@ +139,4 @@
>  
>      // Remove the pages, then block until we're done or until timeout is reached
>      browserHistory.removeAllPages();
> +    expect.waitFor(function () {

Yes

::: lib/toolbars.js
@@ +261,4 @@
>      var spec = aSpec || { };
>      var timeout = (spec.timeout == undefined) ? TIMEOUT : spec.timeout;
>  
> +    assert.waitFor(function() {

nit: please add the space after function

::: tests/addons/[e-mail]/lib/selenium.js
@@ +75,4 @@
>     * Wait for the Selenium IDE window to be closed
>     */
>    waitForClosed : function SeleniumManager_waitForClosed() {
> +    expect.waitFor(function () {

This has to be an assert.

Comment 5

[Daniela Petrovici]

Attachment: patch v1.2

Performed changes based on reviews.

Comment 6

[Daniela Petrovici]

Forgot to add the reports for the clean run:
Addons: http://mozmill-crowd.blargon7.com/#/addons/report/72e15dc943833b8fcba70aeb51d7e19c
Functional: http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51d8166d
http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51d83560
Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/72e15dc943833b8fcba70aeb51d8a65e

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 707628 [details] [diff] [review]
patch v1.2

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/8d2428a3cc14 (default)

Please check your username as I had to fix this in the patch before pushing. In the original patch the username is 'sername=dpetrovici'.

Comment 8

[Daniela Petrovici]

Attachment: patch v1.0 for Aurora branch

Patch for Aurora. It does not apply clearly on other branches. Reports:
http://mozmill-crowd.blargon7.com/#/addons/report/72e15dc943833b8fcba70aeb51e7680f - Addons
Functional:
http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51e7a68f
http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51e7c542
Endurance:
http://mozmill-crowd.blargon7.com/#/endurance/report/72e15dc943833b8fcba70aeb51e83fd8

Comment 9

[Daniela Petrovici]

Attachment: patch v1.0 for Beta and Release branches

patch for Beta branch that applies clearly on release branch too.

Reports:
Addons: http://mozmill-crowd.blargon7.com/#/addons/report/72e15dc943833b8fcba70aeb51e8a2ac
Functional: http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51e8e482
http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51e9015e
Endurance:
http://mozmill-crowd.blargon7.com/#/endurance/report/72e15dc943833b8fcba70aeb51e9fd2f

Release branch - reports:
Functional:
http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51f270c6
http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51f1ed5a
Endurance:
http://mozmill-crowd.blargon7.com/#/endurance/report/72e15dc943833b8fcba70aeb51ef4e41
Addons:
http://mozmill-crowd.blargon7.com/#/addons/report/72e15dc943833b8fcba70aeb51f2d77d

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 708102 [details] [diff] [review]
patch v1.0 for Aurora branch

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

As mentioned when landing the initial patch, please fix your username. It is currently 'sername=dpetrovici'

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 708117 [details] [diff] [review]
patch v1.0 for Beta and Release branches

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

Same issue with username.

Comment 12

[Daniela Petrovici]

Attachment: patch v1.1 for Aurora branch

fixed username Aurora patch

Comment 13

[Daniela Petrovici]

Attachment: patch v1.1 for Beta and Release branches

fixed issues with Beta patch that applies on Release branch too

Comment 14

[Daniela Petrovici]

Attachment: patch v1.0 for ESR17

Patch for ESR17. Reports are:
Addons:
http://mozmill-crowd.blargon7.com/#/addons/report/72e15dc943833b8fcba70aeb51f2faf4
Functional:
http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51f351e4
http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51f33fb1
Endurance:
http://mozmill-crowd.blargon7.com/#/endurance/report/72e15dc943833b8fcba70aeb51f3c548

Comment 15

[Daniela Petrovici]

Comment on attachment 708129 [details] [diff] [review]
patch v1.1 for Beta and Release branches

Marked the wrong patch as obsolete

Comment 16

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 708132 [details] [diff] [review]
patch v1.0 for ESR17

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

::: tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js
@@ +44,5 @@
>        var windowId = mozmill.utils.getWindowId(controller.window);
>  
>        controller.window.close();
>  
> +      assert.utils.waitFor(function () {

This should be assert.waitFor()

::: tests/functional/testPrivateBrowsing/testCloseWindow.js
@@ +55,5 @@
>    // One single window will be opened in PB mode which has to be closed now
>    var cmdKey = utils.getEntity(tabBrowser.getDtds(), "closeCmd.key");
>    controller.keypress(null, cmdKey, {accelKey: true});
>  
> +  assert.utils.waitFor(function () {

This should be assert.waitFor, is skipped on Linux, that is why your report passed.

Comment 17

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 708127 [details] [diff] [review]
patch v1.1 for Aurora branch

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

Still applying cleanly, but also does with the original one now.

Comment 18

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 708129 [details] [diff] [review]
patch v1.1 for Beta and Release branches

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

::: tests/functional/testPrivateBrowsing/testCloseWindow.js
@@ +55,5 @@
>    // One single window will be opened in PB mode which has to be closed now
>    var cmdKey = utils.getEntity(tabBrowser.getDtds(), "closeCmd.key");
>    controller.keypress(null, cmdKey, {accelKey: true});
>  
> +  assert.utils.waitFor(function () {

Same here as for the ESR17 patch. Needs assert.waitFor()

Comment 19

[Daniela Petrovici]

Attachment: patch v1.1 for ESR17 branch

Fixed the issues with assert.utils.waitFor. Reports for MAC are:
Functional:
http://mozmill-crowd.blargon7.com/#/functional/report/0dbaa964aec88a2f5637c68c964607da
http://mozmill-crowd.blargon7.com/#/functional/report/0dbaa964aec88a2f5637c68c964624fb
Addons: http://mozmill-crowd.blargon7.com/#/addons/report/0dbaa964aec88a2f5637c68c9645657a
Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/0dbaa964aec88a2f5637c68c96463177

Comment 20

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 708999 [details] [diff] [review]
patch v1.1 for ESR17 branch

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

Looks good now.

Comment 21

[Daniela Petrovici]

Attachment: patch v1.2 for Beta and Release branches

Fixed issue with assert.utils.waitFor. Reports are here:
Beta:
Addons:http://mozmill-crowd.blargon7.com/#/addons/report/0dbaa964aec88a2f5637c68c96463ea3
Functional:
http://mozmill-crowd.blargon7.com/#/functional/report/0dbaa964aec88a2f5637c68c964645c6
http://mozmill-crowd.blargon7.com/#/functional/report/0dbaa964aec88a2f5637c68c9646752e
Endurance:
http://mozmill-crowd.blargon7.com/#/endurance/report/0dbaa964aec88a2f5637c68c964691b2

Release:
Endurance:
http://mozmill-crowd.blargon7.com/#/endurance/report/0dbaa964aec88a2f5637c68c96471ff5
Functional:
http://mozmill-crowd.blargon7.com/#/functional/report/0dbaa964aec88a2f5637c68c9646e535
http://mozmill-crowd.blargon7.com/#/functional/report/0dbaa964aec88a2f5637c68c9646d3cc
Addons:
http://mozmill-crowd.blargon7.com/#/addons/report/0dbaa964aec88a2f5637c68c9646a506

Comment 22

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 709021 [details] [diff] [review]
patch v1.2 for Beta and Release branches

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

Thanks Daniela.

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/5d4a0603a3ea (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/42f83b721a71 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/09b1af23d450 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/4b5c5dac3092 (mozilla-esr17)

Comment 24

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

(In reply to Dave Hunt (:davehunt) from comment #23)
> Landed as:
> http://hg.mozilla.org/qa/mozmill-tests/rev/5d4a0603a3ea (mozilla-aurora)
> http://hg.mozilla.org/qa/mozmill-tests/rev/42f83b721a71 (mozilla-beta)
> http://hg.mozilla.org/qa/mozmill-tests/rev/09b1af23d450 (mozilla-release)
> http://hg.mozilla.org/qa/mozmill-tests/rev/4b5c5dac3092 (mozilla-esr17)

Why hasn't this been landed on default first? We do not backport patches before we have proven it didn't regress anything on default. Was it by accident?

Thankfully it was staying green given that we had to run tests for 18.0.2 (release) and the next 19.0 beta today.

Comment 25

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

Oh wait. My fault. It actually went in on Jan 29th. Sorry. I will adjust all the flags now.