Hexagon / node-telldus

Node bindings for telldus-core
Other
34 stars 10 forks source link

Uncaught assertions does not work as usual. #18

Closed kmpm closed 10 years ago

kmpm commented 10 years ago

If you run the test below using mocha the fs 'should work like this' shows the uncaught assertion as the failed test. Telldus does not bubble up that in it's callback, instead mocha complains about a timeout because the code never reaches done().

It has the same effect though, the test fails but it does not show the correct reason. It would perhaps not matter that much here but it is really unexpected behaviour and not like for example how the fs module behaves.

var should = require('should');
var fs = require('fs');
var telldus = require('..');

describe('testing callbacks', function(){

    it('should work like this', function(done){
        fs.rename('non existing file', 'whatever', function(err){
            should.not.exist(err);
            done();
        });
    });

    it('does like this', function(done){
        telldus.turnOn(9999, function(returnValue){
            returnValue.should.equal(0);
            done();
        })
    });
})
kmpm commented 10 years ago

forgot to add an example of the output.-

  1) testing callbacks should work like this:
     Uncaught AssertionError: expected { [Error: ENOENT, rename 'non existing file'] errno: 34, code: 'ENOENT', path: 'non existing file' } to not exist
      at /home/code/node-telldus/test/test.js:15:15
      at Object.oncomplete (fs.js:107:15)

  2) testing callbacks does like this:
     Error: timeout of 2000ms exceeded
      at null.<anonymous> (/home/code/node-telldus/node_modules/mocha/lib/runnable.js:165:14)
      at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)
Hexagon commented 10 years ago

Can you try increasing the timeout? turnOn actually return -3, but for some reason it takes some time for it to "fail"

nilzen commented 10 years ago

It takes ~28 sec to get "-6" if no Tellstick is connected to the computer...

Hexagon commented 10 years ago

Hmm, i guess that should be considered being a bug in telldus-core. I can't imagine it should take 28 seconds for the software to realise there is no tellstick connected :)

One thing to test though, is if it fails faster if we run tdInit before turnOn (currently tdInit is only available through the getDevices function, will raise a separate issue on that).

kmpm commented 10 years ago

Ok. you mention valid issues but that was not my case. Error handling in telldus callbacks does not work as in native libraries. In the new revised the test below a changed the actual testing to always fail regardless of the actual result of the call. The important part is, if a error is thrown within the callback it doesn't bubble out to the caller as in the native libraries.

When running the test and the error is thrown, both is expected to display the same result, Uncaught Error: qwerty In the telldus case something makes it do a console.error (my guess) and nothing else happens. The error doesn't bubble up in the hierarchy so that mocha can fetch and display it. The only thing mocha sees is a timeout.

example output

$ mocha test/test.js 

  ․/home/peterm/code/node-telldus/test/test.js:467: Uncaught Error: qwerty

  0 passing (2s)
  2 failing

  1) testing callbacks should work like this:
     Uncaught Error: qwerty
      at /home/peterm/code/node-telldus/test/test.js:9:10
      at Object.cb [as oncomplete] (fs.js:168:19)

  2) testing callbacks does like this:
     Error: timeout of 2000ms exceeded
      at null.<anonymous> (/home/peterm/code/node-telldus/node_modules/mocha/lib/runnable.js:165:14)
      at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

test/test.js

var fs = require('fs');
var telldus = require('..');

describe('testing callbacks', function(){

    it('should work like this', function(done){
        fs.exists('non existing file', function(err){
            throw new Error("qwerty");
            done();
        });
    });

    it('does like this', function(done){
        telldus.turnOn(1, function(returnValue){
            throw new Error("qwerty");
            done();
        });
    });
})
kmpm commented 10 years ago

It does seem to work with for example addRawDeviceEventListener. And no, it's not my callback wrapper ( still not ready but I use it for testing ) in this case, it's equally bad before I introduced my changes.

Hexagon commented 10 years ago

Ah, now i see the problem i, I'm pretty (really) new to node.js so i don't always know how stuff is supposed to work :)

Hexagon commented 10 years ago

I think this is the way to go -> https://github.com/raztus/node-oracle/commit/077f1938de2ace6d1094611f2081fb3fe802c6f5 , will try it out soon

Hexagon commented 10 years ago

https://github.com/kkaefer/node-cpp-modules/blob/master/05_threadpool/modulename.cpp <- Great help, solves my problem with asyncification of getDevices too!

Hexagon commented 10 years ago

2) testing callbacks should work like this: Uncaught Error: qwerty at /home/pi/node-telldus/test/test.js:8:19 at Object.cb as oncomplete

3) testing callbacks does like this: Uncaught Error: qwerty at /home/pi/node-telldus/test/test.js:15:19

nilzen commented 10 years ago

Should this be closed? #24 refers to this as a blocker...

kmpm commented 10 years ago

This should be closed, and it's not blocking the other. All this does is to enable a more accurate error message being shown. It doesn't affect #24 in any functional way that I have noticed.