671290 - Implement assertion methods to support arrays (and other objects)

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Marc-Aurèle DARCHE]

assert.equal(["a", "k", 9, 25], ["a", "k", 9, 25]); is currently failing, while we agreed in bug 568978 that it should pass.

Comment 1

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

Shane, as far as I can remember our talk on IRC from last week, you have already started to create some more members to the Expect and Assert class. Could you give us some more details? That would be great.

Comment 2

[Heather Arthur [:harth]]

I will put back in assert.deepEqual() from the CommonJS module.

Comment 3

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

Thanks Heather. That's a great idea. Would we need something else too or would that cover all object related assertions?

Comment 4

[Heather Arthur [:harth]]

It would cover arrays and objects. http://nodejs.org/docs/v0.3.1/api/assert.html#assert.deepEqual

Comment 5

[Shane Tomlinson [:stomlinson]]

Henrik, I think that the deepEqual will cover the case Marc has pointed out up there.  Last week I did write up some new assertions for mozmill/mozmill/extension/resource/assertions.js, these were isNumber, isFunction, isString, isArray and isObject.  I wrote up some tests as well in mutt/mutt/tests/js/test_assertions.js.  I will create a pull request for you to review.

Comment 6

[Shane Tomlinson [:stomlinson]]

Pull request issued at https://github.com/mozautomation/mozmill/pull/7

Comment 7

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

Thanks Shane. In the Mozmill project we normally work with patches attached to bugs. Could you create a diff and attach it here? Ctalbert or Heather would be the ones to review it.

Comment 8

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

Oh, Jeff has already been filed that as bug 671367.

Comment 9

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

Clint, I have known I missed something yesterday. This request is up for a while and with the removal of JUM we need a replacement in the new assertions module. Requesting for Mozmill 2.0.

Comment 10

[cmtalbert]

We addressed the pull request and wontfixed it because we didn't want to get back to a place where we have 30 specialized assertion functions.  That was what we were trying to move *away* from.  Harth did mention on bug 671367 about taking the rest of the commonJS assertion module which will probably get us what we need here.  So, I commented there to ask about that.  We can morph this bug into the vehicle for that change or we can file a new one and close this one as the specific issue here has been handled.

Comment 11

[Heather Arthur [:harth]]

Attachment: add deepEqual and notDeepEqual

patch adds assert.deepEqual() and assert.notDeepEqual to compare objects and arrays, as well as a few tests for both.

Comment 12

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

Comment on attachment 556761 [details] [diff] [review]
add deepEqual and notDeepEqual

Just a drive-by feedback:

>+  deepEqual: function deepEqual(actual, expected, aMessage) {

Can you please add documentation to both of the new methods? 

>+    let condition = this._deepEqual(actual, expected);
>+    let diagnosis = "got '" + JSON.stringify(actual) + "', not expected '"
>+      + JSON.stringify(expected) + "'";

deepEqual should use a message like 'got JSON(actual), expected JSON(expected)'. 'not expected' is being used in this module only for the not methods.

>+     let diagnosis = JSON.stringify(actual) + " and '"
>+       + JSON.stringify(expected) + "' shouldn't be equal";

Same as above. This should be 'got JSON(actual), not expected JSON(expected)'

>+  // The following deepEquals implementation is from Narwhal under this license:

Not sure how to best include other licenses in our modules. Doesn't it mean that everything below that lines would be covered by that license? Shouldn't we better move external code out to a separate file?

>+++ b/mozmill/mozmill/extension/resource/modules/frame.js
>@@ -108,7 +108,8 @@ var loadFile = function(path, collector) {
>     Cr: Components.results,
>     log: log,
>     assert: new assertions.Assert(),
>-    expect: new assertions.Expect()
>+    expect: new assertions.Expect(),
>+    Expect: assertions.Expect

I thought we didn't wanted to export the classes? Something we have reverted recently because it's an unusual case and never needed by tests. So I think you are working on an older version of the module which adds this back now?

Comment 13

[Marc-Aurèle DARCHE]

(In reply to Henrik Skupin (:whimboo) from comment #12)
> Comment on attachment 556761 [details] [diff] [review]
> 
> >+  // The following deepEquals implementation is from Narwhal under this license:
> 
> Not sure how to best include other licenses in our modules. Doesn't it mean
> that everything below that lines would be covered by that license? Shouldn't
> we better move external code out to a separate file?
> 

Putting code with a different license (but of course compatible) in a different file is what I would advise too.

According to the license, it seems possible to put this license statement "in the Software", where "the Software" can be the "mozmill" code directories and not specifically in this particular "assertions.js" file (so concretely putting a specific note stating the Narwhal license in a separate file should be correct). But the problem with that approach is that the link with the license will eventually be lost after some people simply copy code from the assertions.js file without reading all the other files in the other directories.

Comment 14

[Jeff Hammel]

Comment on attachment 556761 [details] [diff] [review]
add deepEqual and notDeepEqual

Looks basically good to me.  The 3rd party functions are a bit more complicated than I would have gone with, but...I guess that's good? I also wonder if this is a step back towards the path of assertion hell, versus just providing a deepEqual function, e.g. assert(not(deepEqual)).  We can reconsider this now, or later, with the caveat that in the future tests will use this API.  So...any thoughts on this issue?  

Also, as Henrik points out, these functions should have documentation.  I'm not a lawyer so I can't really advise on that side of thing.

r+ with fixes, I think ;) But it would be nice to have one function for deepEqual versus deepEqual and notDeepEqual

Comment 15

[Jeff Hammel]

Oh yeah, and why *is* Expect exported?

Comment 16

[Heather Arthur [:harth]]

(In reply to Jeff Hammel [:jhammel] from comment #14)
> Comment on attachment 556761 [details] [diff] [review]
> add deepEqual and notDeepEqual
> 
> Looks basically good to me.  The 3rd party functions are a bit more
> complicated than I would have gone with, but...I guess that's good? I also
> wonder if this is a step back towards the path of assertion hell, versus
> just providing a deepEqual function, e.g. assert(not(deepEqual)).  We can
> reconsider this now, or later, with the caveat that in the future tests will
> use this API.  So...any thoughts on this issue?  
> 

node has deepEqual() and notDeepEqual() and other assertion functions have a "not" counterpart. I think it's useful, otherwise would have to be:

assert.ok(othermod.deepEqual(x, y))
assert.ok(!othermod.deepEqual(x, y))

Maybe that's okay, if we end up making a module (similar to underscore http://documentcloud.github.com/underscore/ with isEqual() and isNumber()), but for right now, I think it's pretty generic and along the lines of equal()


> Also, as Henrik points out, these functions should have documentation.  I'm
> not a lawyer so I can't really advise on that side of thing.
> 

True, I'll add a comment similar to the other assertion functions.


Expect is exported for the test_assertions.js test, so we can override the error function so it doesn't report expected fails as real fails.

Comment 17

[Heather Arthur [:harth]]

master:
https://github.com/mozautomation/mozmill/compare/a883ac3...0058c8f

Comment 18

[Marc-Aurèle DARCHE]

(In reply to Heather Arthur [:harth] from comment #17)
> master:
> https://github.com/mozautomation/mozmill/compare/a883ac3...0058c8f

Thanks a lot!

Please, will the fix be present in the next Mozmill "stable" release (the one just following Mozmill 1.5.4) ? I'm asking because I have created this ticket for this purpose, and not as a goal for the future Mozmill 2.0, and the last time my work on array equality assertions was targeted for Mozmill 2.0 and then never reached the stable release. Thanks again.

Comment 19

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

(In reply to Heather Arthur [:harth] from comment #16)
> Expect is exported for the test_assertions.js test, so we can override the
> error function so it doesn't report expected fails as real fails.

No, please don't do that. It's the only file which really subclasses the Expect class. The test already requires the assertion module. So there is absolutely no need to add the Expect class to the module scope. Can you please fix that, Heather? Thanks.

Comment 20

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

(In reply to Marc-Aurèle DARCHE from comment #18)
> Please, will the fix be present in the next Mozmill "stable" release (the
> one just following Mozmill 1.5.4) ? I'm asking because I have created this

No, it will not be backported. But I will take care of it for our shared modules on bug 683294.

Comment 21

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

I don't want that slips through. It's a blocker for Mozmill 2, so it should be marked as such.

Comment 22

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

(In reply to Henrik Skupin (:whimboo) from comment #12)
> >+    let condition = this._deepEqual(actual, expected);
> >+    let diagnosis = "got '" + JSON.stringify(actual) + "', not expected '"
> >+      + JSON.stringify(expected) + "'";
> 
> deepEqual should use a message like 'got JSON(actual), expected
> JSON(expected)'. 'not expected' is being used in this module only for the
> not methods.
[..]
> >+     let diagnosis = JSON.stringify(actual) + " and '"
> >+       + JSON.stringify(expected) + "' shouldn't be equal";
> 
> Same as above. This should be 'got JSON(actual), not expected JSON(expected)'

Those comments have not been addressed yet. The code still contains the wrong messages.

> >+  // The following deepEquals implementation is from Narwhal under this license:
> 
> Not sure how to best include other licenses in our modules. Doesn't it mean
> that everything below that lines would be covered by that license? Shouldn't
> we better move external code out to a separate file?

What about the licensing issue? I haven't seen anything which clarifies how we want to do it.

> >+    Expect: assertions.Expect

Again, not necessary.

Comment 23

[Heather Arthur [:harth]]

https://github.com/mozautomation/mozmill/commit/598571ea772339704167ca3e180d325f6e9652b9

Comment 24

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

Still not fixed even with the second follow-up push:

https://github.com/mozautomation/mozmill/commit/90c9cf040e41afcdc4280d9b3d0aeeed3fae0e16


Can we please stick with finishing up a discussion for topics before pushing code to the repository in small pieces? The question re: licenses has still not been answered. I don't want to nitpick but I don't think we can close this bug out that easily.

Comment 25

[Heather Arthur [:harth]]

(In reply to Henrik Skupin (:whimboo) from comment #24)

With regards to licencing, I don't want to seperate it into a separate file. The not before the license says it applies to the _deepEquals() code only, I think that's sufficient.

Comment 26

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Heather, the block at the top of file says the whole file is MPL (with the usual exceptions about releasing as GPL, etc.)

To that point, the file can no longer be released as GPL unless the Narwhal section is redacted due to the copyright clause (no additional requirements allowed by GPL).

I have no idea what our policy overall is on all this since to my knowledge, we're required to tri-license everything we release, but I'd recommend a separate file if only for the conflicting license issue. 

At the very least, I think we'd need to amend the MPL block at the top to exclude the Narwhal code, and that strikes me as risky. A separate file may be a momentary hassle, but seems like the safe option.

Comment 27

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Actually, scratch the copyright argument. I think GPL does allow that particular requirement.

But I'd still recommend separating the files for safety's sake due to the overall conflict with MPL saying it applies to the whole file.

Comment 28

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

Gerv, could you please advice how we have to embed external code which isn't licensed under the GPL? Should we as best move it out to an external file?

Comment 29

[Gervase Markham [:gerv]]

The code in question seems to be under an MIT licence, which is both MPL, LGPL and GPL-compatible. So there is no legal problem copying it into a tri-licensed file in the way you have done here. The default effect would be that any changes you make to that code would be tri-licensed, and so could not be sent back for integration by the original developer without the permission of the person who wrote the patch.

If that's not a problem, leave things as-is. If it is a problem, best to pull it out into a separate file, with a clear licensing statement at the top.

Gerv

Comment 30

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

Thanks Gerv. Heather, please follow-up on it.

Beside that the following code still has to be fixed:

> >+     let diagnosis = JSON.stringify(actual) + " and '"
> >+       + JSON.stringify(expected) + "' shouldn't be equal";
> 
> Same as above. This should be 'got JSON(actual), not expected JSON(expected)'

Comment 31

[Heather Arthur [:harth]]

(In reply to Henrik Skupin (:whimboo) from comment #30)
> Thanks Gerv. Heather, please follow-up on it.

Yes, thanks Gerv! I will leave it in the file, I doubt we will ever upstream changes to the function.

> 
> Beside that the following code still has to be fixed:
> 
> > >+     let diagnosis = JSON.stringify(actual) + " and '"
> > >+       + JSON.stringify(expected) + "' shouldn't be equal";
> > 
> > Same as above. This should be 'got JSON(actual), not expected JSON(expected)'

'got {}, not expected {}' doesn't work in this case because it's assert.notDeepEqual(), I'm not sure what other wording you want, maybe 'got {}, shouldn't equal {}' ?

Comment 32

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

That's exactly what we have for notEqual:
https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/assertions.js#L197

Why does the same wording not work? We have two objects here too which have to be different. It should simply be the negative description in the second part as what we have for deepEqual.

Comment 33

[Heather Arthur [:harth]]

(In reply to Henrik Skupin (:whimboo) from comment #32)
> That's exactly what we have for notEqual:
> https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/
> extension/resource/modules/assertions.js#L197
> 
> Why does the same wording not work? We have two objects here too which have
> to be different. It should simply be the negative description in the second
> part as what we have for deepEqual.

It doesn't make much sense. "got x, not expected y" reads like "got x, not the expected y", we need to take the word "expected" out.

Comment 34

[Jeff Hammel]

Since the patch is already landed, is there anything at this point that shouldn't be addressed as a follow up? If we're not going to backout the patch or if there aren't functional deficiencies that require addressing, I would prefer to close and move on doing any sort of e.g. wording changes as follow-ups.

Comment 35

[cmtalbert]

I'm with Jeff.  This is a follow up bug and if the wording bothers you that much, make a patch and we'll debate it there.  The wording is fine, we don't have automation that parses the lexical statements here, and we if we do have automation that parses lexical log messages then we have already failed.

This bug is done.  File follow on issues.

Comment 36

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

Works quite well. I followed up with bug 696484. Marking this bug as verified fixed.