Handle errors correctly based on suite.options.error and the number of parameters expected by the vow #263

Merged
merged 24 commits into from May 13, 2014

Conversation

Projects
None yet
2 participants
Contributor

adamstallard commented Jan 23, 2013

Currently, if an error occurs in a topic and a subsequent vow expects 2 or more arguments, the first (error) argument will be set to null and the second will contain the error.

Currently a reference error in a topic will be thrown.

This the expected (fixed) behavior:

Don't throw errors; catch them and handle them as follows: When suite.options.error is set to false or a vow expects two or more arguments, return the error as the first argument and don't report it (let the user handle it); When suite.options.error is set to true (default) and a vow expects zero or one parameters, report the error in the test runner and don't run the vow.

Update:

I fixed another issue (#231) in this pull request: sub-topics will still proceed if the parent topic throws or returns an error.

Update:

Apparently, basic error-handling through vows is also broken(#280)--fixed by this pull request

adamstallard added some commits Jan 23, 2013

@adamstallard adamstallard fix typo in comment 26e5941
@adamstallard adamstallard Handle errors correctly based on suite.options.error and the number o…
…f parameters expected by the vow: When suite.options.error is set to false or a vow expects two or more parameters, get the error as the first argument and don't report it; When suite.options.error is set to true and a vow expects zero or one parameters, report the error and do not run the vow.
50a13b5
Contributor

JerrySievert commented Aug 19, 2013

can you do a merge so that there's a possibility of the merge occurring without conflicts? if so, i will take a look.

thanks much!

Contributor

adamstallard commented Aug 19, 2013

I merged from upstream and resolved the conflicts in lib/vows.js and lib/vows/reporters/spec.js (I chose the upstream over my changes). I ran the tests and they work--except isolate and suppress-stdout are still broken on Windows, node 0.10. (They do work on linux though)

Contributor

JerrySievert commented May 6, 2014

hm. i seem to have lost push after the move from @cloudhead to @flatiron - @indexzero can you reinstate?

Contributor

adamstallard commented May 6, 2014

@JerrySievert thanks for looking at this again. I haven't merged from upstream on my fork for 9 months, so let me know if you need me to do that.

Contributor

adamstallard commented May 6, 2014

@flatiron I'd be willing to be a maintainer of vows as well (of the repo and/or the npm package). I'm pretty familiar with the code and I still use it as my primary js test framework. Also, how is the website maintained? The documentation there is out-of-date.

Contributor

adamstallard commented May 6, 2014

Looks like the only thing that's changed since I merged last was adding the repository field to package.json #283 , so this should still be good to merge with no conflicts and I won't both to update my fork just for that.

@JerrySievert JerrySievert added a commit that referenced this pull request May 13, 2014

@JerrySievert JerrySievert Merge pull request #263 from adamstallard/master
Handle errors correctly based on suite.options.error and the number of parameters expected by the vow
aa8cd5e

@JerrySievert JerrySievert merged commit aa8cd5e into vowsjs:master May 13, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment