Closed
Bug 1070681
Opened 10 years ago
Closed 10 years ago
[communications] unit test refactoring for new version chai.assert api
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Firefox OS Graveyard
Gaia::Contacts
Tracking
(Not tracked)
RESOLVED
FIXED
2.1 S7 (24Oct)
People
(Reporter: sv99, Unassigned)
References
Details
Attachments
(2 files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0 Build ID: 20140920030206 Steps to reproduce: test_agent used very old version chai library version=0.5.3 in the current version=1.9.1 deepEqual take prototype into consideration - problem with tone.url contacts/test/unit/views/details_test.js 650: assert.include(dom.innerHTML, contact.photo[0]); in old version chaijs this assert do nothing, in new version - fail!
Comment 2•10 years ago
|
||
Comment on attachment 8492700 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24244 Redirecting to peers for Contacts and Dialer. Hey Francisco, Anthony, the goal here is to make the tests ready for updating chai to a newer version. Thanks !
Attachment #8492700 -
Flags: review?(francisco)
Attachment #8492700 -
Flags: review?(felash)
Attachment #8492700 -
Flags: review?(anthony)
Comment 3•10 years ago
|
||
Comment on attachment 8492700 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24244 Why are we adding all those try/catches? When I'm testing with the newer chai, I'm getting an error directly.
Attachment #8492700 -
Flags: review?(anthony)
without try catch if you have failed test then we have uncaught exception - no help stack trace. This is from chaijs site async template
Comment 5•10 years ago
|
||
You can use this (which works only in our test-agent thanks to an helper): done(function() { // assertions }); Then the assertions are correctly caught inside the helper and the stack trace is displayed correctly.
Comment 6•10 years ago
|
||
Comment on attachment 8492700 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24244 Can you try with Julien's suggestion in comment 5? Thx!
Attachment #8492700 -
Flags: review?(francisco)
some test change, Julien suggestion not totally equivalent, some test after change not worked!!
Comment 8•10 years ago
|
||
It should be totally equivalent :) see [1] for the implementation. Please try with it and we can then discuss why you still have issues. [1] https://github.com/mozilla-b2g/gaia/blob/fc44f9912f2cb9f33f1117e30c713121a753b486/dev_apps/test-agent/common/test/mocha_generators.js#L36-L48
i try rewrite apps/communications/contacts/test/unit/utilities/ice_data_test.js all rewrite except 2 tests 67: test('> Data sent to DS is valid', function(done) { subject.load().then(function() { subject.setICEContact(2, 1, true).then(function(data) { try { assert.lengthOf(data, 2); done(); } catch (err) { done(err); } }); }, done); }); }); second promise first variant done in first promise: test('> Data sent to DS is valid', function(done) { subject.load().then(done(function() { subject.setICEContact(2, 1, true).then(function(data) { assert.lengthOf(data, 2); // any value test ok }); }), done); }); }); first variant done in first promise: test('> Data sent to DS is valid', function(done) { subject.load().then(function() { subject.setICEContact(2, 1, true).then(done(function(data) { assert.lengthOf(data, 2); })); }, done); }); }); when set 2->3 this write TypeError: this.obj is undefined!! 150: test('> ICE contact removed', function(done) { var changeCallback; sinon.stub(document, 'addEventListener', function(name, cb) { changeCallback = cb; }); subject.listenForChanges(function() { try { assert.isFalse(subject.iceContacts[0].id === 1); assert.isFalse(subject.iceContacts[0].active); done(); } catch (err) { done(err); } }); changeCallback({ detail: { contactID: 1, reason: 'remove' } }); document.addEventListener.restore(); }); when i rewrite it as subject.listenForChanges(done(function() { assert.isFalse(subject.iceContacts[0].id === 1); assert.isFalse(subject.iceContacts[0].active); })); not change assert -> Error: expected true to be false at chaiAssert (app://communications.gaiamobile.org/common/test/helper.js:31:1) at get (app://communications.gaiamobile.org/common/vendor/chai/chai.js:393:1) at isFalse (app://communications.gaiamobile.org/common/vendor/chai/chai.js:1385:5) at (anonymous) (app://communications.gaiamobile.org/contacts/test/unit/utilities/ice_data_test.js:157:11) at (anonymous) (app://communications.gaiamobile.org/common/test/mocha_generators.js:40:15) at (anonymous) (app://communications.gaiamobile.org/contacts/test/unit/utilities/ice_data_test.js:156:32)
Reporter | ||
Comment 10•10 years ago
|
||
my PR have same repeated try catch, but worked! I have not change test logic - simply minimal rewrite it for working in both version chaijs
Comment 11•10 years ago
|
||
I think subject.listenForChanges(done(function() { assert.isFalse(subject.iceContacts[0].id === 1); assert.isFalse(subject.iceContacts[0].active); })); needs to be subject.listenForChanges(function() { done(function() { assert.isFalse(subject.iceContacts[0].id === 1); assert.isFalse(subject.iceContacts[0].active); }); }); (I haven't checked the code closely) The reason: in the first code, you're calling "done" _before_ listenForChanges is called. In the second code, we're calling "done" in the callback for listenForChanges. Hope it's clear enough :) Next time, it's easier if you publish the code on github on a separate branch and do a separate pull request, so that we can comment directly on the code :)
Attachment #8492700 -
Flags: review?(felash)
Reporter | ||
Comment 13•10 years ago
|
||
worked version first code subject.load().then(function() { subject.setICEContact(2, 1, true).then(function(data) { done(function() { assert.lengthOf(data, 22); }); }); }, done); Based on second code sugestion Thanks
Comment 14•10 years ago
|
||
Comment on attachment 8492700 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24244 Francisco is the peer :)
Attachment #8492700 -
Flags: review?(felash) → review?(francisco)
Comment 15•10 years ago
|
||
Hi, following the thread on bugzilla I though we were going to get ride of the try/catch statements, but still see them on the PR. Is going to be the only way of updating to a newer test agent?
Flags: needinfo?(sv99)
Comment 16•10 years ago
|
||
Comment on attachment 8492700 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24244 This will require review as well from a dialer peer.
Attachment #8492700 -
Flags: review?(anthony)
Comment 17•10 years ago
|
||
We can definitely remove all try/catch using the done() thingie. Please do it.
Reporter | ||
Comment 18•10 years ago
|
||
I remove all try/catch. Some test rewrite using done(). Problems with two level async test. Problem in both version try/catch and done() Catch and readable stack only for nearest assert. Second level assert -> throw not catched exception without stack trace, only test name!! Sample apps/communications/dialer/test/unit/call_log_db_test.js 129: CallLogDBManager.add(call, function(result) { CallLogDBManager.getGroupList(function(groups) { assert.equal(groups.length, 1); checkGroup(groups[0], call, call.date, 1, true, result); CallLogDBManager.getRecentList(function(recents) { assert.lengthOf(recents, 1); checkCall(recents[0], call); done(); }); }); });
Flags: needinfo?(sv99)
Comment 19•10 years ago
|
||
Yeah, the 2 levels callbacks is not easy to catch. One way is to put try/catch in the "first" nested callback. Another way is to do it manually (like: if (groups.length !== 1) { done(new Error('XXX')); return; }). Then this is not really better. Best would be to move to Promises :) But I think it's good enough to not catch these errors for now. What do you think?
Reporter | ||
Comment 20•10 years ago
|
||
For chai upgrade this is enough. Later may be add chai-as-promised plugin. in helper.js var chaiAsPromised = require("chai-as-promised"); chai.use(chaiAsPromised);
Comment 21•10 years ago
|
||
I mean, the application code itself should use Promises instead of callbacks. Using chai-as-promised is part of bug 991663, but of course we first need to upgrade chai :)
Comment 22•10 years ago
|
||
Comment on attachment 8492700 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24244 Now looking much better, executed the contacts tests, pretty fast btw :), and no clue of the try/catch sentences. r+ the contacts part, please wait till dialer review.
Attachment #8492700 -
Flags: review?(francisco) → review+
Updated•10 years ago
|
Attachment #8492700 -
Flags: review?(anthony) → review+
Comment 24•10 years ago
|
||
Comment on attachment 8498742 [details] [review] Link to PR (rebased) Carrying r+ since this is just the same PR.
Attachment #8498742 -
Flags: review?(anthony) → review+
Attachment #8498742 -
Flags: review+ → review?(francisco)
Comment 25•10 years ago
|
||
Comment on attachment 8498742 [details] [review] Link to PR (rebased) Unfortunately we have a unit test failing: https://tbpl.mozilla.org/?rev=93d7535a60fc1ac730495e5b646500d363b2c33e&tree=Gaia-Try Could you take a look?
Attachment #8498742 -
Flags: review?(francisco)
Updated•10 years ago
|
Flags: needinfo?(sv99)
Reporter | ||
Comment 26•10 years ago
|
||
Strange behavior this test - on local system test ok - on server error. ?test('without params', -> assertSaveState('disabled'); test('with date params' -> ? assertSaveState('disabled'); date param set into hidden fiels #date_0.valueAsDate buttonSave disabled if emptyForm() == true emptyForm test value param for text fields - #date_0.value == "" #date_0.valueAsDate == Date(0), but this value not tested!!
Reporter | ||
Comment 27•10 years ago
|
||
b2g test('with date params' -> ? assertSaveState(null); buttonSave enabled FirefoxNightly test('with date params' -> ? assertSaveState('disabled'); buttonSave disabled Which is right behavior?
Flags: needinfo?(sv99)
Comment 28•10 years ago
|
||
I think I've seen the same issue recently, maybe while working on bug 874510 but I'm not sure. This is most certainly a race issue: the TRY server is likely slower than your computer and some things are not running in the same order. These issues are the most difficult to fix sadly :/ Francisco maybe you can help?
Updated•10 years ago
|
Flags: needinfo?(francisco)
Reporter | ||
Comment 30•10 years ago
|
||
Francisco, buttonSave must by disabled when send date param or not?
Attachment #8498742 -
Flags: review?(francisco)
Comment 32•10 years ago
|
||
Comment on attachment 8498742 [details] [review] Link to PR (rebased) \o/ Excellent work, thanks for the patch!
Flags: needinfo?(francisco)
Attachment #8498742 -
Flags: review?(francisco) → review+
Keywords: checkin-needed
Comment 33•10 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/8fa5ffe5054303a9086dcbc404de182a0b9652ab
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
You need to log in
before you can comment on or make changes to this bug.
Description
•