Closed
Bug 565765
Opened 14 years ago
Closed 13 years ago
test.assertEqual should use ===
Categories
(Add-on SDK Graveyard :: General, defect, P1)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
1.0
People
(Reporter: zpao, Assigned: zpao)
References
Details
Attachments
(3 files, 1 obsolete file)
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•14 years ago
|
||
Thanks for pointing this out, I definitely agree. Can you make the change and push it?
Assignee | ||
Comment 2•14 years ago
|
||
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
Assignee | ||
Comment 3•14 years ago
|
||
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 | ||
Comment 4•14 years ago
|
||
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•14 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•14 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-
Assignee | ||
Comment 7•14 years ago
|
||
From IRC discussion, I'm going to go with assert(Not)Identical for === and leave assert(Not)Equal as is.
Assignee | ||
Comment 8•14 years ago
|
||
Now uses assert[Not]Identical and leaves assertEqual alone.
Attachment #445730 -
Attachment is obsolete: true
Attachment #445974 -
Flags: review?(avarma)
Comment 9•14 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-
Comment 10•14 years ago
|
||
(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•14 years ago
|
||
Ack. I guess we can come up with an assertRecursivelyIdentical() that does the same thing as same() later, or something?
Comment 12•14 years ago
|
||
(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•14 years ago
|
||
Are you suggesting we rename assertIdentical to assertIdenticalWhereIdenticalIsAWordYouKeepUsingThatDoesNotMeanWhatYouThinkItMeans?
Comment 14•14 years ago
|
||
(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.
Assignee | ||
Comment 15•14 years ago
|
||
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•14 years ago
|
||
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•14 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•14 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.
Comment 19•14 years ago
|
||
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
Comment 20•13 years ago
|
||
Paul: might you be able to finish this up?
Priority: -- → P1
Whiteboard: [triage:followup]
Target Milestone: --- → 1.0
Updated•13 years ago
|
Whiteboard: [triage:followup]
Assignee | ||
Comment 22•13 years ago
|
||
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.
Assignee | ||
Comment 23•13 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Comment 24•13 years ago
|
||
Pointer to Github pull-request
Comment 25•13 years ago
|
||
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)
Comment 26•13 years ago
|
||
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•13 years ago
|
||
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+
Comment 28•13 years ago
|
||
Change: https://github.com/mozilla/addon-sdk/commit/c38d70538d5e240fffa03025dd27de6b7a901a37 Merge: https://github.com/mozilla/addon-sdk/commit/7652a2723fc8bf31a4e0882c6faf806baed345fc Thanks for the fix!
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•