574632 - controller.assertProperty(Elem, attr, '') always returns true

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Marc-Aurèle DARCHE]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.2.4) Gecko/20100611 Firefox/3.6.4
Build Identifier: 

For example the following assertProperty call returns true while it should return false because the "command" property of the new_tab_menu_item is indeed "cmd_newNavigatorTab" :

var new_tab_menu_item = new elementslib.ID(controller.window.document,
    'menu_newNavigatorTab');
controller.assertProperty(new_tab_menu_item, 'command', '');





Reproducible: Always

Comment 1

[Marc-Aurèle DARCHE]

Attachment: Modification of the condition to decide if the value matches. Fixes the case of a passed empty string value.

I'm wondering why there is the (value.indexOf(val) != -1) test. What is it for?

Comment 2

[Marc-Aurèle DARCHE]

Attachment: Modification of the condition to decide if the value matches. Fixes the case of a passed empty string value.

Comment 3

[Marc-Aurèle DARCHE]

Attachment: Modification of the condition to decide if the value matches. Fixes the case of a passed empty string value.

Comment 4

[cmtalbert]

Awesome, thanks for the patch.  let's take this for 1.4.2.

Comment 5

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

Comment on attachment 454015 [details] [diff] [review]
Modification of the condition to decide if the value matches. Fixes the case of a passed empty string value.

>   try {
>-    if (value.indexOf(val) != -1){
>+    // Question: Why this test ? It does only test that value contains val. This
>+    // really is not an enough condition.
>+    if (value.indexOf(val) != -1) {
>       res = true;
>     }
>   }

I don't see a reason why this block should be left in the code. We set res to true but immediately overwrite its value here:

>+  // This test does catch the case where val == ''
>+  // cf. Bugzilla@Mozilla bug #574632
>+  res = (String(value) == String(val));

Why do we have to transform the type into a string object? That seems to be unnecessary. Also please remove both comments. Those aren't necessary with the mentioned changes.

>   } else {
>-    throw new Error("Controller.assertProperty(" + el.getInfo() + ") : " + val + " == " + value);
>+    throw new Error("Controller.assertProperty(" + el.getInfo() + ") : " + val + " != " + value);

Please don't change that. The condition we are testing here is for equal values. If the comparison fails we have to visualize that.

Thanks for working on a fix!

Comment 6

[Marc-Aurèle DARCHE]

Attachment: Modification of the condition to decide if the value matches. Fixes the case of a passed empty string value.

Comment 7

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

Comment on attachment 456443 [details] [diff] [review]
Modification of the condition to decide if the value matches. Fixes the case of a passed empty string value.

>+  var value = eval('element.' + attrib + ';');

I read a couple of stuff over the weekend and part of that were docs about eval. Eval is evil. I wonder why we are using eval here, while we can directly access the property via element.attrib.

>+  var res = (String(value) == String(val));

Should we really convert the value to a String? That would mean a numerical property with the value 8 would be identical to a property with a string value of "8".

I will leave additional comments up to Clint. With the above changed it will be r=me from my perspective.

At some point it would also be nice to see bug 519427 fixed.

Comment 8

[Marc-Aurèle DARCHE]

I agree on your two remarks, the one about eval and the one about the string comparison. I feel the same about those.

But my aim with the provided patch was just to address the specific point of the subject of this ticket. I tried to not touch anything else. I thought that if I was touching other areas of this method my patch would be questioned and more likely to be rejected.

And to avoid regression, it would be good to write a unit test for assertProperty if it's possible. Shouldn't we do that?

Comment 9

[cmtalbert]

Comment on attachment 456443 [details] [diff] [review]
Modification of the condition to decide if the value matches. Fixes the case of a passed empty string value.

(In reply to comment #8)
> I agree on your two remarks, the one about eval and the one about the string
> comparison. I feel the same about those.
> 
I'm also not sure why we're using eval here.  The only real use case I can see for doing that would be to ensure that the argument we're looking at hasn't changed out from underneath us.  But I honestly don't think that's a huge worry.  If the element is changing that fast, we'd probably be unable to use this API in the first place to do anything meaningful.

And as far as converting the to strings go, that was a problem with this code before your patch, so it makes sense to correct the problem you saw and leave that questionable behavior intact.

Please file a follow-up bug on this issue though so we can take a look at rationalizing this API for 2.0.
> 
> And to avoid regression, it would be good to write a unit test for
> assertProperty if it's possible. Shouldn't we do that?
Yes, if you could also attach a simple unit test that'd be awesome.  You can just write a simple mozmill test that tests this API for now and place it in the mozmill/test directory.  I'd really appreciate that.

Thanks so much for the patch!
r=ctalbert

Comment 10

[Marc-Aurèle DARCHE]

As for the use of "eval" in the Mozmill code, there is already #530546.
And regarding the dubious String comparison I have created the ticket #580963.

Comment 11

[Marc-Aurèle DARCHE]

Attachment: Mozmill test for Mozmill itself testing the testProperty and testPropertyNotEquals methods

I couldn't run those tests successfully myself since I got other unrelated errors when testing this against the 1.4.2 branch.

Could you please describe on
https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup
how to create self tests for Mozmill and especially how to run them.

Being limited by this situation I couldn't add the examples below that you proposed:
"
assertPropertyEquals(myelem, attrib, '8')
assertPropertyNotEquals(myelem, attrib, '8')
"

The problem is also that this kind of test requires that one modifies an existing element of a XUL interface.

Comment 12

[Marc-Aurèle DARCHE]

Comment on attachment 459381 [details] [diff] [review]
Mozmill test for Mozmill itself testing the testProperty and testPropertyNotEquals methods

Forgot this patch, it's buggy. I'm working on a new one. Sorry

Comment 13

[Marc-Aurèle DARCHE]

Attachment: Mozmill test for Mozmill itself testing the testProperty and testPropertyNotEquals methods

This patch is better :-)

Comment 14

[cmtalbert]

Comment on attachment 459459 [details] [diff] [review]
Mozmill test for Mozmill itself testing the testProperty and testPropertyNotEquals methods

This looks great.  Thanks for the test.  We don't actually have a good test harness for mozmill's own APIs right now.  That's one of the things I'm working on.  I'll update the documentation when I've got something working.  Thanks for your help.

Comment 15

[cmtalbert]

* master: http://github.com/mozautomation/mozmill/commit/e3e654d049b36b40e6ce4702a4b2f79d50c9efa3

* 1.4.2: http://github.com/mozautomation/mozmill/commit/5de486f97ff85e17d2de7f13f8bec77a49e62e70

Comment 16

[Aaron Train [:aaronmt]]

Trying to verify this, but when I run the test_property.js test, I get controller.assertPropertyNotEquals is not a function. Huh?

Comment 17

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

Aaron, do you have the latest dev version of Mozmill or running the beta 1? The latter one doesn't have this feature included.

Comment 18

[Aaron Train [:aaronmt]]

(In reply to comment #17)
> Aaron, do you have the latest dev version of Mozmill or running the beta 1? The
> latter one doesn't have this feature included.

Ah tested on beta 1. I'll pass on verification of this while I correct my mozmill environment

Comment 19

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

(In reply to comment #18)
> Ah tested on beta 1. I'll pass on verification of this while I correct my
> mozmill environment

Can we mark it verified?

Comment 20

[Aaron Train [:aaronmt]]

(In reply to comment #19)
> (In reply to comment #18)
> > Ah tested on beta 1. I'll pass on verification of this while I correct my
> > mozmill environment
> 
> Can we mark it verified?

Yep. VERIFIED FIXED w/mozmill  1.4.2b2