506380 - Need an assertNotChecked method

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[u279076]

Mozmill has an assertChecked method but no assertNotChecked method.  This is handy when checking that certain prefs are supposed to be not checked.

Comment 1

[u279076]

Attachment: Patch

Comment 2

[u279076]

Attachment: Rev1

Comment 3

[u279076]

Original patch checked if checked == false which is incorrect.  If a box is unchecked it can be !true or non-existant.  Updated the if to check if hasAttribute("checked") or checked != true.

Comment 4

[cmtalbert]

Comment on attachment 390572 [details] [diff] [review]
Rev1

Next time, dont' forget to put an id in the review request ;)  The only thing I'd change here would be to add one more item to the if statement and that is:
if (n && !n.hasAttribute...
That way in case el.getNode returns null we don't just die.  I'll make that change when I check it in.  

Thanks for the patch!!
r=ctalbert

Comment 5

[cmtalbert]

"bug506275" [New] 25L, 890C written
host-2-8:mozmill clint$ svn commit -m "Bug 506380, patch by Ashughes, r=ctalbert, Add assertNotChecked method to controller.js" mozmill/extension/resource/modules/controller.js
Sending        mozmill/extension/resource/modules/controller.js
Transmitting file data .
Committed revision 535.

Itook out the commented out line of the // window.focus() call and added in a if statement so that we'd throw with an appropriate error message in case el.getNode() returned null.

--> FIXED

Comment 6

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

Comment on attachment 390572 [details] [diff] [review]
Rev1

>+  var n = el.getNode();
>+
>+  if (!n.hasAttribute("checked") || n.checked != true){ 
>+    frame.events.pass({'function':'Controller.assertNotChecked()'});
>+    return true; 
>+    }

For consistency it would have been nice to name it element instead of n. Clint, can you please make the change with the upcoming assertPropertyNotExist checkin?

Comment 7

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

Comment on attachment 390572 [details] [diff] [review]
Rev1

>+  var n = el.getNode();
>+
>+  if (!n.hasAttribute("checked") || n.checked != true){ 
>+    frame.events.pass({'function':'Controller.assertNotChecked()'});
>+    return true; 
>+    }

As already said please rename 'n' to 'element'.

>+  throw new Error("assert failed for checked element " + el.getInfo());
>+  return false;

The assert message is wrong. It should be named "assert failed for not checked element ".

We have to update this.

Comment 8

[u279076]

Are you going to back out the patch or should I just write an incremental patch...

Comment 9

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

An incremental patch would be fine I think. There is no need to let Clint or Mikeal do twice that work.

Comment 10

[u279076]

Attachment: Rev2

Revisions made...

Comment 11

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

Comment on attachment 391398 [details] [diff] [review]
Rev2

Clint, can you please review?

Comment 12

[cmtalbert]

Comment on attachment 391398 [details] [diff] [review]
Rev2

r=ctalbert.  My apologies to Anthony for having to waste time on this. This is not user facing code, and the code was absolutely clear before this change in my opinion.  There was no reason to reopen this bug whatsoever.  But thank you for the patch.  I'll check it in.

Comment 13

[cmtalbert]

Transmitting file data .
Committed revision 538.

Comment 14

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

(In reply to comment #12)
> r=ctalbert.  My apologies to Anthony for having to waste time on this. This is
> not user facing code, and the code was absolutely clear before this change in
> my opinion.  There was no reason to reopen this bug whatsoever.  But thank you

The wrong assert error text is definitely user facing code which would be wrongly inserted into the result server at a later stage. So I still think it was worth fixing it. The element renaming was just a side effect.

The last patch has broken the assertNotChecked function. Follow-up patch upcoming.

Comment 15

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

Attachment: Follow-up

Just a small note. Please test your patches before attaching it to the bug. That would eliminate those follow-ups.

Comment 16

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

Adding in-testsuite? to get a unit test at some time.

Comment 17

[cmtalbert]

Comment on attachment 391816 [details] [diff] [review]
Follow-up

(In reply to comment #15)
> Created an attachment (id=391816) [details]
> Follow-up
> 
> Just a small note. Please test your patches before attaching it to the bug.
> That would eliminate those follow-ups.

Crap! I assumed the patch was tested and only looked at the diff in bugzilla since it was a simple patch. Thanks for catching it Henrik.

My bad.

Do we have a bug open on unit testing mozmill API's?  I'm tired of getting bit by things like this :(  We started out with doing that, but then it fell aside. Again, my bad.  I'll file a follow on bug for one if one doesn't exist.

Comment 18

[cmtalbert]

r539 :(

Fixed. Again. Dammit.

Comment 19

[u279076]

Crap...in all the confusion and back-n-forth I forgot to test this patch before attaching it to the bug.  I apologize...

Comment 20

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

(In reply to comment #17)
> Do we have a bug open on unit testing mozmill API's?  I'm tired of getting bit
> by things like this :(  We started out with doing that, but then it fell aside.
> Again, my bad.  I'll file a follow on bug for one if one doesn't exist.

We should not file new bugs for getting tests in. We have the in-testsuite flag which is a wonderful thing for this type of work. Each bug with in-testsuite? or an empty flag should be taken into account. There is only one bug with in-testsuite+ yet.

Ok, this time we have it. Marking verified fixed for 1.3.