583773 - Add (not) property assert functions for JS and DOM properties

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Andrew Halberstadt [:ahal]]

The test case that M.A Darche submitted for his 'assertProperty always returns true' fix uses a function called 'assertPropertyNotEquals()'.  This function is documented on MDC but doesn't actually exist in the controller API.

Currently we can use 'assertProperty()' to test both that a property exists and that a property's value is equal to some other value.  We also have a method called 'assertPropertyNotExist()'.  I think to be consistent we should roll 'assertPropertyNotExist()' into a method called 'assertNotProperty()' which can be used to check both existence and value.

Currently in the mozmill-tests, 'assertPropertyNotExist()' is only referenced once in 'testPrivateBrowsing/testDisabledElements.js' so the test breakage should be minimal.

Comment 1

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

The decision for [mozmill-1.4.2+] is made in our project meeting.

Comment 2

[cmtalbert]

I'm happy to talk about this in the meeting, but we talked about it here in great length today, and decided this is something we should take for two reasons:
1. assertPropertyNotEquals is mentioned in the docs, and without that api you have no way to test that a property does not equal some value.  !assertPropertyEqual() will not work as that will throw rather than returning false.

2. assertProperty() tests both existance and equality.  It really doesn't make sense not to have the reverse of that.  We only have the reverse that tests for the case that a property does not exist.

This change makes testing easier, impacts only one known test, and is very small; therefore, I told Andrew we should take it for 1.4.2.

Comment 3

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

That's fine then. Next time such a decision should just be part of the comment.

Comment 4

[cmtalbert]

(In reply to comment #3)
> That's fine then. Next time such a decision should just be part of the comment.
Yeah I planned on it, but you -'ed it before I got to writing the comment ;)

Comment 5

[Andrew Halberstadt [:ahal]]

Attachment: Adds assertNotProperty() to API

This patch deprecates assertPropertyNotExists() as assertNotProperty can be used to check both the existence of a property and its value.

This patch also modifies assertProperty() so that using it without the 'val' parameter checks for existence rather than value.

Comment 6

[cmtalbert]

Comment on attachment 462225 [details] [diff] [review]
Adds assertNotProperty() to API

This looks good.  We should also change the documentation on MDC (or set that doc-needed flag on this bug), file a second patch to change the one instance using assertPropertyNotExists in the mozmill-tests, and change the M.A. Darche's test that uses assertPropertyNotEquals to use assertNotProperty in our mozmill/tests folder as well.

r+ with those three extra things.

Comment 7

[Andrew Halberstadt [:ahal]]

- Updated MDC (with note about function only being available in 1.4.2 and later)
- Will file bug for mozmill-test
- M.A Darche's test case is updated locally for me already, and I'll check it in when I'm finished cleaning up that directory

Master: http://github.com/mozautomation/mozmill/commit/1f022c859adf77881e2290d7d379cd5e77efd8d1

1.4.2: http://github.com/mozautomation/mozmill/commit/22605167c83136454f63000cf4920c39127b04dd

Comment 8

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

(In reply to comment #7)
> - M.A Darche's test case is updated locally for me already, and I'll check it
> in when I'm finished cleaning up that directory

Please don't check the menu entry as what is done in this test. It will fail on OS X. We should simply check an UI element are have a custom test case file.

Comment 9

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

This patch massively breaks existing tests because it makes wrong assumptions for an identical named attribute which doesn't have to exist for a property. All Mozmill tests which handle auto-complete popups are failing:

> -  var res = (String(value) == String(val));
> +  var res = (element.hasAttribute(attrib) && (val == undefined ? true : String(value) == String(val)));

Comment 10

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

Oh, missed the exact file names of those tests. Please check tests under firefox/testFormManager/

Comment 11

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

Backed-out only on 1.4.2:
http://github.com/mozautomation/mozmill/commit/4a8cd0945d4b1b24fe4c9b2389f8d0f0774227c8

Comment 12

[Andrew Halberstadt [:ahal]]

I think that this should be discussed in the meeting.  I did this intentionally because when I call 'assertProperty()', I imagine that if the property doesn't exist, the assertion should fail.

I didn't realize that there were tests out there that actually did this.

If we decide that 'assertProperty()' can return true even if the property doesn't exist then we can do:
>-  var res = (element.hasAttribute(attrib) && (val == undefined ? true : String(value) == String(val)));
>+  var res = (val == undefined ? element.hasAttribute(attrib) : String(value) == String(val));

Comment 13

[Andrew Halberstadt [:ahal]]

(In reply to comment #9)
> it makes wrong assumptions for an identical named attribute which doesn't have > to exist for a property.

Henrik, can you elaborate?  This 'PopupAutoComplete' element seems to be really strange.

For some reason 'element[attrib] == true' but 'element.hasAttribute(attrib) == false'

Do you have any idea why this might be the case?

Comment 14

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

I think I don't have to give any more input. We have discussed that very well in the meeting.

Comment 15

[Andrew Halberstadt [:ahal]]

Attachment: Adds separate controller functions for JS and DOM properties

As discussed in the meeting there are now four controller API functions for property assertions.

-assertJSProperty
-assertNotJSProperty
-assertDOMProperty
-assertNotDOMProperty

All functions can be used to check either existence or value of a property.  Both assertProperty() and assertPropertyNotExist() now show deprecation errors.

Comment 16

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

Comment on attachment 462658 [details] [diff] [review]
Adds separate controller functions for JS and DOM properties

>+MozMillController.prototype.assertJSProperty = function(el, attrib, val) {
[..]
>+  var res = (value !== undefined && (val === undefined ? true : String(value) == String(val)));

>+MozMillController.prototype.assertNotJSProperty = function(el, attrib, val) {
[..]
>+  var res = (val === undefined ? value === undefined : String(value) != String(val));

While having a quick look, why do those lines differ that much? Can't we have a similar style like the second one here?

We will need a good documentation for people which describes how to differentiate between those two different kinds of assert functions and how they know which one they have to use.

Comment 18

[Andrew Halberstadt [:ahal]]

(In reply to comment #16)
> Comment on attachment 462658 [details] [diff] [review]
> Adds separate controller functions for JS and DOM properties
> 
> >+MozMillController.prototype.assertJSProperty = function(el, attrib, val) {
> [..]
> >+  var res = (value !== undefined && (val === undefined ? true : String(value) == String(val)));
> 
> >+MozMillController.prototype.assertNotJSProperty = function(el, attrib, val) {
> [..]
> >+  var res = (val === undefined ? value === undefined : String(value) != String(val));
> 
> While having a quick look, why do those lines differ that much? Can't we have a
> similar style like the second one here?
For assertJSProperty we always want to make sure that the property exists, whether or not val was passed in.  That's why the value !== undefined has its own clause

For assertNotJSProperty we don't always want to check if the property exists, because sometimes we want to return true when it does, and sometimes we return true when it doesn't.

> We will need a good documentation for people which describes how to
> differentiate between those two different kinds of assert functions and how
> they know which one they have to use.
Yeah, I'll update the docs when this gets reviewed and checked in.

Comment 19

[Heather Arthur [:harth]]

Comment on attachment 462658 [details] [diff] [review]
Adds separate controller functions for JS and DOM properties

>+  var res = (value !== undefined && (val === undefined ? true : String(value) == String(val)));

r+ but add comment about how the third 'val' argument is optional.

Comment 20

[Andrew Halberstadt [:ahal]]

Comments added.

master: http://github.com/mozautomation/mozmill/commit/27c1d6e798ba36faff61f9d4e2535bb8deff6bec

1.4.2: http://github.com/mozautomation/mozmill/commit/6aff4d267b6ae5fcd8169a35a56cb85809aebe8b

Note that any tests that use assertProperty or assertPropertyNotExist will still work but will receive deprecation errors.  I'll file a new bug for changing these tests.

Comment 21

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

Not sure why this patch has been landed. It marks all existing tests which use the assertProperty() method as fail. Please test the patches with our existing  Firefox tests before asking review. Deprecation warnings should never mark tests as failed!

Comment 22

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

The code should really use frame.log as given in the controller.rightclick() method:

frame.log({function:'rightclick - Deprecation Warning', message:'Controller.rightclick should be renamed to Controller.rightClick'});

This is one more checkin which blocks me locally from running tests for my own patches.

Comment 23

[cmtalbert]

(In reply to comment #21)
> Not sure why this patch has been landed. It marks all existing tests which use
> the assertProperty() method as fail. Please test the patches with our existing 
> Firefox tests before asking review. Deprecation warnings should never mark
> tests as failed!

Yes, it should.  Otherwise, the error is invisible.  This is working as designed.  The test must be changed, and the best way to ensure that it will be changed is to fail it until the test is fixed.

Comment 24

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

If an API method is marked as deprecated a warning should be shown to the user of that method. It's a transition phase from one to another function or name, while the new and the old method exists beside each other. Both existing methods should still be able to use, which gives customers time to update their code. Please don't force them to update their code immediately after the update to 1.4.2. Give them time at least until the next version before throwing a hard failure.

In the current case the deprecation doesn't make sense and you could have simply removed the function. Using frame.log() instead will add an entry to the log file.

I'm worried about that behavior because people who are using Mozmill will probably not immediately update to Mozmill 1.4.2. They will no longer be able to run our tests. Also imagine if we have a regression and we have to go back to 1.4.1 we would have to completely revert all those tests again. I really would like to wait with that update for the next 1 or 2 weeks until 1.4.2 has been proofed as stable, secure, and reliable.

Comment 25

[cmtalbert]

(In reply to comment #24)
> If an API method is marked as deprecated a warning should be shown to the user
> of that method. It's a transition phase from one to another function or name,
> while the new and the old method exists beside each other. Both existing
> methods should still be able to use, which gives customers time to update their
> code. Please don't force them to update their code immediately after the update
> to 1.4.2. Give them time at least until the next version before throwing a hard
> failure.
> 
> In the current case the deprecation doesn't make sense and you could have
> simply removed the function. Using frame.log() instead will add an entry to the
> log file.
>
I'd have preferred to remove the function entirely.  I only had Andrew do the deprecation message because that's what you wanted.

I'll change it to log if that's what you want, but I really disagree.  We have fundamentally changed the behavior here.  It is likely that the people using this API are not testing what they intended to be testing, so it seems to me like "fail" is the appropriate reaction here to force people to fix their tests properly.

Comment 26

[cmtalbert]

Attachment: change to frame.log

Comment 27

[Heather Arthur [:harth]]

Comment on attachment 463673 [details] [diff] [review]
change to frame.log

looks good.

Comment 28

[cmtalbert]

Landed on master: http://github.com/mozautomation/mozmill/commit/b428fa2f63ae81ed92ded55bdaa105df4c3ba105

Landed on 1.4.2: http://github.com/mozautomation/mozmill/commit/dfb4049d8e33e375257561909ee32ecffb76eb60

Comment 29

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

Thanks for the follow-up push which gives us some time to change our tests for the new API methods.

Everything looks good while testing with the latest Mozmill dev build. Marking as verified fixed.