506393 - Need an assertPropertyNotExist in Mozmill controller API

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[cmtalbert]

We currently have no way to specify that a DOM property does not exist in Mozmill.  Since most DOM attributes can either "not exist" by being not "true" and/or not existing, we need a special method to check this.

Anthony has volunteered to take this on.

Comment 1

[u279076]

Attachment: Initial patch

Comment 2

[cmtalbert]

Comment on attachment 390582 [details] [diff] [review]
Initial patch

This does assert that the property does not exist.  However, it does not assert that the property is not applied.  But, since different properties work in different ways, it makes sense to just test for "notExists".  Otherwise, you open the gate to all kinds of side-effect behaviors.  I'll take this. r=ctalbert

Comment 3

[cmtalbert]

Sending        mozmill/extension/resource/modules/controller.js
Transmitting file data .
Committed revision 537.

--> FIXED

Comment 4

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

Comment on attachment 390582 [details] [diff] [review]
Initial patch

>+// Assert that an element's property does not exist
>+MozMillController.prototype.assertPropertyNotExist = function(el, attrib) {
>+  var element = el.getNode();
>+  if (!element) {
>+    throw new Error("could not find element " + el.getInfo());
>+	return false;
>+  }
>+  if (!element.hasAttribute(attrib)) {
>+    frame.events.pass({'function':'Controller.assertPropertyNotExist()'});
>+    return true;
>+  }
>+  throw new Error("assert failed for checked element " + el.getInfo());

The thrown error has been copied from assertChecked. It also needs an update.

Comment 5

[u279076]

(In reply to comment #4)
> (From update of attachment 390582 [details] [diff] [review])
> >+// Assert that an element's property does not exist
> >+MozMillController.prototype.assertPropertyNotExist = function(el, attrib) {
> >+  var element = el.getNode();
> >+  if (!element) {
> >+    throw new Error("could not find element " + el.getInfo());
> >+	return false;
> >+  }
> >+  if (!element.hasAttribute(attrib)) {
> >+    frame.events.pass({'function':'Controller.assertPropertyNotExist()'});
> >+    return true;
> >+  }
> >+  throw new Error("assert failed for checked element " + el.getInfo());
> 
> The thrown error has been copied from assertChecked. It also needs an update.

I've assumed checked as in verified not checked as in checkbox.  "assert failed for property not exist element" does not make sense.

Comment 6

[u279076]

If "assert failed for checked element" is meant as a general throw as in "I'm looking at this element to see something" then let's leave it as is for all tests.  Never once did it occur to me that this was a specific statement for a checkbox.  If we want specific messages, we should change to "<methodName> failed for element <elementName>" for ALL assertions.

Comment 7

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

Checked is a bad wording with can cause confusion. We only had it for assertChecked for now which meant that a checkbox is checked and not the element we are working on.

We still have to many different wordings for exceptions we throw if an assert fails. A general wording would be fine. But I would leave this for the future. It's not so important for our work at the moment.

But we could start with this function so it would be like "assertPropertyNotExist for element <el>". The "Failed: " prefix will automatically added by Mozmill.

Mikeal?

Comment 8

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

Can you please check comment 7?

Comment 9

[Adam Christian]

What we do with windmill is dynamically build assertNot for every assert with the correct wording. So adding an assert automaticallly also adds an assertNot. If we want to manually create these we can just decide to use a syntax, OR we could juse create a function that takes a couple params and generates them so that the code is all in one place. Just my two cents.

Comment 10

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

Anthony, can you please file a new bug so we can move the discussion over there?

Meanwhile we should get a temporary solution in for this bug like "Assert for not existent property failed for <el>"?

Comment 11

[u279076]

(In reply to comment #10)
> Anthony, can you please file a new bug so we can move the discussion over
> there?
> 
> Meanwhile we should get a temporary solution in for this bug like "Assert for
> not existent property failed for <el>"?
If I'm going to change the message for assertPropertyNotExist, I would rather just do it for all asserts at once.  So, I can A) do them all now in a single patch on this bug or B) re-resolve this bug as-is and do them all in the new bug in a single patch.

Comment 12

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

Ok, lets push it to a new bug.

Comment 13

[u279076]

New bug filed for assert message templating: bug 507654.

Re-re-resolving this bug (hopefully for the last time).

Comment 14

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

Verified fixed so far. Thanks Anthony!