[communications] unit test refactoring for new version chai.assert api

RESOLVED FIXED in 2.1 S7 (24Oct)

Status

Firefox OS
Gaia::Contacts
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sv99, Unassigned)

Tracking

unspecified
2.1 S7 (24Oct)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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!
(Reporter)

Comment 1

4 years ago
Created attachment 8492700 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24244
Attachment #8492700 - Flags: review?(felash)
Blocks: 1070047
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 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)
(Reporter)

Comment 4

4 years ago
without try catch if you have failed test then we have uncaught exception - no help stack trace.
This is from chaijs site async template
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 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)
(Reporter)

Comment 7

4 years ago
some test change, Julien suggestion not totally equivalent, some test after change not worked!!
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
(Reporter)

Comment 9

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

4 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
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 :)
(Reporter)

Comment 12

4 years ago
second code changed
Thanks
(Reporter)

Updated

4 years ago
Attachment #8492700 - Flags: review?(felash)
(Reporter)

Comment 13

4 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 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)
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 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)
We can definitely remove all try/catch using the done() thingie. Please do it.
(Reporter)

Comment 18

4 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)
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

4 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);
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 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

4 years ago
Attachment #8492700 - Flags: review?(anthony) → review+
(Reporter)

Comment 23

4 years ago
Created attachment 8498742 [details] [review]
Link to PR (rebased)
Attachment #8498742 - Flags: review?(anthony)
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+
(Reporter)

Updated

4 years ago
Attachment #8498742 - Flags: review+ → review?(francisco)
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)
Flags: needinfo?(sv99)
(Reporter)

Comment 26

4 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

4 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)
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?
Flags: needinfo?(francisco)
Yes, I can, keeping the ni to track this.
(Reporter)

Comment 30

4 years ago
Francisco, buttonSave must by disabled when send date param or not?
(Reporter)

Updated

4 years ago
Attachment #8498742 - Flags: review?(francisco)
(Reporter)

Comment 31

4 years ago
change - test complete!!
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+
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/8fa5ffe5054303a9086dcbc404de182a0b9652ab
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 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.