async call returns an error #97

Closed
razvandimescu opened this Issue Jun 26, 2011 · 6 comments

Projects

None yet

6 participants

@razvandimescu
  var assert, vows;
  vows = require('vows');
  assert = require('assert');


  function DataStore(){}
  DataStore.prototype.all = function(callback){ return callback([1,2,3])}


  vows.describe('database communication').addBatch({
    'retrieving all items': {
        topic: new(DataStore),
        'calling all':{
            topic: function(store) {store.all(this.callback)},
            'should return items': function(result){
                assert.ok (result.length>0)
            }
        }
    }
  }).run();

Returns:

  retrieving all items calling all
    ✗ should return items
      » An unexpected error was caught: [1,2,3]
@diversario

Holy crap, I just spent two hours combing through my code because of this! I get the same error when using this.callback:

.addBatch({
  'test': {
    'add record': {
      topic: function(){
        var options= {
                     // parameters
                    };
        addRec(options, this.callback);
        },
        'should be OK': function (result){
          assert.deepEqual(result.ok, true);
        }
    }
  }
})

results in

  test
    ✗ should be OK
      » An unexpected error was caught: {"ok":true}
@cloudhead

Vows assumes the first argument is an error, by convention. See #24 — I'm open to suggestions on how this can be improved/be more obvious.

@diversario

Oh, I didn't realize that. I'm not going to suggest anything here, since I see you have a way to alter this default behaviour. But, as suggested in that thread - it should be pointed out in the docs (especially on vowsjs.org).

@uberbrady

I just lost about an hour on this one. Again - documented would help, but my vote is that this feature isn't necessary at all.

(TL;DR - look at the end for a proposal that's probably an OK compromise, if it will even work).

Something that's quite telling is that when I was typing into Google search:

vows "an unexpected error was caught"

It autocompleted for me and several posts came up.

My feeling is, parameters should hold no special meaning.

If you want to ensure something doesn't error, you should assert that err is null.

How many real bugs have been found using this feature, versus how many people have been tripped up? Nothing in nodejs.org implies that the func(err,result) is the way that all things should/should not work.

My test should look like this:

topic: function () {statedb.get_data("testblobs",17,this.callback)},
'we get the data': function (blob) {
    assert.deepEqual(blob,{id: 17, name: "groodle2",something: "some thang else",number: 1});
}

But instead I'm going to have to change that topic to:

topic: function {var self=this;statedb.get_data("testblobs",17,function (results) {self.callback(null,results)})}

(or use the {error: false} thing - but what if some of my tests I do want the autodetect error? Not sure.)

And all of that to help something like this:

topic: function () {mysql.query(testquery,this.callback)},
'we get the data': function (err,result) {
    assert.isNull(err); //<- Explicitly stating this is probably *good*. No?
    assert.deepEqual(result,{id: 17, name: "groodle2",something: "some thang else",number: 1});
}

Idea 1

Maybe a compromise would be to have several available callbacks in the 'this' object. (Please forgive the horrible naming, I'm sure we can come up with something better if you like the idea):

this.callback - plain callback, no special treatment of parameters (as per documentation)

this.noerrorcall - node.js module convention, func(err,param1,param2,param3,param4...), err must be null? Automatically strips error from the results (to make it more obvious)?

this.errors - node.js module convention, func (err,param, param, param...) - MUST have a non-null error, doesn't pass additional parameters.

The problem I have with my own proposal is that I like the invocation of 'callback' to be plain and simple and unadorned, and the assertions on the results side to be more ornate.

Idea 2 - if possible, it's the "good one"

How about this - and this is a little ugly, so you can feel free to hate it, I'm just throwing out ideas here. What if you, within the actual vow - look at the source (.toSource()) and parse out the function definition. If the first param is called "err" or "error", then it MUST be null?

@adamstallard

Interesting ideas. I had similar ideas (before I read this). Basically, just set some options on this.callback to indicate what kind of callback it is. Here are my complete thoughts: #264 . I also document what I feel are the use-cases (prioritized) and how you have to currently implement them, vs. how you could implement them.

I think the reason this is poorly documented, is because barely anyone understands it completely to be able to document it, and it might be embarrassing to try to do so. It also explains why it is not tested, and is actually currently broken. (though pull request #263 would fix it)

@indexzero
vowsjs member

Closing this since it is apparently fixed by #263. Going to add the same "error-handling" label. Thanks @adamstallard!

@indexzero indexzero closed this Nov 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment