Closed Bug 1495558 Opened 2 years ago Closed 2 years ago

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


(Thunderbird :: Testing Infrastructure, enhancement)

Not set


(Not tracked)

Thunderbird 64.0


(Reporter: aceman, Assigned: aceman)



(1 file, 1 obsolete file)

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=" + +
                    ", 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., 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.
Attached 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:
Attachment #9013410 - Flags: review?(jorgk)
Attachment #9013410 - Flags: feedback?(arai.unmht)
Comment on attachment 9013410 [details] [diff] [review]

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.
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
Make utils.waitFor() accept function to properly retrieve state of objects after failure (idea by Tooru Fujisawa). r=jorgk
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.