Closed Bug 583604 Opened 10 years ago Closed 10 years ago

Synthesizing events doesn't return any information if event has been successfully processed

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Whiteboard: [mozmill-1.5.1+][mozmill-doc-needed])

Attachments

(1 file, 8 obsolete files)

This is a kinda bad behavior because failures we are seeing in our tests are not the ones we would have to fix. In case of the safe browsing pages it is not possible to click on elements in the first 200ms. There is no response that the event hasn't been processed instead we get a failure while waiting for the result of the synthesize event.

Since a couple of versions back EventUtils.js supports the synthesizeMouseExpectEvent and synthesizeKeyExpectEvent, which allow us to check if the synthesized event has really happened. That shouldn't be that hard to enable for Mozmill 1.4.2. Would be perfect to slip this in after the next beta or even before. I will have a patch ready hopefully soon.
Attached patch WIP v1 (obsolete) — Splinter Review
Work in progress. Clint, it would be great to get some feedback from you. Right now I only pass in an expectedEvent value but no expectedTarget. The latter one should probably also done once we have to operate on trees and have to check for a specific cell. What do you think? For such a case I would simply wrap the function parameter into an object like {event: "focus", target: searchbar}. Further I have removed the dependency to the events.js function, which doesn't make sense. As in other areas we are too smart here in automatically focusing elements. We should really do this manually.

Still to do:
* Update controller.type to use EventUtils directly.
* Pass in expectedTarget element (if undefined use the first parameter)
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attachment #461946 - Flags: feedback?(ctalbert)
Attachment #461946 - Flags: feedback?(harthur)
Attached patch WIP v1.1 (obsolete) — Splinter Review
Gna, that was a diff against the wrong branch. Here the full patch.
Attachment #461946 - Attachment is obsolete: true
Attachment #461947 - Flags: feedback?(ctalbert)
Attachment #461946 - Flags: feedback?(harthur)
Attachment #461946 - Flags: feedback?(ctalbert)
Comment on attachment 461947 [details] [diff] [review]
WIP v1.1

In general, this looks like a good idea and a good approach. I like the way it gets rid of a bunch of repeated code.  But, it's too invasive of a change for 1.4.2 if we're really going to ship 1.4.2 in the near term.

I don't think we have enough benefit from this change to counterbalance the risk.
Attachment #461947 - Flags: feedback?(ctalbert) → feedback+
(In reply to comment #3)
> In general, this looks like a good idea and a good approach. I like the way it
> gets rid of a bunch of repeated code.  But, it's too invasive of a change for
> 1.4.2 if we're really going to ship 1.4.2 in the near term.

Do you mean the removal of how the focus is set, is the invasive change? That's the only thing I could think about. All other stuff is only removing duplicated code. The new feature to check the results for synthesized events is optional and no existing test will be affected by this patch.

I can revert the focus handling as it was before and we can target that part for Mozmill 2.0. In that way there is really no risk.
We'll take this but only if we can get the tests passing.  Currently 3 tests fail (on linux) and the checked in test fails on linux.
Whiteboard: [mozmill-1.4.2?] → [mozmill-1.4.2+]
Attached patch Patch v1 (obsolete) — Splinter Review
Ok, I have finalized the patch with the following changes:

1. I don't have that much time to completely remove the dependency of the triggerKeyEvent call and the combined check for focus. That should be really something for the next big refactoring which will be part of Mozmill 2.0.

2. I have created a new controller.mouseEvent function as a central member for mouse events. That will give us the flexibility to simulate any kind of event. Users aren't limited to our existent functions, i.e. simulating a triple-click.

3. As an addition the expectedEvent parameter has been added to all synthesize class members. It's optional and current tests, which do not use this parameter, are not affected. If it is specified we check the type and target members to forward it to EventUtils.

4. It comes with some tests. I will still have to figure out why the one case fails on Linux. But IMO I simply check for the wrong target event.

Clint, would be great if we can already start the review process, to have it land soon. An updated test will be delivered as soon as possible.
Attachment #461947 - Attachment is obsolete: true
Attachment #462777 - Flags: review?(ctalbert)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Silly mistake. isNan() is not a function. It should be isNaN().
Attachment #462777 - Attachment is obsolete: true
Attachment #462786 - Flags: review?(ctalbert)
Attachment #462777 - Flags: review?(ctalbert)
Attached patch Patch v1.2 (obsolete) — Splinter Review
This patch now completely fixes the other silly mistakes from the last version. I have also added some more tests to make sure everything works as expected.

The failure on Linux for the test I have supplied is simply that no select event is raised when hitting Ctrl+A, which is suspicious. Instead a keypress event is created.
Attachment #462786 - Attachment is obsolete: true
Attachment #462931 - Flags: review?(ctalbert)
Attachment #462786 - Flags: review?(ctalbert)
I've wrestled with this patch some more, and it is still failing more tests with the patch than without it.  I really don't think this is something we want to take for 1.4.2.  I'm going to minus it, as I think it is just too risky to take at this point.
Whiteboard: [mozmill-1.4.2+] → [mozmill-1.4.2-]
(In reply to comment #9)
> I've wrestled with this patch some more, and it is still failing more tests
> with the patch than without it.  I really don't think this is something we want
> to take for 1.4.2.  I'm going to minus it, as I think it is just too risky to
> take at this point.

I have spent a lot of time in the last days on that patch to make sure no existing test is affected by the changes introduced with this patch. Compared to the first WIP I have removed any code, which has been changed the behavior of the API. Only the (1,1)->(2,2) mouse event click position update, which I have taken over from the Mochitests, is left. The new expected event parameter is completely optional and doesn't affect any test.

Clint, it would have been great if we could have the same way of discussion as what you had with Andrew and Jeff in the office in the last days. I feel sad about the current and quick decision without any ongoing conversation and real background to focus on. Also I haven't seen any test-run report from you for the latest patch. So I can't say anything why you are getting more failures with this patch applied. Could be an issue on your machine only. Please give me that information so we have data to work with. Thanks. 

The new possibilities introduced with this patch would be a big step forward for our Mozmill test work and also the time we can safe for investigating test failures.
(In reply to comment #10)

> Clint, it would have been great if we could have the same way of discussion as
> what you had with Andrew and Jeff in the office in the last days. I feel sad
> about the current and quick decision without any ongoing conversation and real
> background to focus on. Also I haven't seen any test-run report from you for
> the latest patch. So I can't say anything why you are getting more failures
> with this patch applied. Could be an issue on your machine only. Please give me
> that information so we have data to work with. Thanks. 
There hasn't been any other discussion in the office.  It's all been in email and bugs.  We need to ship 1.4.2.  We need to ship it now.

I told you when we +'d this change that I didn't want it, and it would be held to a much higher standard than anything else because it is changing the core mozmill event generation code, which we all know is fragile.  Jeff reproduced this error, and even changing your code and working with it I still can't decipher what is going on here; I have no idea why this is failing (you're not doing anything wrong) and so I think we need to take this after 1.4.2 so that we have more time for debugging and for making this more robust code.
> 
> The new possibilities introduced with this patch would be a big step forward
> for our Mozmill test work and also the time we can safe for investigating test
> failures.
I understand that, but I also can't have it destabilizing the event generation system for all other Mozmill users in the meantime.  It will have to wait until the next dot release.
Whiteboard: [mozmill-1.4.2-] → [mozmill-1.4.2-][mozmill-1.4.3?]
Blocks: 551101
Whiteboard: [mozmill-1.4.2-][mozmill-1.4.3?] → [mozmill-1.4.2-][mozmill-1.5.1?]
Whiteboard: [mozmill-1.4.2-][mozmill-1.5.1?] → [mozmill-1.4.2-][mozmill-1.5.1+]
Comment on attachment 462931 [details] [diff] [review]
Patch v1.2

>diff --git a/mozmill/mozmill/extension/resource/modules/controller.js b/mozmill/mozmill/extension/resource/modules/controller.js
>index 3a520ba..6f3d21f 100644
>@@ -168,28 +168,28 @@ var MozMillController = function (window) {
> 
> MozMillController.prototype.sleep = utils.sleep;
> 
>@@ -213,199 +213,134 @@ MozMillController.prototype.open = function(url)
> 
>-MozMillController.prototype.click = function(elem, left, top)
>-{
>-  var element = elem.getNode();
>-  if (!element){
>-    throw new Error("could not find element " + elem.getInfo());
>-    return false;
>-  }
>+/**
>+ * Synthesize a general mouse event on the given element
>+ */
>+MozMillController.prototype.mouseEvent = function(aTarget, aOffsetX, aOffsetY,
>+                                                  aEvent, aExpectedEvent) {
> 
>-  if (element.tagName == "menuitem") {
>-    element.click();
>-    return true;
>+  var element = aTarget.getNode();
>+  if (!element) {
>+    throw new Error(arguments.callee.name + ": could not find element " + elem.getInfo());
This will fail, there is no longer an elem in this function.  You should use aTarget ^ instead.

>+  if (aExpectedEvent && aExpectedEvent.event) {
>+    var target = aExpectedEvent.target ? aExpectedEvent.target.getNode(): element;
>+    EventUtils.synthesizeMouseExpectEvent(element, aOffsetX, aOffsetY, aEvent,
>+      target, aExpectedEvent.event, "controller.mouseEvent()",
>+      element.ownerDocument.defaultView
We need to be sure that we document the structure of the exepcted event object.  This code is making a lot of assumptions about how that object is put together. You have two methods of failure here.  If the aExpectedEvent doesn't exist, then we will default to the old way of using synthesizeMouse, which is correct.  If the aExpectedEvent object is not properly formatted (i.e. it exists but (aExepected.event) is false, then we will fall back to using the old method as well.  Is that what you want?  Or should we throw an error in that case?  I tend to think we should throw an error telling the test writer that their exepectedEvent is not properly constructed because otherwise, this will constitute a silent failure.

This comment applies to everywhere you do this in this patch.

> MozMillController.prototype.rightclick = function(){
>-  frame.log({function:'rightclick - Deprecation Warning', message:'Controller.rightclick should be renamed to Controller.rightClick'});
>+  frame.log({function:'rightclick - Deprecation Warning',
>+            message:'controller.rightclick should be renamed to Controller.rightClick'});
>   this.rightClick.apply(this, arguments);
For future reference, I would really prefer code cleanup things like this be in their own patches.  It makes it so much easier to review.  

>diff --git a/mozmill/mozmill/extension/resource/stdlib/EventUtils.js b/mozmill/mozmill/extension/resource/stdlib/EventUtils.js
>index 0329d47..6267e41 100644
>--- a/mozmill/mozmill/extension/resource/stdlib/EventUtils.js
>+++ b/mozmill/mozmill/extension/resource/stdlib/EventUtils.js
>@@ -1,15 +1,13 @@
> // Export all available functions for Mozmill
> var EXPORTED_SYMBOLS = ["sendMouseEvent", "sendChar", "sendString", "sendKey",
>-                         "__doEventDispatch", "_parseModifiers", "synthesizeMouse",
>-                         "synthesizeMouseScroll", "synthesizeKey", "_expectEvent",
>-                         "_checkExpectedEvent", "synthesizeMouseExpectEvent",
>-                         "synthesizeDragStart", "synthesizeDrop",
>-                         "disableNonTestMouseEvents", "_getDOMWindowUtils",
>-                         "synthesizeComposition", "synthesizeText",
>-                         "synthesizeQuerySelectedText", "synthesizeQueryTextContent",
>-                         "synthesizeQueryCaretRect", "synthesizeQueryTextRect",
>-                         "synthesizeQueryEditorRect", "synthesizeCharAtPoint",
>-                         "synthesizeSelectionSet"];
>+                        "synthesizeMouse", "synthesizeMouseScroll", "synthesizeKey",
>+                        "synthesizeMouseExpectEvent", "synthesizeKeyExpectEvent",
>+                        "synthesizeDragStart", "synthesizeDrop", "synthesizeText",
>+                        "disableNonTestMouseEvents", "synthesizeComposition", 
>+                        "synthesizeQuerySelectedText", "synthesizeQueryTextContent",
>+                        "synthesizeQueryCaretRect", "synthesizeQueryTextRect",
>+                        "synthesizeQueryEditorRect", "synthesizeCharAtPoint",
>+                        "synthesizeSelectionSet"];
Why are you removing exports from this code? Is this just another cleanup step?

So, I ran this again on master, and it passes more tests than it failed.  That's a little disconcerting as it had the opposite behavior when run against the 1.5 branch and I don't think the patch has changed at all.  I have a feeling that this is going to have a huge fall out in terms of follow on bugs, so I encourage you to test this thoroughly before you check it in.  If it has a huge fallout in terms of follow on bugs, we'll back this out from 1.5 and just take it on master.

r=ctalbert with the changes and questions answered from above (particuarly about how to handle improperly constructed exepectedEvent objects.)
Attachment #462931 - Flags: review?(ctalbert) → review+
(In reply to comment #12)
> >+MozMillController.prototype.mouseEvent = function(aTarget, aOffsetX, aOffsetY,
> >+                                                  aEvent, aExpectedEvent) {
> > 
> >-  if (element.tagName == "menuitem") {
> >-    element.click();
> >-    return true;
> >+  var element = aTarget.getNode();
> >+  if (!element) {
> >+    throw new Error(arguments.callee.name + ": could not find element " + elem.getInfo());
> This will fail, there is no longer an elem in this function.  You should use
> aTarget ^ instead.

Good catch. Thanks.

> We need to be sure that we document the structure of the exepcted event object.

Will be done. Even in the controller.js file itself.

>  This code is making a lot of assumptions about how that object is put
> together. You have two methods of failure here.  If the aExpectedEvent doesn't
> exist, then we will default to the old way of using synthesizeMouse, which is
> correct.  If the aExpectedEvent object is not properly formatted (i.e. it
> exists but (aExepected.event) is false, then we will fall back to using the old
> method as well.  Is that what you want?  Or should we throw an error in that
> case?  I tend to think we should throw an error telling the test writer that
> their exepectedEvent is not properly constructed because otherwise, this will
> constitute a silent failure.

Yeah, that would make sense. When the object has been specified we should throw an error if the type does not exist. The target will still be optional and use the current element as default.

> > MozMillController.prototype.rightclick = function(){
> >-  frame.log({function:'rightclick - Deprecation Warning', message:'Controller.rightclick should be renamed to Controller.rightClick'});
> >+  frame.log({function:'rightclick - Deprecation Warning',
> >+            message:'controller.rightclick should be renamed to Controller.rightClick'});
> >   this.rightClick.apply(this, arguments);
> For future reference, I would really prefer code cleanup things like this be in
> their own patches.  It makes it so much easier to review.  

That was by accident. I will remove that part. We should even completely get rid of this function.

> >-                         "__doEventDispatch", "_parseModifiers", "synthesizeMouse",
> >-                         "synthesizeMouseScroll", "synthesizeKey", "_expectEvent",
> >-                         "_checkExpectedEvent", "synthesizeMouseExpectEvent",
>
> Why are you removing exports from this code? Is this just another cleanup step?

The only exports which get removed are the ones with a _ prefix. Those shouldn't be exported at all. 

I will upload a new patch soonish.
Whiteboard: [mozmill-1.4.2-][mozmill-1.5.1+] → [mozmill-1.4.2-][mozmill-1.5.1+][mozmill-doc-needed]
Attached patch Patch v2 (obsolete) — Splinter Review
Patch with review comments addressed.
Attachment #462931 - Attachment is obsolete: true
Attachment #481253 - Flags: review?(ctalbert)
Comment on attachment 481253 [details] [diff] [review]
Patch v2

Sorry, missed to update the test. :) New version upcoming once it works.
Attachment #481253 - Attachment is obsolete: true
Attachment #481253 - Flags: review?(ctalbert)
Attached patch Patch v2.1 (obsolete) — Splinter Review
Updated patch with working tests and fixed problems catched by the tests. Yay!
Attachment #481267 - Flags: review?(ctalbert)
Attached patch Patch v2.2 (obsolete) — Splinter Review
Not sure why the test itself got lost from the patch queue. Now it is attached.
Attachment #481267 - Attachment is obsolete: true
Attachment #481474 - Flags: review?(ctalbert)
Attachment #481267 - Flags: review?(ctalbert)
Comment on attachment 481474 [details] [diff] [review]
Patch v2.2

>-  if (isNaN(left)) { left = 1; }
>-  if (isNaN(top)) { top = 1; }
>+  // If no offset is given we have we will use (2,2) as position, because of
>+  // rounded borders (identical to Mochitests)
>+  if (isNaN(aOffsetX))
>+    aOffsetX = 2;
>+  if (isNaN(aOffsetY))
>+    aOffsetY = 2;

Given the posting from dbaron on the dev-apps-firefox list we should probably change it to use (6,6) for now or even better calculate the center of the element. Clint, what do you think?
Attached patch Patch v2.3Splinter Review
As talked with Clint on IRC we also want to synthesize a click at the center of the element when no offset is given. This patch adds it.
Attachment #481474 - Attachment is obsolete: true
Attachment #482431 - Flags: review?(ctalbert)
Attachment #481474 - Flags: review?(ctalbert)
Blocks: 603675
Blocks: 603725
Blocks: 603726
Blocks: 604042
Blocks: 604096
Attachment #482431 - Flags: review?(ctalbert) → review+
Landed on hotfix-1.5.1 branch:
http://github.com/mozautomation/mozmill/commit/12bf6079104e37df1c7858d83c6df6a75efb6560
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-1.4.2-][mozmill-1.5.1+][mozmill-doc-needed] → [mozmill-1.5.1+][mozmill-doc-needed][needs-landing-2.0]
No longer blocks: 604096
Verified  fixed with 1.5.1rc1
Status: RESOLVED → VERIFIED
Pushed to master:
https://github.com/mozautomation/mozmill/commit/746c992ecbba943a0ca3946cd29a5472850a6194
Whiteboard: [mozmill-1.5.1+][mozmill-doc-needed][needs-landing-2.0] → [mozmill-1.5.1+][mozmill-doc-needed]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.