Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional error parameter in asynchronous testing not always optional #129

Closed
ghost opened this issue Aug 31, 2011 · 5 comments
Closed

Optional error parameter in asynchronous testing not always optional #129

ghost opened this issue Aug 31, 2011 · 5 comments

Comments

@ghost
Copy link

ghost commented Aug 31, 2011

Hi,

I'm new to vows and Node, so forgive me if I have got this all wrong.

When I was writing some tests yesterday I got confused as to why one set of async tests were behaving differently to another set. It turns out it was related to the number of parameters that I was passing to 'this.callback' within my topic function.

I created a gist to demonstrate what confused me:

https://gist.github.com/1182624

When you pass an error and a parameter to this.callback:

callback(null, [4,5,6]);

:then the error parameter in your tests is optional. Vows seems to be smart enough to either pass the error or just pass the parameter depending on whether your test function takes one or two parameters.

However when you pass an error and two parameters to this.callback:

callback(null, [4,5,6], 42);

;then the error parameter is no longer optional in your test function. You must have an error parameter, even if you don't need it, otherwise your other parameters will have the wrong values.

I think it would have been less confusing if the error parameter was mandatory for all async tests. However, maybe it is too late to change this behaviour.

Anyway, hopefully this explanation will help someone else if they encounter the same problem.

@beatgammit
Copy link

I had a similar problem to this, and I was surprised that vows actually handled the error parameter at all (I didn't really read the docs all the way through though).

Personally, I think it would be simpler to not check for the error handling, but that would require extra steps and possibly cluttered code in the test code.

+1 for always requiring the first parameter to be the error parameter (or null if not). It promotes good style (similar to what node's API uses).

@gabrielf
Copy link

I don't know whether I prefer vows treating the first parameter as an error or not. Doing a test involving websockets or any other library that doesn't conform to the node convention is definitely made more bloated because of it. But it should be mentioned in the docs and it should be consistent regardless of the number of parameters.

This is what I did in the webhook test, maybe it can be done even simpler?

topic: function () {
  var socket = io.connect();
  var self = this;

  socket.on('message', function (data) {
    self.callback(null, data); // Vows treats the first parameter as an error
  });
},

@adamstallard
Copy link

The feature is that you can let vows handle errors for you. You specify that you want this by giving your vow zero or one parameters.

Letting vows handle errors only works if the callback has exactly two arguments, the first being the error, the second being the data. If your callback has any other format, you have to wrap it to use this feature.

If your callback only takes one argument, you should wrap it either way, otherwise it won't work as expected. The alternative is to specify a second, unused parameter to your vow and then check whether the first parameter is an error or data.

@adamstallard
Copy link

Proposal for alternative error-handling for vows: #264

@indexzero
Copy link

This is a duplicate of #264.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants