568978 - Need assertEquals support for arrays in JUM module

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, enhancement)

Description

[Marc-Aurèle DARCHE]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: 

Having a JUM  assertion to compare 2 arrays and return true if they have the same values would be useful addition.

Reproducible: Always




Simply modifying the JUM assertEquals method to specifically handle arrays would be an option:

var array1 = ["a", "k", 9, 25];

var array2 = ["a", "k", 9, 25];

assertEquals(array1, array2);

=> returns true

The fact is that in JavaScript ["a", "k", 9, 25] != ["a", "k", 9, 25], while "a" == "a", but what would be the use of this kind of equality in unit tests? And I even wonder what is the use if this kind of array equality in JavaScript in whole? In all other languages ["a", "k", 9, 25] == ["a", "k", 9, 25].

On this JavaScript array equality stuff, the advice of a developer knowing JavaScript from a long time, and why that design choice was made in the beginning, will help.

Comment 1

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

Personally I would like to see it as a smart feature of assertEquals and assertNotEquals to check what instance do we have. If its an array do something special otherwise the default comparison. Having additional functions doesn't sound like a good idea.

Clint, what do you think?

Comment 2

[cmtalbert]

(In reply to comment #1)
> Personally I would like to see it as a smart feature of assertEquals and
> assertNotEquals to check what instance do we have. If its an array do something
> special otherwise the default comparison. Having additional functions doesn't
> sound like a good idea.
> 
> Clint, what do you think?
I'd have to look into this one a bit more.  On the surface it seems fine, but I think there are going to be interesting oddities depending on what gets passed in, and I'd like to limit that/fix it.

Comment 3

[Marc-Aurèle DARCHE]

Looks like the [e-mail] addon has the same need:
http://hg.mozilla.org/qa/mozmill-tests/file/507d3aa49e8a/addons/toolbar%40google.com/shared-modules/testArrayUtilsAPI.js

Comment 4

[Marc-Aurèle DARCHE]

Attachment: Patch against mozmill JUM to add array assertions (also removes assertNull duplicate declaration)

Could this be included in Mozmill 1.4.2 please?

Comment 5

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

Comment on attachment 460005 [details] [diff] [review]
Patch against mozmill JUM to add array assertions (also removes assertNull duplicate declaration)

Don't forget to put Clints address in the review field. Otherwise reviews can get lost. Doing it now.

Why you remove the assertNull function?

Comment 6

[Marc-Aurèle DARCHE]

The assertNull function was declared *twice* in the file.

Comment 7

[Marc-Aurèle DARCHE]

Attachment: Patch against mozmill JUM to add array assertions

Sorry for the previous patch. It contained the removal of the assertNull function that I must erroneously have added myself just before! Sorry Henrik for my answer to your question: I thought it was so obvious :-\

Comment 8

[cmtalbert]

Comment on attachment 460514 [details] [diff] [review]
Patch against mozmill JUM to add array assertions

>diff --git a/mozmill/mozmill/extension/resource/modules/jum.js b/mozmill/mozmill/extension/resource/modules/jum.js
>index a2f625a..73ec45a 100644
>--- a/mozmill/mozmill/extension/resource/modules/jum.js
>+++ b/mozmill/mozmill/extension/resource/modules/jum.js
>@@ -1,26 +1,28 @@
> // ***** BEGIN LICENSE BLOCK *****
> // Version: MPL 1.1/GPL 2.0/LGPL 2.1
>-// 
>+//
Not sure what you did here that caused a bunch of whitespace changes...

> 
> var EXPORTED_SYMBOLS = ["assert", "assertTrue", "assertFalse", "assertEquals", "assertNotEquals",
>@@ -205,6 +207,41 @@ var assertNotNaN = function (value, comment) {
>   }
> }
> 
>+var assertArrayEquals = function(value1, value2, comment) {
I'd rather see this as part of the assertEquals function rather than a separate function, so let's go that route.
>+  if (value1.length != value2.length) {
>+    frame.events.fail({'function':'jum.assertArrayEquals', 'comment':comment,
>+                       'message':"The arrays have not the same length",
>+                       'value1':ifJSONable(value1), 'value2':ifJSONable(value2)});
>+    return false;
>+  }
>+  for (var i = 0; i < value1.length; i++) {
>+    if ((value1[i] != value2[i]) || (typeof value1[i] != typeof value2[i])) 
Why not use if (!(value1[i] === value2[i]))
=== compares the values and the types of the variables.
== compares the values and does a conversion if necessary (i.e. the variables are different types)
You should also consider the case where the value in the array is an object.  

>+
>+var assertArrayContains = function(array, value, comment) {
I'm ok with adding this as a separate function.

First, I'd assert that the array you are given is actually an array, and return false if it is not.
>+  for (var i = 0; i < array.length; i++) {
>+    if (array[i] == value) {
Don't you want === here ^?  I can't quite imagine why we'd want to do the implicit type conversion.

Going to have to r- this one.  I'm not seeing a huge benefit to this going into mozmill 1.4.2, so I'm inclined not to take it at this late stage.  If you have some reasons that this change is worth the risk to the schedule at this point, I'd like to hear them.  Otherwise, I think we should take this on the next mozmill release post 1.4.2.

Also, your change will need some tests. At a minimum, I'd like to see the following tests:
* Test with arrays of different lengths
* Tests with one item an array and the other not an array
* Test with items of different types but same values, i.e. '25' and 25
* Test with an array containing objects and scalar values

Thanks again for your patch, let me know if you have any questions.

Comment 9

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

(In reply to comment #8)
> >+var assertArrayContains = function(array, value, comment) {
> I'm ok with adding this as a separate function.

Could we consider to add this as a new member to the Array class, i.e. Array.prototype.contains()? Or don't we wanna do those modifications?

Comment 10

[cmtalbert]

(In reply to comment #9)
> (In reply to comment #8)
> > >+var assertArrayContains = function(array, value, comment) {
> > I'm ok with adding this as a separate function.
> 
> Could we consider to add this as a new member to the Array class, i.e.
> Array.prototype.contains()? Or don't we wanna do those modifications?
No, I don't want our test assertion code modifying prototypes of JavaScript objects.

Comment 11

[Marc-Aurèle DARCHE]

OK not to have this addition inside the 1.4.2 release especially if the assertEquals function is to be modified.

I'll do all the requested points.

Comment 12

[Marc-Aurèle DARCHE]

Attachment: Patch against mozmill JUM to add array support to assertEquals and assertArrayContains

This patch is against the "master" branch.

This patch addresses all the points that you have raised.

The tests have been tested with the development version of mozmill itself.

Comment 13

[Marc-Aurèle DARCHE]

Also a note, to both Clint and Henrik: There is definitely a duplicate definition of the "assertNull" method in http://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/jum.js . This happens at line 136.

Comment 14

[cmtalbert]

Comment on attachment 474085 [details] [diff] [review]
Patch against mozmill JUM to add array support to assertEquals and assertArrayContains

Sorry this took so long to review.  I remember making comments on this patch, but it seems like they never landed into bugzilla.  :/ 


>diff --git a/mozmill/mozmill/extension/resource/modules/jum.js b/mozmill/mozmill/extension/resource/modules/jum.js
>old mode 100644
>new mode 100755
>index a2f625a..fe00058
>--- a/mozmill/mozmill/extension/resource/modules/jum.js
>+++ b/mozmill/mozmill/extension/resource/modules/jum.js
>@@ -20,6 +20,7 @@
> // 
> // Contributor(s):
> //  Mikeal Rogers <[e-mail]>
>+//  M.-A. Darche (array support in assertEquals, and assertArrayContains)
Please just put your name and email address.  It's not sustainable to list every person's contributions in the license header, and it's also going to break our scripts that we use to pull out contributors and/or change/update licenses in the future.

>@@ -14,6 +15,8 @@ var testAsserts = function() {
>   jum.assertNotUndefined('asdf');
>   jum.assertNaN('a');
>   jum.assertNotNaN(4);
>+  jum.assertArrayContains(['a', 'k', 9, 25], 'a');
>+  jum.assertArrayContains(['a', 'k', 9, 25], 9);
>   jum.pass();
> }
> testAsserts.meta = {'litmusids':[2345678]}
>@@ -48,6 +51,11 @@ var testNotAsserts = function() {
>   jum.assertFalse(true);
>   jum.assertFalse('asdf');
>   jum.assertEquals('asdf', 'fdsa');
>+  jum.assertEquals(['a', 'k', 9, 25], ['a', 'k', 9]);
>+  jum.assertEquals(['a', 'k', 9, new Object()], ['a', 'k', 9, new Object()]);
>+  jum.assertEquals(['a', 'k', 9, new Object()], new Object());
>+  jum.assertEquals([1], 1);
>+  jum.assertEquals([1], ['1']);
>   jum.assertNotEquals('asdf', 'asdf');
>   jum.assertNull(true);
>   jum.assertNotNull(null);
I'd also like to see a test for arrayContains where the array contains an object.  Just to cover the bases since that's a simple thing.

Also, if you wouldn't mind a little bit of scope creep here, would you do us a favor and remove the duplicated assertNull function as part of this patch or as a quick second patch on this bug? (whichever is easier).  I think it was a mistake of patch applying or change management that ended up giving us two copies of that function.

Thanks again for the patch and I apologize about how long it took to get to this review. 

r+ with those changes.

Comment 15

[Marc-Aurèle DARCHE]

Attachment: Patch against mozmill JUM to add array assertions (also removes assertNull duplicate declaration)

This patch addresses all the raised points. The duplicate assertNull declaration is removed.

The extra unit tests that you asked me were (of course) really necessary, they helped me fixed some typos I wrongly introduced in the last patch last time and that I didn't test well (forgot to redo a reinstallation of the modified mozmill before retesting it).

So, normally we're done with this ticket at last :-)

Comment 16

[cmtalbert]

Comment on attachment 480376 [details] [diff] [review]
Patch against mozmill JUM to add array assertions (also removes assertNull duplicate declaration)

I'm glad the extra unit tests proved to be useful!

This looks really great, there is only one small change that I'll make before checking it in, and I'll note that below.

Thanks again for another patch!

>diff --git a/mozmill/mozmill/extension/resource/modules/jum.js b/mozmill/mozmill/extension/resource/modules/jum.js
>+    if (value1.length != value2.length) {
>+      frame.events.fail({'function':'jum.assertEquals', 'comment':comment,
>+                         'message':"The arrays have not the same length",
>+                         'value1':ifJSONable(value1), 'value2':ifJSONable(value2)});
Should read: 'message':"The arrays do not have the same length",
Just more correct usage of English.  Don't worry about submitting another patch for that, I'll just change it before I check it in.

Thanks again!

Comment 17

[Marc-Aurèle DARCHE]

So could you actually check this patch in please? :-)

Comment 18

[cmtalbert]

(In reply to comment #17)
> So could you actually check this patch in please? :-)
I'm so sorry.  Your patch fell through the cracks. Please ping me earlier when this kind of stuff happens.  It's landed now.  Again I apologize.

https://github.com/mozautomation/mozmill/commit/ab79297496854b4246535c0497874c0e29da6994

Comment 19

[Marc-Aurèle DARCHE]

(In reply to comment #18)
> (In reply to comment #17)
> > So could you actually check this patch in please? :-)
> I'm so sorry.  Your patch fell through the cracks. Please ping me earlier
> when this kind of stuff happens.  It's landed now.  Again I apologize.
> 
> https://github.com/mozautomation/mozmill/commit/
> ab79297496854b4246535c0497874c0e29da6994

Thanks a lot for the landing :-)

May I ask if it would be possible to have the needed landing so that the incoming Mozmill 1.5.4 can contain this new functionality too?

Mozmill 1.5.4 has been mentioned here: http://groups.google.com/group/mozmill-dev/browse_thread/thread/e8732d838ef2ed43?hl=en

This new functionality has all the needed unit tests and we can be very confident with it and could be used to get rid of /mozmill-tests/tests/addons/[e-mail]/lib/testArrayUtilsAPI.js as well as simplifying the code involved in bug 570493.

Comment 20

[Marc-Aurèle DARCHE]

(In reply to comment #19)
> (In reply to comment #18)
> > 
> > https://github.com/mozautomation/mozmill/commit/ab79297496854b4246535c0497874c0e29da6994
> 
> Thanks a lot for the landing :-)
> 
> This new functionality has all the needed unit tests and we can be very
> confident with it and could be used to get rid of
> /mozmill-tests/tests/addons/[e-mail]/lib/testArrayUtilsAPI.js as
> well as simplifying the code involved in bug 570493.

Again, why not put this feature in the next stable beta? It should really not changed any behavior.

Comment 21

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

Please see my mailing list post:
http://groups.google.com/group/mozmill-dev/browse_thread/thread/c2cefe1aee569f0d

It's now available in our mozmill-tests repository and can be used with Mozmill 1.5.x.

Comment 22

[Marc-Aurèle DARCHE]

(In reply to comment #21)
> Please see my mailing list post:
> http://groups.google.com/group/mozmill-dev/browse_thread/thread/
> c2cefe1aee569f0d
> 
> It's now available in our mozmill-tests repository and can be used with
> Mozmill 1.5.x.

OK. I didn't understand that I should be interested in this assertion module when I've read your message in the mailing list.

I've read the module code now.

And I can tell you that the code of the assertion module in the default branch as of now doesn't work with arrays:
assert.equal(["a", "k", 9, 25], ["a", "k", 9, 25]); is currently failing, while we agreed that it should pass. The work done in the proposed patch has not been integrated in the assertion module.

Comment 23

[Marc-Aurèle DARCHE]

(In reply to comment #21)
> Please see my mailing list post:
> http://groups.google.com/group/mozmill-dev/browse_thread/thread/
> c2cefe1aee569f0d
> 
> It's now available in our mozmill-tests repository and can be used with
> Mozmill 1.5.x.

Would that be possible to have this assertion module in the mozmill-tests aurora branch please?

Comment 24

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

Oh I see. I have read the summary of this bug too quickly. Sorry for that. So lets get a bug filed to extend the assertions module with the functions you have had added here to the JUM module. Not sure if we will have a 1-1 mapping but it would be helpful to have them. I simply missed that bug.

Comment 25

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

(In reply to comment #23)
> Would that be possible to have this assertion module in the mozmill-tests
> aurora branch please?

It's available on all of our branches from default to mozilla-release. Just pull and update the repository.

Comment 26

[Marc-Aurèle DARCHE]

(In reply to comment #25)
> (In reply to comment #23)
> > Would that be possible to have this assertion module in the mozmill-tests
> > aurora branch please?
> 
> It's available on all of our branches from default to mozilla-release. Just
> pull and update the repository.

My time to be sorry: I was checking the "aurora" branch and not the "mozilla-aurora" branch as explained in https://developer.mozilla.org/en/Mozmill_Tests#Handling_branches

My mistake comes from that I was only taking this message of yours as a reference:
http://groups.google.com/group/mozmill-dev/browse_thread/thread/55fcd8b832c808c7

Sorry!