test.assertEqual should use ===

RESOLVED FIXED in 1.0

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: zpao, Assigned: zpao)

Tracking

unspecified

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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

8 years ago
Thanks for pointing this out, I definitely agree. Can you make the change and push it?
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.
Blocks: 565767
Created attachment 445730 [details] [diff] [review]
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.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #445730 - Flags: review?(avarma)
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

8 years ago
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

8 years ago
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
Attachment #445730 - Flags: review?(avarma) → review-
From IRC discussion, I'm going to go with assert(Not)Identical for === and leave assert(Not)Equal as is.
Created attachment 445974 [details] [diff] [review]
Patch v0.2

Now uses assert[Not]Identical and leaves assertEqual alone.
Attachment #445730 - Attachment is obsolete: true
Attachment #445974 - Flags: review?(avarma)

Comment 9

8 years ago
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.
Attachment #445974 - Flags: review?(avarma) → review-
(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

8 years ago
Ack. I guess we can come up with an assertRecursivelyIdentical() that does the same thing as same() later, or something?
(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

8 years ago
Are you suggesting we rename assertIdentical to assertIdenticalWhereIdenticalIsAWordYouKeepUsingThatDoesNotMeanWhatYouThinkItMeans?
(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.
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.
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

8 years ago
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

8 years ago
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.
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".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
Paul: might you be able to finish this up?
Priority: -- → P1
Whiteboard: [triage:followup]
Target Milestone: --- → 1.0
Duplicate of this bug: 569273
Whiteboard: [triage:followup]
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.
Created attachment 531813 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/162

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

Pointer to Github pull-request
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.
Attachment #531814 - Flags: review?(myk)
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 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.
Attachment #531814 - Flags: review?(myk) → review+
You need to log in before you can comment on or make changes to this bug.