1495558 - incorrect use of message argument to utils.waitFor() in Thunderbird's mozmill tests

Status: RESOLVED FIXED

(Thunderbird :: Testing Infrastructure, enhancement)

Description

[:aceman]

The waitFor() function in mail/test/resources/mozmill/mozmill/extension/content/modules/utils.js
waits for something to happen (first argument). If it doesn't within a timeout it aborts the test with a message that was provided to it in the second argument.

Consider a call to the function like this (in mail/test/mozmill/shared-modules/test-window-helpers.js::click_menus_in_sequence):

utils.waitFor(() => aRootPopup.state == "open",
                    "Popup never opened! id=" + aRootPopup.id +
                    ", state=" + aRootPopup.state);

The intent of the message in the second argument seems to be to print the "state=" of the popup after the timeout happens and the state isn't "open". So we want to see what it is. But the message (expression) is evaluated before the waitFor function is entered (this before the wait), so it will actually contain the .state of the popup before the wait (which expectedly is not "open"). It does not say anything about state after wait.
It often shows "state=showing" in case of failure on Linux (e.g. https://queue.taskcluster.net/v1/task/RCcxYd4xQyO-TXI5i-VuvA/runs/0/artifacts/public/logs/live_backing.log), which has often mislead us in the past.

Now when debugging bug 1490639 I found out the state is actually "closed", not "showing", because the menu really closes itself for some reason.

Comment 1

[:aceman]

Attachment: 1495558.patch

This passes the message argument to the waitFor() function as a function so that the expression is evaluated only at the time of failure and shows the correct state of the object when we actually need it.

This solution seems to be the simplest for the task, as suggested by arai.

The patch fixes all the uses of the functions across mozmill tests, where the late evaluation seems to be needed.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c18d3463449efa3eb5e6fef3f6dceff757b7441b

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 9013410 [details] [diff] [review]
1495558.patch

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

And you can add "suggestion by Tooru" to the commit message.

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +978,5 @@
>       *          an empty array if aKeepOpen was set to false.
>       */
>      click_menus_in_sequence: function _click_menus(aRootPopup, aActions, aKeepOpen) {
>        if (aRootPopup.state != "open") { // handle "showing"
> +        utils.waitFor(() => {dump ("ACE:"+aRootPopup.state+"\n"); return aRootPopup.state == "open"},

Please remove this.

Comment 3

[:aceman]

Attachment: 1495558.patch v1.1

Ok

Comment 4

[Jorg K (CEST = GMT+2)]

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

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

Good :-) - Getting more and more perfect.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1d9d688d5669
Make utils.waitFor() accept function to properly retrieve state of objects after failure (idea by Tooru Fujisawa). r=jorgk