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

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 64.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch 1495558.patch (obsolete) — Splinter Review
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
Attachment #9013410 - Flags: review?(jorgk)
Attachment #9013410 - Flags: feedback?(arai.unmht)
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.
Ok
Attachment #9013410 - Attachment is obsolete: true
Attachment #9013410 - Flags: review?(jorgk)
Attachment #9013410 - Flags: feedback?(arai.unmht)
Attachment #9013426 - Flags: review?(jorgk)
Comment on attachment 9013426 [details] [diff] [review]
1495558.patch v1.1

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

Good :-) - Getting more and more perfect.
Attachment #9013426 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.