Closed Bug 1118012 Opened 6 years ago Closed 6 years ago

waitForEvents has errors

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Taraman, Assigned: Taraman, Mentored)

References

()

Details

(Whiteboard: [lang=js][mozmill-2.0.10])

Attachments

(2 files, 2 obsolete files)

The function waitForEvents.wait() produces 2 errors:
- ex is not defined
- this.node is not defined

The Problem is in [1], which is the copy, Thunderbird uses for its Mozmill.
first error is caused by a typo (ex ISO e)
second error is caused by this.node not being in scope. We need to pass "this" to "waitFor".

The following solves the problem:
- }, "waitForEvents.wait(): Event '" + ex + "' has been fired.", timeout, interval);
+ }, "waitForEvents.wait(): Event '" + e + "' has been fired.", timeout, interval, this);

I have a patch for comm-central ready, but I don't know if this is the right repo to fix it.

[1]: http://mxr.mozilla.org/comm-central/source/mail/test/resources/mozmill/mozmill/extension/resource/modules/controller.js#133
Hi Markus, thank you for the bug report! Actually this method was never officially supported so we have never used it. We should have removed it or marked it as such. If you want to fix that I would totally appreciate a patch. The right repository to get it fixed is https://github.com/mozilla/mozmill. But please keep in mind that any work for the branches 1.5.x and 2.0 are done. If you want to provide a patch make sure its created against the master branch.

If you need this fix for Thunderbird it has to be fixed also on their side, which currently is 1.5.x. If really necessary we could land such a patch on the above mentioned hotfix branches in our repository, but we will not release a new version of those older versions of Mozmill.
Mentor: hskupin
Hardware: x86 → All
Whiteboard: [mozmill 1.5.16] → [lang=js]
Hi Henrik,

I'm experimenting with this function, because I need to make sure a given calendar-view is loaded.
Since it is not supported, I will try to think of a different solution.

Nevertheless, I will create a pull-Request on GitHub for this one.
As said if you can make it work, we can get it landed. But I will not release a new version of Mozmill 1.5. If you have a patch please attach it here on that bug.

It would be great to have this fix, given that we also use events and mostly duplicate our code :( See that library for an example:

http://hg.mozilla.org/qa/mozmill-tests/file/ca5598acd4a9/firefox/lib/tabs.js#l365
Flags: needinfo?(Mozilla)
Attached patch fix_waitForEvents.diff (obsolete) — Splinter Review
So, here is the patch for comm-central I have.
If this looks good, I will go ahead and put the same in github for the current repo.
Are there other places this will have to go?
Flags: needinfo?(Mozilla)
Attachment #8556032 - Flags: feedback?(hskupin)
Comment on attachment 8556032 [details] [diff] [review]
fix_waitForEvents.diff

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

Markus, the patch seems to be incomplete. Or is this really the only necessary change to do? If that is the case I would suggest you create a `self` variable before the waitFor() call and assign `this` to it. Then we wouldn't have to pass in this, which nearly all of the other methods do meanwhile.
Attachment #8556032 - Flags: feedback?(hskupin) → feedback+
It really is the whole patch.

I'll take in your comment and prepare an updated version.
Attached patch fix_waitForEvents.diff V2 (obsolete) — Splinter Review
Updated patch
Attachment #8556032 - Attachment is obsolete: true
Attachment #8557364 - Flags: review?(hskupin)
Comment on attachment 8557364 [details] [diff] [review]
fix_waitForEvents.diff V2

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

::: mail/test/resources/mozmill/mozmill/extension/resource/modules/controller.js
@@ +132,3 @@
>        utils.waitFor(function() {
>          return this.node.firedEvents[e] == true;
> +      }, "Timeout happened before event '" + e +"' was fired.", timeout, interval, self);

What I meant here is that you do not have to pass-in self as last argument to the waitFor call. Simply use `self.node.firedEvents` inside it.
Attachment #8557364 - Flags: review?(hskupin) → review-
Ok, now I got it.
Attachment #8560132 - Flags: review?(hskupin)
Attachment #8557364 - Attachment is obsolete: true
Comment on attachment 8560132 [details] [diff] [review]
fix_waitForEvents.diff V3

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

Sweet! Taken, and will be landed in a moment. Thanks!
Attachment #8560132 - Flags: review?(hskupin) → review+
Comment on attachment 8560132 [details] [diff] [review]
fix_waitForEvents.diff V3

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

Markus, I have seen that you created this patch against the mozmill version in /mail. It's not something I can use to import for Mozmill proper. So please apply your changes to our Mozmill repository on Github for the master branch: https://github.com/mozilla/mozmill.
Attachment #8560132 - Flags: review+ → review-
Henrik, do you want me to prepare a patch to attach or a pull-request directly on github?
For Mozmill we handle patches here on Bugzilla. Use "git format-patch" to export it.
Status: NEW → ASSIGNED
After some tuition on git, here is the patch.
Hope the format is right.
Attachment #8560457 - Flags: review?(hskupin)
Comment on attachment 8560457 [details] [diff] [review]
fix_waitForEvents for git

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

Great. I will get the commit message updated so it applies to our guidelines, and get it landed. Thanks Markus for the patch!
Attachment #8560457 - Flags: review?(hskupin) → review+
Landed as:
https://github.com/mozilla/mozmill/commit/ab080aecdf21d5b5d3a3aa7a7ffd68fbab8b2b82
Blocks: 970594
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [lang=js] → [lang=js][mozmill-2.1]
Small fix which we can actually push out with the upcoming 2.0.10 release of Mozmill.

Cherry-picked for hotfix-2.0 branch as:
https://github.com/mozilla/mozmill/commit/54f5dda9e169e4e1e60639f59650e50562467faf
Blocks: 1135004
No longer blocks: 970594
Whiteboard: [lang=js][mozmill-2.1] → [lang=js][mozmill-2.0.10]
Will this fix find it's way into comm-central somehow?
Not via this release. comm-central is still using mozmill 1.5.x, but once they decide to upgrade they will get it for free. If that should be fixed in comm-central too, you will have to file another bug for it and provide the same patch but only against comm-central.
Blocks: 1137643
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.