Closed Bug 512789 Opened 10 years ago Closed 10 years ago

controller.check and controller.radio not working

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: merike, Assigned: whimboo)

References

Details

(Whiteboard: [mozmill-doc-complete][mozmill-1.2.2])

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; et; rv:1.9.1.2) Gecko/20090729 Guess me! (.NET CLR 3.5.30729)
Build Identifier: 

Both give: fail :: TypeError: MozMillController.click is not a function

controller.click on the same element works.

Reproducible: Always
Both perform a simple .click on the element. See:
http://code.google.com/p/mozmill/source/browse/trunk/mozmill/extension/resource/modules/controller.js#467

Mikeal, do we really need those two functions? We don't use them at all in Firefox. Given the fact that both don't forward an x/y coordinate those could fail under some conditions.
Confirmed this bug. It shouldn't be too hard to fix it.

At least the check function should also get a 2nd parameter which gives information about the target value of the checkbox.

I will check.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Patch (obsolete) — Splinter Review
Ok, so I have updated the check function. Now it clicks the checkbox if the target state is different from the current state. That should be the way we have to go when comparing it to other functions.

Further I have removed the function for radio buttons because they don't make sense. We cannot get a reference to the radio group easily. Mikeal, if you still wanna have a radio function we should consider doing following:

1. Get the radiogroup
2. Check all the contained radio buttons that only the given element is checked

What do you think?
Attachment #398454 - Flags: review?(mrogers)
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Comment on attachment 398454 [details] [diff] [review]
Patch

This looks fine.  r=ctalbert if you will also go onto MDC and update the controller API doc there with the change too, please.
Attachment #398454 - Flags: review?(mrogers) → review+
I will update the docs once it has been checked in.

Mikeal whenever you think you checkin this patch please do so.
Keywords: checkin-needed
Whiteboard: [mozmill-doc-needed][mozmill-1.2.2]
The final return false does not actually cause this function to fail.

Also, this new validation seems to only work if the box is not already checked. It was my understanding that check() would check or uncheck a box depending on it's current state. 

If this is no longer the case, and we want to fail if it is already checked then we need to add an uncheck function for the reverse case. For what it's worth I like this API better because it's more explicit and will cause failures when the boxes are in an unintended state.
(In reply to comment #6)
> The final return false does not actually cause this function to fail.

That was not my aim. This function can only fail when the click does not succeed. Otherwise I thought it is the correct way to return false because the checked value hasn't been modified in this function.

> Also, this new validation seems to only work if the box is not already checked.
> It was my understanding that check() would check or uncheck a box depending on
> it's current state. 

The box will be checked or unchecked when it hasn't already the target state. That's why the new parameter for this function. See the select function where we use the same way. The callee specifies the target state and the function executes as wanted. For the checkbox we only have true/false.

When you further check all of our existing tests where we operate on checkboxes you will see that we always have to check the checked value before operating on the checkbox, which is a bit annoying and useless when we are going this way.

> If this is no longer the case, and we want to fail if it is already checked
> then we need to add an uncheck function for the reverse case. For what it's
> worth I like this API better because it's more explicit and will cause failures
> when the boxes are in an unintended state.

If we wanna check the state of a checkbox we can use assertChecked which is already a controller function. If you feel better to separate it into two functions we can do that but I just wanted to not overload the controller object. The name 'check' is my understanding the action and is not bind to the current check/uncheck state. Otherwise we should have been split the select function too.
Mikeal, Clint? So how do we wanna proceed?
Attached patch Patch v2 (obsolete) — Splinter Review
Until the patch doesn't get checked-in I have another improved version:

* Added back controller.radio which is necessary as what I have recognized while working on a test with radio buttons.
* We have to wait for the target state and should fail if it hasn't been set
* More clear pass/fail frame events
Attachment #402306 - Flags: review?(ctalbert)
Attachment #398454 - Attachment is obsolete: true
Clint, can you please have a look at the new patch?
Comment on attachment 402306 [details] [diff] [review]
Patch v2

>diff --git a/mozmill/extension/resource/modules/controller.js b/mozmill/extension/resource/modules/controller.js
>index cc5046c..e91b844 100644
>--- a/mozmill/extension/resource/modules/controller.js
>+++ b/mozmill/extension/resource/modules/controller.js
>@@ -329,6 +329,45 @@ MozMillController.prototype.rightclick = function(){
>   this.rightClick.apply(this, arguments);
> }
> 
>+/**
>+ * Enable/Disable a checkbox depending on the target state
>+ */
>+MozMillController.prototype.check = function(el, state) {
>+  var result = false;
>+  var element = el.getNode();
>+
>+  if (!element) {
>+    throw new Error("could not find element " + el.getInfo());
>+    return false;
>+  }
>+
>+  if (state != element.checked) {
>+    this.click(el);
>+    this.waitForEval("subject.checked == " + state, 500, 100, element);
Let me walk through the uncheck behavior here:
Now, if you call into here with controller.check(el), then state is going to be undefined, so state != element.checked will be true, and so we will click on the item, which should uncheck it.  However, there is no clear case that this waitForEval will work.  Usually unchecking something means that the checked attribute is removed from the element.  It might go to null, it might be undefined, or it might be false.  I think in some cases, this waitForEval is going to evaluate incorrectly based on that behavior.

Now, if this is only going to be called when you want to check a previously unchecked box, then you will be ok, I believe.

So r+ if this is only supposed to be used to "check" a box and not uncheck it.  r- if it is supposed to be used to uncheck. I'll hold off review pending clarification.
Comment on attachment 402306 [details] [diff] [review]
Patch v2

It's for both cases. Thanks for pointing me to that missed check. I'll come up with a new patch immediately.
Attachment #402306 - Flags: review?(ctalbert)
Attached patch Patch v3Splinter Review
This fixes the problem with undefined, null, or whatever value of state which is not boolean. Now this function works reliable for both cases (check/uncheck).
Attachment #404592 - Flags: review?(ctalbert)
Attachment #404592 - Flags: review?(ctalbert)
Attachment #402306 - Attachment is obsolete: true
merged to mikeal/master
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 520761
Verified fixed with:
http://github.com/mikeal/mozmill/commit/1a3f9c5bf42d8036e4d2f2d0844bcd826d9d7ac1

Completed Mozmill docs too.
Severity: minor → normal
Status: RESOLVED → VERIFIED
Whiteboard: [mozmill-doc-needed][mozmill-1.2.2] → [mozmill-doc-complete][mozmill-1.2.2]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.