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.

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

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.

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

(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 ;)

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 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.

- 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

(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.

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)));

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

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

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));

(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?

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

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 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.

(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 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.

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.

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!

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.

(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.

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.

(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 on attachment 463673 [details] [diff] [review] change to frame.log looks good.

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

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.