565765 - test.assertEqual should use ===

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Type coercion is bad when asserting things are equal.

I was using it to test undefined === null (expecting assertion failure, but no...)

Thoughts?

(FWIW, making the change doesn't break any current tests)

Comment 1

[Atul Varma [:atul]]

Thanks for pointing this out, I definitely agree. Can you make the change and push it?

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Hmm looks like I lied and this is causing some tests to fail... I'll investigate, will probably have to change a couple tests for correctness.

Currently blocking bug 565767 just because I want to do that test the right way.

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

Soooo what I did...

* assertEqual and assertNotEqual now use === & !== for equality checks.
* added assertEqualish to cover the tests that check for ==
  (this could use a better name. I wanted to use 'assertEqualy' but that's too close... suggestions welcome)
* updated tests that were expecting == to use assertEqualish
* updated test-nsJetpack.js to use proper assert* instead of just assert(a === b)
* updated test-context-menu to do the same

Atul - since most of the test changes are in nsJetpack, you might want to give those a closer look than you would usually. I'm pretty sure I haven't, but I want to make sure I haven't changed any of the test logic.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Additionally, grep told me there were a few tests in packages/jetpack-core/tests/interoperablejs-read-only/compliance/ that were also using the assert(a === b) style, but I wasn't sure if those were ok to change (since it says read-only in the path). Say the word and I can change those too.

Comment 5

[Atul Varma [:atul]]

Hmm, so I'm not a big fan of the assertEqualish(), as it seems to make the testing API more complex than it ought to be. But maybe I'm wrong.

One of the testing APIs that I'd like to maintain some level of "compatibility" with is QUnit, which--the last I heard--was to be CommonJS' standard testing API, and which seems to be the de facto standard for many web projects, possibly because its API is really simple but effective. It has equals() test using ==, and same() test recursively using ===.  So it looks like, if we want to maintain a somewhat compatible API, we would keep assertEqual() as-is and create a new function called assertSame() which would recursively compare using ===. Does that make any sense?

Comment 6

[Atul Varma [:atul]]

Comment on attachment 445730 [details] [diff] [review]
Patch v0.1

At the very least I am going to r- this for now, because we really need to figure out a better name than assertEqualish(). :P

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

From IRC discussion, I'm going to go with assert(Not)Identical for === and leave assert(Not)Equal as is.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

Now uses assert[Not]Identical and leaves assertEqual alone.

Comment 9

[Atul Varma [:atul]]

Comment on attachment 445974 [details] [diff] [review]
Patch v0.2

Thanks. Are you getting any JS warnings when running the nsjetpack tests? Right now I'm getting 4 warnings of 'reference to undefined property wrapped[name]' and one warning referring to an undefined prop at 'unwrap(wrapped).blarg' in test-legacy-nsjetpack, lines 179 and 240 respectively. Since the test runner doesn't tolerate even JS strict warnings, this is causing the test suite to fail.

Also, I noticed that assertIdentical() isn't making recursive comparisons the way that qunit's same() does. I'm actually unclear on why qunit bothers to do this, since I'd think that if a === b, any sub-parts of a and b would be ===, but I probably don't know as much about this as I ought to.

Other than that, this looks good.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #9)
> Also, I noticed that assertIdentical() isn't making recursive comparisons the
> way that qunit's same() does. I'm actually unclear on why qunit bothers to do
> this, since I'd think that if a === b, any sub-parts of a and b would be ===,
> but I probably don't know as much about this as I ought to.

This article <http://philrathe.com/articles/equiv> explains more, but the summary is that qunit doesn't do a === b, it does something like |for (x in a) a[x] === b[x]|, except a lot more complicated and particular (f.e. two instances of the same constructor function are apparently the same).  So it's nothing like the simple reference equivalence check we're doing here.

Comment 11

[Atul Varma [:atul]]

Ack. I guess we can come up with an assertRecursivelyIdentical() that does the same thing as same() later, or something?

Comment 12

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #11)
> Ack. I guess we can come up with an assertRecursivelyIdentical() that does the
> same thing as same() later, or something?

Sure, if we discover that we have an actual use for it, we can always implement it later, although I recommend we call it assertSame (either that, or assertRecursivelyIdenticalWhereIdenticalIsAWordYouKeepUsingThatDoesNotMeanWhatYouThinkItMeans).

Comment 13

[Atul Varma [:atul]]

Are you suggesting we rename assertIdentical to assertIdenticalWhereIdenticalIsAWordYouKeepUsingThatDoesNotMeanWhatYouThinkItMeans?

Comment 14

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #13)
> Are you suggesting we rename assertIdentical to
> assertIdenticalWhereIdenticalIsAWordYouKeepUsingThatDoesNotMeanWhatYouThinkItMeans?

No!  I'm suggesting that name as an alternative to assertSame, if we ever implement a qunit same-like test.

Comment 15

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Soo what's the deal then? Keep it assertIdentical (which doesn't mean recursively identical) or just say f--- it and forget about this bug until it really becomes an issue that we really need to test === recursively? In reality, using |assert(a === b)| isn't that bad.

At this point, I'm thinking we should just forget about this for now.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

IMHO people mostly want ===, whether they know it or not.  I suspect that's true for qunit as well, so rather than try to keep assertEqual equivalent to qunit's "equal" (and add yet another test method for ===), we should simply make assertEqual support the most common use case of ===, as this bug originally suggested, and then advocate that qunit fix its "equal" implementation to do the same.

Comment 17

[Atul Varma [:atul]]

Oh, my bad re: QUnit being the CommonJS standard... it looks like they've resolved some of the confusion in the Unit Testing 1.0 spec:

  http://wiki.commonjs.org/wiki/Unit_Testing/1.0

So they've got assert.equal(), which tests ==, assert.deepEqual(), which appears to be the same as qunit's same(), and assert.strictEqual(), which does ===.

Since this is commonJS it would be phat to be at least somewhat aligned with it.

Comment 18

[Atul Varma [:atul]]

So it sounds like adding test.assertStrictEqual() might be the way to go here, and then add test.assertDeepEqual() if/when we need it.  This way there's a really simple, non-confusing mapping to the CommonJS standard.

Comment 19

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Paul: might you be able to finish this up?

Comment 22

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

It's been a while since I looked at any of this, but I can wrap this up.

Just to make sure we're all on the same page, here's what needs to be done:

* s/assert(Not)?Identical/assert(Not)?StrictEqual/ in current patch
* find added uses of assert(a === b) and assert(a !== b) and replace those.

Comment 23

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/162

Pointer to Github pull-request

Comment 24

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/162#

Pointer to Github pull-request

Comment 25

[Alexandre Poirot [:ochameau]]

Comment on attachment 531814 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/162#

This patch is still up to date. All existing test.assert( a === b ) or test.assert( a !== b ) have been replaced and cfx testall is green.
So we only need a review.

Comment 26

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I think it will be better if we'll reuse code that is already used internally by adding `assert` property to test object:

test.assert = require("test/assert").Assert(test);

this will allow writing tests as follows:

exports.testFoo = function({ assert, done }) { // done is not always necessary
   assert.equal(a, b, 'a == b');
   assert.strictEqual(d, c, 'd === c');
};

Which is btw also makes syntax more concise.

Comment 27

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 531814 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/162#

Looks good, r=myk.


(In reply to comment #26)
> I think it will be better if we'll reuse code that is already used
> internally by adding `assert` property to test object:

Perhaps, but that's a broader change that we should consider separately.

Comment 28

[Myk Melez [:myk] [@mykmelez]]

Change:

https://github.com/mozilla/addon-sdk/commit/c38d70538d5e240fffa03025dd27de6b7a901a37

Merge:

https://github.com/mozilla/addon-sdk/commit/7652a2723fc8bf31a4e0882c6faf806baed345fc

Thanks for the fix!