is_async test considered harmful

[krivin]

Love yadda! However, just updated from 0.17.0 to 0.17.10 and got bit by this change 1745737#diff-d2708c4b2779758848a0614156e0d22fR51. It seems helpful, but doesn't always work and when it's wrong, it just next() before it should leading to some very, very confusing test results.

Cases I can think of where it doesn't work:

• variadic async functions (functions with no parameters that uses the arguments variable, like the one the above line of code is in), then macro.length is zero and args.length might as well.
• a macro function is used that (by mistake or not) has fewer argument than the step provides, e.g. .when("$NUM $COLOR bottle accidentally falls", function(number_of_falling_bottles) { })

I think that if you're trying to be helpful but sometimes mess things up badly, you shouldn't try to help.

[cressie176]

Sorry you were 'bit' by this. In general I agree with overly helpful == dangerous sentiment. I'll take a look at the code and go through your examples. I don't want to remove the is_async test, but will see if I can make it more robust.

[cressie176]

I've pushed a branch I was working on called macro-refactor, which at least makes the code in this area a bit easier to read. It won't help with the variadic async functions but handles the "too few arguments" problem for synchronous steps better.

Sync

.when("$NUM $COLOR bottle accidentally falls", function (num) {})
Passes

.when("$NUM $COLOR bottle accidentally falls", function () {})
Passes

.when("$NUM $COLOR bottle accidentally falls", function (num, color, extra) {})
Hangs (because it's assumed to be async, but the 'extra' is never called).

Async

.when("$NUM $COLOR bottle accidentally falls", function (num, color, next) { next() })
Passes

.when("$NUM $COLOR bottle accidentally falls", function (num, next) { next() })
Errors because next is not a function

.when("$NUM $COLOR bottle accidentally falls", function (num, color, extra, next) { next() })
Errors because next is not a function

I think all the of the above demonstrate reasonable behaviour but variadic async are still a problem. It may be possible to provide an explicit flag when defining the step

.when("$NUM $COLOR bottle accidentally falls", function () { last(arguments)() }, Step.ASYNC)

or

.when("$NUM $COLOR bottle accidentally falls", function () { last(arguments)() }).async()

Clumsy, but at least it would provide a workaround.

[krivin]

Cool.

Still think the hanging issues is problematic and subtle.

The reason I got bit was actually because I was using 0.17.0 to test some Promise-heavy code and so had introduced two variadic higher-order methods, sync() and async(), which would decorate synchronous and asynchronous macros respectfully, i.e. .when("$NUM $COLOR bottle accidentally falls", async(function (num) { ... return Promise... })). But I think that if #217 got fixed, I can ditch those decorators and the variadic methods wouldn't be an issue for me, and I could be a happy 0.17.10+ camper. Until, at least, a test suddenly starts hanging :-)

You decide what to do :-)

[cressie176]

#217 should be fixed in macro-refactor. Also includes async variadic methods with

.when("$NUM $COLOR bottle accidentally falls", function () { 
  arguments[arguments.length - 1]()
}, {}, { mode: 'async'} )

Want to test it out with some examples, then I'll publish to npm.

[cressie176]

I've refactored the Macro code and provide the option to specify whether it's sync, async or promise based. Released in 0.18.0