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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[sv99]

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 1

[sv99]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24244

Comment 2

[Julien Wajsberg [:julienw]]

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 !

Comment 3

[Anthony Ricaud (:rik)]

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.

Comment 4

[sv99]

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

[Julien Wajsberg [:julienw]]

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

[Francisco Jordano [:arcturus] [:francisco]]

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!

Comment 7

[sv99]

some test change, Julien suggestion not totally equivalent, some test after change not worked!!

Comment 8

[Julien Wajsberg [:julienw]]

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

Comment 9

[sv99]

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)

Comment 10

[sv99]

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

[Julien Wajsberg [:julienw]]

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

Comment 12

[sv99]

second code changed
Thanks

Comment 13

[sv99]

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

[Julien Wajsberg [:julienw]]

Comment on attachment 8492700 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24244

Francisco is the peer :)

Comment 15

[Francisco Jordano [:arcturus] [: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?

Comment 16

[Francisco Jordano [:arcturus] [:francisco]]

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.

Comment 17

[Julien Wajsberg [:julienw]]

We can definitely remove all try/catch using the done() thingie. Please do it.

Comment 18

[sv99]

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();
          });
        });
      });

Comment 19

[Julien Wajsberg [:julienw]]

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?

Comment 20

[sv99]

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

[Julien Wajsberg [:julienw]]

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

[Francisco Jordano [:arcturus] [:francisco]]

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.

Comment 23

[sv99]

Attachment: Link to PR (rebased)

Comment 24

[Anthony Ricaud (:rik)]

Comment on attachment 8498742 [details] [review]
Link to PR (rebased)

Carrying r+ since this is just the same PR.

Comment 25

[Francisco Jordano [:arcturus] [: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?

Comment 26

[sv99]

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!!

Comment 27

[sv99]

b2g test('with date params' -> ? assertSaveState(null); buttonSave enabled

FirefoxNightly  test('with date params' -> ? assertSaveState('disabled'); buttonSave disabled

Which is right behavior?

Comment 28

[Julien Wajsberg [:julienw]]

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?

Comment 29

[Francisco Jordano [:arcturus] [:francisco]]

Yes, I can, keeping the ni to track this.

Comment 30

[sv99]

Francisco, buttonSave must by disabled when send date param or not?

Comment 31

[sv99]

change - test complete!!

Comment 32

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8498742 [details] [review]
Link to PR (rebased)

\o/

Excellent work, thanks for the patch!

Comment 33

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/8fa5ffe5054303a9086dcbc404de182a0b9652ab