808586 - Add waitForEvents() and waitForEvent() to our shared modules

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, enhancement)

Description

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

Similar to the code we already have in Mozmill 1.5 but which doesn't get exposed to the tests, we need a waitForEvents() and waitForEvent() method to make it easier for everyone to wait for specific events.

What we have so far is:
https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L100

We can't make use of it 1-1 and have to refactor the code, but it should be a good start.

Once it is implemented we can proof it's stability and get it hopefully into Mozmill 2.0.

Comment 1

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

As decided on IRC it would be best to add a new module for events under /lib. The assertions module can then make use of the methods and pass-through parameters.

Comment 2

[Bob Silverberg [:bsilverberg]]

Attachment: Adds waitForEvent and waitForEvents to /lib/events.js

This includes the implementation of and tests for waitForEvent and waitForEvents in /lib/events.js.  I thought it might make sense to get these in shape first, and then add the passthroughs into assertions.js later.

Comment 3

[Bob Silverberg [:bsilverberg]]

Attachment: Adds waitForEvent and waitForEvents to /lib/events.js

Oops, the previous patch contained an extra file.

Comment 4

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

Comment on attachment 678812 [details] [diff] [review]
Adds waitForEvent and waitForEvents to /lib/events.js

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

Looks good! A few relatively minor comments.

::: lib/events.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * @fileoverview
> + * The EventsAPI The ToolbarAPI adds support for waiting for events to occur.

Remove 'The ToolbarAPI'

@@ +23,5 @@
> + * @param {Number} interval
> + *        The interval between evaluation attempts.
> + * @returns {Boolean} Did all the events fire?
> + */
> +function waitForEvents(container, events, aCallback, timeout, interval) {

Should we name these arguments aContainer, aEvents, aTimeout, aInterval as we have in assertions.js? This is more a question for Henrik. It would be my preference to keep this consistency.

@@ +25,5 @@
> + * @returns {Boolean} Did all the events fire?
> + */
> +function waitForEvents(container, events, aCallback, timeout, interval) {
> +  mySelf = this;
> +  timeout = timeout || 5000;

We should be able to omit the timeout and interval and rely on the defaults set in mozmill.utils.waitFor.

::: lib/tests/testEvents.js
@@ +15,5 @@
> +function testEventsClass() {
> +
> +  eventContainer = controller.window;
> +  eventNames = ["TabOpen"];
> +  eventCallback = function () {controller.mainMenu.click("#menu_newNavigatorTab");}

I would maybe use keyboard shortcuts instead of mouse clicks as they should be more likely to pass.

@@ +23,5 @@
> +  expect.ok(itWorked, "Expected the TabOpen event to fire.");
> +
> +  // waitForEvents test for listening for a single event that does not fire
> +  itWorked = events.waitForEvents(eventContainer, eventNames, function () {});
> +  expect.notEqual(itWorked, true, "Did not expect the TabOpen event to fire.");

You should use expect.ok(!itWorked, "The TabOpen event did not fire");

@@ +25,5 @@
> +  // waitForEvents test for listening for a single event that does not fire
> +  itWorked = events.waitForEvents(eventContainer, eventNames, function () {});
> +  expect.notEqual(itWorked, true, "Did not expect the TabOpen event to fire.");
> +
> +  // waitForEvents test for listening for a two events that do fire

'a two' -> 'two'

@@ +34,5 @@
> +  }
> +  itWorked = events.waitForEvents(eventContainer, eventNames, eventCallback);
> +  expect.ok(itWorked, "Expected the TabOpen and TabClose events to fire.");
> +
> +  // waitForEvents test for listening for a two events when only one fires

As above: 'a two' -> 'two'

@@ +40,5 @@
> +  eventCallback = function () {
> +    controller.mainMenu.click("#menu_newNavigatorTab");
> +  }
> +  itWorked = events.waitForEvents(eventContainer, eventNames, eventCallback);
> +  expect.notEqual(itWorked, true, "Did not expect both events to fire.");

As above: expect.ok(!itWorked...

@@ +42,5 @@
> +  }
> +  itWorked = events.waitForEvents(eventContainer, eventNames, eventCallback);
> +  expect.notEqual(itWorked, true, "Did not expect both events to fire.");
> +
> +  // waitForEvent test for listening for an event that does fire

I would suggest moving this up, and using eventName[0] to save resetting eventName and eventCallback.

@@ +52,5 @@
> +  expect.ok(itWorked, "Expected the TabOpen event to fire.");
> +
> +  // waitForEvent test for listening for an event that does not fire
> +  itWorked = events.waitForEvent(eventContainer, eventName, function () {});
> +  expect.notEqual(itWorked, true, "Did not expect the TabOpen event to fire.");

As above: expect.ok(!itWorked...

Comment 5

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

(In reply to Dave Hunt (:davehunt) from comment #4)
> > + * @fileoverview
> > + * The EventsAPI The ToolbarAPI adds support for waiting for events to occur.
> 
> Remove 'The ToolbarAPI'

We should also remove 'waiting' because the module will contain stuff for all sort of event related activities.

> Should we name these arguments aContainer, aEvents, aTimeout, aInterval as
> we have in assertions.js? This is more a question for Henrik. It would be my
> preference to keep this consistency.
 
Yes please.

> > +function waitForEvents(container, events, aCallback, timeout, interval) {
> > +  mySelf = this;
> > +  timeout = timeout || 5000;
> 
> We should be able to omit the timeout and interval and rely on the defaults
> set in mozmill.utils.waitFor.

And don't forget to add thisObject as last parameter. Do we need a custom message? Hm, lots of parameters. :( But I still would prefer that way instead of having a real Event object as given by the original code in Mozmill. Dave, what's your opinion on?

I haven't checked the code yet, but will do once it has been updated.

Comment 6

[Bob Silverberg [:bsilverberg]]

Attachment: Adds waitForEvent and waitForEvents to /lib/events.js

Comment 7

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

Comment on attachment 679128 [details] [diff] [review]
Adds waitForEvent and waitForEvents to /lib/events.js

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

This looks good to me, however leaving the review request for Henrik to comment.

::: lib/tests/testEvents.js
@@ +27,5 @@
> +  expect.ok(!itWorked, "The TabOpen event did not fire");
> +
> +  eventName = "TabOpen";
> +  // waitForEvent test for listening for an event that does fire
> +  itWorked = events.waitForEvent(eventContainer, eventName, eventCallback);

As mentioned, you could also use eventNames[0] here instead of creating a new variable.

Comment 8

[Bob Silverberg [:bsilverberg]]

Attachment: Adds waitForEvent and waitForEvents to /lib/events.js

I missed one of davehunt's comments. The latest attachment addresses that one as well.

Comment 9

[Bob Silverberg [:bsilverberg]]

Attachment: Adds waitForEvent and waitForEvents to /lib/events.js

Problem with the last patch. This one should be ok.

Comment 10

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

Comment on attachment 679157 [details] [diff] [review]
Adds waitForEvent and waitForEvents to /lib/events.js

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

Wow, that looks great. I havent' tested the patch myself yet but will do next time with the review comments addressed.

::: lib/events.js
@@ +22,5 @@
> + *        The timeout in waiting for events to fire.
> + * @param {Number} aInterval
> + *        The interval between evaluation attempts.
> + * @param {Object} aThisObject this object
> + * @returns {Boolean} Did all the events fire?

Types should all start with a lowercase letter.

@@ +24,5 @@
> + *        The interval between evaluation attempts.
> + * @param {Object} aThisObject this object
> + * @returns {Boolean} Did all the events fire?
> + */
> +function waitForEvents(aContainer, aEvents, aCallback, aTimeout, aInterval, aThisObject) {

Given that we will make use of an element as the first parameter I would change it's name to aElement and always ensure that we have an element created by elementslib.*().

@@ +25,5 @@
> + * @param {Object} aThisObject this object
> + * @returns {Boolean} Did all the events fire?
> + */
> +function waitForEvents(aContainer, aEvents, aCallback, aTimeout, aInterval, aThisObject) {
> +  mySelf = this;

Call it simply 'self' and make sure you put a 'var' before. Personally I would move down the declaration to the block where you really need this variable.

@@ +30,5 @@
> +  if (aContainer.getNode != undefined)
> +    aContainer = aContainer.getNode();
> +
> +  this.events = aEvents;
> +  this.container = aContainer;

Why is it necessary to create properties on the function?

@@ +32,5 @@
> +
> +  this.events = aEvents;
> +  this.container = aContainer;
> +  this.firedEvents = {};
> +  this.registry = {};

I would not bind those to the function but make them local instances.

@@ +34,5 @@
> +  this.container = aContainer;
> +  this.firedEvents = {};
> +  this.registry = {};
> +
> +  for each(e in this.events) {

Please never use 'for each' because it will also iterate over properties of an object. Use forEach instead.

@@ +35,5 @@
> +  this.firedEvents = {};
> +  this.registry = {};
> +
> +  for each(e in this.events) {
> +    var listener = function(event) {

nit: blank after function. Can we define this method outside of the loop?

@@ +44,5 @@
> +  }
> +
> +  try {
> +
> +    aCallback();

I would rename that parameter so it describes better what type of callback it is. Also please remove the above extra empty line.

@@ +46,5 @@
> +  try {
> +
> +    aCallback();
> +
> +    mozmill.utils.waitFor(function () {

You want to use the waitFor() method from the assertions module instead.

@@ +50,5 @@
> +    mozmill.utils.waitFor(function () {
> +      for (var e in mySelf.registry) {
> +        if (!mySelf.firedEvents[e])
> +          return false;
> +      }

So with the current implementation we can't detect if the events have been fired in the order as specified. That's something we absolutely have to fix.

@@ +57,5 @@
> +
> +    return true;
> +
> +  } catch (e) {
> +    return false;

We will also have to send a passing or failing frame for this method. See the waitFor implementation.

@@ +60,5 @@
> +  } catch (e) {
> +    return false;
> +  } finally {
> +    for (var e in this.registry) {
> +      this.container.removeEventListener(e, this.registry[e], false);

I would like to see a better name for variables, especially if these are too short. Further use forEach again.

::: lib/tests/testEvents.js
@@ +38,5 @@
> +  eventNames = ["TabOpen", "TabClose"];
> +  eventCallback = function () {
> +    controller.keypress(null, "t", {accelKey: true});
> +    controller.keypress(null, "w", {accelKey: true});
> +  }

I wouldn't redefine it but give it a different name. You can have 2 globally defined methods then.

@@ +45,5 @@
> +
> +  // waitForEvents test for listening for two events when only one fires
> +  eventCallback = function () {
> +    controller.keypress(null, "t", {accelKey: true});
> +  }

that would be not necessary with the above change.

Comment 11

[Bob Silverberg [:bsilverberg]]

(In reply to Henrik Skupin (:whimboo) from comment #10)

I am uploading a new patch which addresses most of these comments.  Some responses of my own inline:

> Comment on attachment 679157 [details] [diff] [review]
> Adds waitForEvent and waitForEvents to /lib/events.js
> 
> Review of attachment 679157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +24,5 @@
> > + *        The interval between evaluation attempts.
> > + * @param {Object} aThisObject this object
> > + * @returns {Boolean} Did all the events fire?
> > + */
> > +function waitForEvents(aContainer, aEvents, aCallback, aTimeout, aInterval, aThisObject) {
> 
> Given that we will make use of an element as the first parameter I would
> change it's name to aElement and always ensure that we have an element
> created by elementslib.*().
>

Are you suggesting that the type of that parameter be {ElemBase}? I tried that initially and had type errors when passing in controller.window, which I use for testing the TabOpen event. Should this method not support passing in objects and only support elements?
 
> 
> @@ +50,5 @@
> > +    mozmill.utils.waitFor(function () {
> > +      for (var e in mySelf.registry) {
> > +        if (!mySelf.firedEvents[e])
> > +          return false;
> > +      }
> 
> So with the current implementation we can't detect if the events have been
> fired in the order as specified. That's something we absolutely have to fix.
>

I will address this in a future patch (coming soon). For now I wanted to address all of the "cosmetic" issues.
 
> @@ +57,5 @@
> > +
> > +    return true;
> > +
> > +  } catch (e) {
> > +    return false;
> 
> We will also have to send a passing or failing frame for this method. See
> the waitFor implementation.
>

I'm not sure I understand what's needed or what the code does, but I did attempt to borrow code from the waitFor implementation for this, which is in the patch. 
 
> @@ +60,5 @@
> > +  } catch (e) {
> > +    return false;
> > +  } finally {
> > +    for (var e in this.registry) {
> > +      this.container.removeEventListener(e, this.registry[e], false);
> 
> I would like to see a better name for variables, especially if these are too
> short. Further use forEach again.
> 

I had trouble getting forEach to work with an object as opposed to an array. As this is just a for / in (and not a for each / in) is it necessary to change this to a forEach?

Comment 12

[Bob Silverberg [:bsilverberg]]

Attachment: Adds waitForEvent and waitForEvents to /lib/events.js

See my comments above which apply to this patch.

Comment 13

[Bob Silverberg [:bsilverberg]]

Attachment: Adds waitForEvent and waitForEvents to /lib/events.js

I forgot to change one set of argument names in the last patch.

Comment 14

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

(In reply to Bob Silverberg from comment #11)
> Are you suggesting that the type of that parameter be {ElemBase}? I tried
> that initially and had type errors when passing in controller.window, which
> I use for testing the TabOpen event. Should this method not support passing
> in objects and only support elements?

Right, in this case you will have to pass 'new elementslib.Elem(controller.window)' as parameter. With MozElement in Mozmill 2.0 it will be easier.

> > @@ +57,5 @@
> > > +
> > > +    return true;
> > > +
> > > +  } catch (e) {
> > > +    return false;
> > 
> > We will also have to send a passing or failing frame for this method. See
> > the waitFor implementation.
> >
> 
> I'm not sure I understand what's needed or what the code does, but I did
> attempt to borrow code from the waitFor implementation for this, which is in
> the patch. 

Forget about that one. It has to be part of the wrappers in the assertions module. 

> I had trouble getting forEach to work with an object as opposed to an array.
> As this is just a for / in (and not a for each / in) is it necessary to
> change this to a forEach?

No, that's right. forEach() will not work with object properties.

Comment 15

[Bob Silverberg [:bsilverberg]]

Attachment: Adds waitForEvent and waitForEvents to /lib/events.js

This addresses all comments above and introduces checking for a specific sequence of events.

Comment 16

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

Comment on attachment 679725 [details] [diff] [review]
Adds waitForEvent and waitForEvents to /lib/events.js

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

Sorry that it has been taken a bit longer to get reviewed. There are some things left which need to be addressed.

::: lib/events.js
@@ +15,5 @@
> +/**
> + * Wait for a set of events to fire on a container
> + *
> + * @param {elemBase} aElement
> + *        An object or node on which events are expected to fire.

This is 'ElemBase'. Reason why I mentioned lower letters in my last review is that typeof() returns e.g. 'string' and not 'String'. But that doesn't affect class names.

@@ +29,5 @@
> + * @returns {boolean} Did all the events fire?
> + */
> +function waitForEvents(aElement, aEvents, aEventFiringCallback, aTimeout, aInterval, aThisObject) {
> +  if (aElement.getNode != undefined)
> +    aElement = aElement.getNode();

This check is not necessary given that any ElemBase element will have a getNode() method. But you have to check that the real node exists.

@@ +38,5 @@
> +  var listener = function (event) {
> +    firedEvents.push(event.type);
> +  }
> +
> +  aEvents.forEach(function (eventName) {

Nits: Please add the 'a' prefix for the parameter and add a comment before the loop what's actually happening here.

@@ +49,5 @@
> +  try {
> +    aEventFiringCallback();
> +
> +    assert.waitFor(function () {
> +      return aEvents.join('|') == firedEvents.join('|');

You should call `aEvents.join('|')` once before the loop and assign to a temporary variable. Also can we join with spaces?

@@ +54,5 @@
> +    }, "All events fired in the specified sequence before timeout.", aTimeout, aInterval, aThisObject);
> +
> +    return true;
> +
> +  } catch (e) {

nit: try to avoid the hanging catch/finally/else and put it into a new line.

@@ +55,5 @@
> +
> +    return true;
> +
> +  } catch (e) {
> +    return false;

Something I'm worried about is that we do not output which events actually have fired. The message for waitFor() doesn't have this information and we can't add it there. So I wonder if we could at least send a frame in case we fail.

@@ +70,5 @@
> + *
> + * @param {elemBase} aElement
> + *        An element on which events are expected to fire.
> + * @param {string} aEvent
> + *        A event names.

nit: 'Event name'

::: lib/tests/testEvents.js
@@ +8,5 @@
> +
> +function setupModule() {
> +  controller = mozmill.getBrowserController();
> +}
> +

Ensure to add a teardownModule() method to close all the opened tabs.

@@ +11,5 @@
> +}
> +
> +function testEventsClass() {
> +
> +  eventContainer = controller.window;

This should be wrapped into an element with elementslib.elem().

@@ +31,5 @@
> +  eventCallbackOpenTabCloseTabOpenTab = function () {
> +    controller.keypress(null, "t", {accelKey: true});
> +    controller.keypress(null, "w", {accelKey: true});
> +    controller.keypress(null, "t", {accelKey: true});
> +  }

nit: mind separating it into blocks? I would use names like 'eventNameOpenAndCloseTab' and put it beside the callback. It also misses 'var' for each declaration.

Comment 17

[Bob Silverberg [:bsilverberg]]

(In reply to Henrik Skupin (:whimboo) from comment #16)

I have made most of the suggested changes in my next patch but am unclear about a few things:

> 
> @@ +29,5 @@
> > + * @returns {boolean} Did all the events fire?
> > + */
> > +function waitForEvents(aElement, aEvents, aEventFiringCallback, aTimeout, aInterval, aThisObject) {
> > +  if (aElement.getNode != undefined)
> > +    aElement = aElement.getNode();
> 
> This check is not necessary given that any ElemBase element will have a
> getNode() method. But you have to check that the real node exists.

I added a test and a throw for this last item, but I'm not sure if I've done what you want. See the patch.

> 
> @@ +54,5 @@
> > +    }, "All events fired in the specified sequence before timeout.", aTimeout, aInterval, aThisObject);
> > +
> > +    return true;
> > +
> > +  } catch (e) {
> 
> nit: try to avoid the hanging catch/finally/else and put it into a new line.
>

I am not sure what you mean here.
 
> @@ +55,5 @@
> > +
> > +    return true;
> > +
> > +  } catch (e) {
> > +    return false;
> 
> Something I'm worried about is that we do not output which events actually
> have fired. The message for waitFor() doesn't have this information and we
> can't add it there. So I wonder if we could at least send a frame in case we
> fail.

I'm not sure how to do what you are suggesting. Can you point me to an example?

> 
> @@ +31,5 @@
> > +  eventCallbackOpenTabCloseTabOpenTab = function () {
> > +    controller.keypress(null, "t", {accelKey: true});
> > +    controller.keypress(null, "w", {accelKey: true});
> > +    controller.keypress(null, "t", {accelKey: true});
> > +  }
> 
> nit: mind separating it into blocks? I would use names like
> 'eventNameOpenAndCloseTab' and put it beside the callback. It also misses
> 'var' for each declaration.

I'm not sure what you mean. Do you mean to create a const object that contains each of the callbacks?

Comment 18

[Bob Silverberg [:bsilverberg]]

Attachment: Adds waitForEvent and waitForEvents to /lib/events.js

This addresses the concerns from the last review, except for the comments I have entered as a reply above.

Comment 19

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

Comment on attachment 681825 [details] [diff] [review]
Adds waitForEvent and waitForEvents to /lib/events.js

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

(In reply to Bob Silverberg from comment #17)
> > > +    }, "All events fired in the specified sequence before timeout.", aTimeout, aInterval, aThisObject);
> > > +
> > > +    return true;
> > > +
> > > +  } catch (e) {
> > 
> > nit: try to avoid the hanging catch/finally/else and put it into a new line.
> 
> I am not sure what you mean here.

It should be:

try {
  ..
}
catch (e) {
  ..
}

> > Something I'm worried about is that we do not output which events actually
> > have fired. The message for waitFor() doesn't have this information and we
> > can't add it there. So I wonder if we could at least send a frame in case we
> > fail.
> 
> I'm not sure how to do what you are suggesting. Can you point me to an
> example?

It's similar to frame.fail() but just call frame.log(). I think that should work. As last resort you can have a look at Mozmill's frame.js module.

> > > +  eventCallbackOpenTabCloseTabOpenTab = function () {
> > > +    controller.keypress(null, "t", {accelKey: true});
> > > +    controller.keypress(null, "w", {accelKey: true});
> > > +    controller.keypress(null, "t", {accelKey: true});
> > > +  }
> > 
> > nit: mind separating it into blocks? I would use names like
> > 'eventNameOpenAndCloseTab' and put it beside the callback. It also misses
> > 'var' for each declaration.
> 
> I'm not sure what you mean. Do you mean to create a const object that
> contains each of the callbacks?

Don't worry about that one. See my other comment about these specific tests.

::: lib/events.js
@@ +30,5 @@
> + */
> +function waitForEvents(aElement, aEvents, aEventFiringCallback, aTimeout, aInterval, aThisObject) {
> +  node = aElement.getNode();
> +  if (node == undefined)
> +    throw new Error("The element passed into waitForEvents does not contain a valid node.");

That looks mostly fine. I just want to propose we do it similar to the code in Mozmill itself:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/controller.js#L663

@@ +35,5 @@
> +
> +  var firedEvents = [];
> +  var observedEvents = {}
> +
> +  var listener = function (event) {

nit: aEvent please.

@@ +42,5 @@
> +
> +  // For each event name passed in, add a listener to the Element for that
> +  // event, but only if a listener for that event has not already been added.
> +  aEvents.forEach(function (aEventName) {
> +    if (!observedEvents.hasOwnProperty(aEventName)) {

I thought a bit about this piece of code and I wonder if we really can do that. What happens if we want to listen for an e.g. click event but the testcase itself already has this event registered? Not sure if we ever thought about that.

@@ +51,5 @@
> +
> +  try {
> +    aEventFiringCallback();
> +
> +    joinedEvents = aEvents.join(' ');

Do not forget to declare with var. Otherwise it will become a global variable which can break the test under special conditions.

::: lib/tests/testEvents.js
@@ +16,5 @@
> +}
> +
> +function testEventsClass() {
> +
> +  eventContainer = new elementslib.Elem(controller.window);

This one I would call 'win' instead.

Similar to the module please use `var` for all declarations including the methods.

@@ +69,5 @@
> +
> +  // waitForEvents test for listening for three events that do fire in specified sequence
> +  itWorked = events.waitForEvents(eventContainer, eventNamesThree, eventCallbackOpenTabCloseTabOpenTab);
> +  expect.ok(!itWorked, "The TabOpen event fired, then the TabClose event fired, then the TabOpen event fired again.");
> +

I wonder if it would have been easier to test with a content example, like a textbox and onfocus, onblur, onchange, and other others. But that's just some thoughts. At least that wouldn't be application specific, and we could easily port this to Mozmill base later.

Comment 20

[Bob Silverberg [:bsilverberg]]

(In reply to Henrik Skupin (:whimboo) from comment #19)
> > 
> > I am not sure what you mean here.
> 
> It should be:
> 
> try {
>   ..
> }
> catch (e) {
>   ..
> }
>

Done.
 
> > > Something I'm worried about is that we do not output which events actually
> > > have fired. The message for waitFor() doesn't have this information and we
> > > can't add it there. So I wonder if we could at least send a frame in case we
> > > fail.
> > 
> > I'm not sure how to do what you are suggesting. Can you point me to an
> > example?
> 
> It's similar to frame.fail() but just call frame.log(). I think that should
> work. As last resort you can have a look at Mozmill's frame.js module.

I have looked at this and while I think I sort of understand what's needed I'm still unsure as to how to implement it. I'm not sure where in the code we could add this log as it's the waitFor timing out that generates the failure. I am also unclear as to what to pass into the log method. We can chat about this the next time we're both on IRC.

> 
> > > > +  eventCallbackOpenTabCloseTabOpenTab = function () {
> > > > +    controller.keypress(null, "t", {accelKey: true});
> > > > +    controller.keypress(null, "w", {accelKey: true});
> > > > +    controller.keypress(null, "t", {accelKey: true});
> > > > +  }
> > > 
> > > nit: mind separating it into blocks? I would use names like
> > > 'eventNameOpenAndCloseTab' and put it beside the callback. It also misses
> > > 'var' for each declaration.
> > 
> > I'm not sure what you mean. Do you mean to create a const object that
> > contains each of the callbacks?
> 
> Don't worry about that one. See my other comment about these specific tests.
> 

Ok. Ignored.

> ::: lib/events.js
> @@ +30,5 @@
> > + */
> > +function waitForEvents(aElement, aEvents, aEventFiringCallback, aTimeout, aInterval, aThisObject) {
> > +  node = aElement.getNode();
> > +  if (node == undefined)
> > +    throw new Error("The element passed into waitForEvents does not contain a valid node.");
> 
> That looks mostly fine. I just want to propose we do it similar to the code
> in Mozmill itself:
> https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/
> resource/driver/controller.js#L663
> 

Done. 

> @@ +35,5 @@
> > +
> > +  var firedEvents = [];
> > +  var observedEvents = {}
> > +
> > +  var listener = function (event) {
> 
> nit: aEvent please.
> 

Done

> @@ +42,5 @@
> > +
> > +  // For each event name passed in, add a listener to the Element for that
> > +  // event, but only if a listener for that event has not already been added.
> > +  aEvents.forEach(function (aEventName) {
> > +    if (!observedEvents.hasOwnProperty(aEventName)) {
> 
> I thought a bit about this piece of code and I wonder if we really can do
> that. What happens if we want to listen for an e.g. click event but the
> testcase itself already has this event registered? Not sure if we ever
> thought about that.
>

I'm not sure that that would be a problem. observedEvents is an object that we are managing ourselves, inside waitForEvents, so it seems like it wouldn't matter whether a dev had already added an observer for the same event in the test. Or maybe I'm not understanding the issue.
 
> @@ +51,5 @@
> > +
> > +  try {
> > +    aEventFiringCallback();
> > +
> > +    joinedEvents = aEvents.join(' ');
> 
> Do not forget to declare with var. Otherwise it will become a global
> variable which can break the test under special conditions.
> 

Done.

> ::: lib/tests/testEvents.js
> @@ +16,5 @@
> > +}
> > +
> > +function testEventsClass() {
> > +
> > +  eventContainer = new elementslib.Elem(controller.window);
> 
> This one I would call 'win' instead.
> 
> Similar to the module please use `var` for all declarations including the
> methods.
> 

Done and done.

> @@ +69,5 @@
> > +
> > +  // waitForEvents test for listening for three events that do fire in specified sequence
> > +  itWorked = events.waitForEvents(eventContainer, eventNamesThree, eventCallbackOpenTabCloseTabOpenTab);
> > +  expect.ok(!itWorked, "The TabOpen event fired, then the TabClose event fired, then the TabOpen event fired again.");
> > +
> 
> I wonder if it would have been easier to test with a content example, like a
> textbox and onfocus, onblur, onchange, and other others. But that's just
> some thoughts. At least that wouldn't be application specific, and we could
> easily port this to Mozmill base later.

Ok, I have taken a stab at this. I await your comments.

Comment 21

[Bob Silverberg [:bsilverberg]]

Attachment: Adds waitForEvent and waitForEvents to /lib/events.js

This patch addresses the last set of review comments as per my reply above.

Comment 22

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

Comment on attachment 684562 [details] [diff] [review]
Adds waitForEvent and waitForEvents to /lib/events.js

Removing myself as a reviewer for this patch, as I haven't worked on this code as closely as Henrik, and think he'd be the best person to review.

Comment 23

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

Comment on attachment 684562 [details] [diff] [review]
Adds waitForEvent and waitForEvents to /lib/events.js

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

Sorry for the delay in review here. But I somehow totally missed this request. :( Lot of things have been changed meanwhile. So feature-wise we might need this module meanwhile, but then it should also support observer notifications.

Bob, I assume you can't continue to work here? We could re-assign it to one of our SV people or find another contributor. Please let me know.

::: lib/events.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

The methods in this file might better end-up in the assertions module, next to the waitFor() methods.

@@ +54,5 @@
> +    aEventFiringCallback();
> +
> +    var joinedEvents = aEvents.join(' ');
> +    assert.waitFor(function () {
> +      return joinedEvents == firedEvents.join(' ');

This would fail if events are coming through in the wrong order, which might happen. I don't think we force a strict order here, so we should have it more flexible.

Comment 24

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

mozmill-tests are no longer used. So this is no longer necessary.