bitovi / syn

Standalone Synthetic Event Library
https://www.npmjs.com/package/syn
MIT License
444 stars 260 forks source link

Suggestion: use a futures API rather than callbacks #78

Open fresheneesz opened 9 years ago

fresheneesz commented 9 years ago

So instead of writing:

    syn.type(a, "x", function() {
        syn.click(b, function() {
            syn.click(c, function() {
               syn.click(d)
           })
        })
    })

you could write something like:

syn.type(a, "x").then(function() {
     return syn.click(b)
}).then(function() {
     return syn.click(c)
}).then(function() {
     return syn.click(d)
}).done()

so you don't get all stuck in callback hell. I'd suggest a futures library but i don't wanna plug my stuff too much all at once ; )

justinbmeyer commented 9 years ago

Although futures prevent callback hell, a few things to consider:

fresheneesz commented 9 years ago

Another consideration is exception handling. If errors happen within the callbacks, you get a whole nother kind of debugging hell

justinbmeyer commented 9 years ago

How are promises more difficult to debug than callbacks?

If code breaks within a promise, the error is caught because everything happens within a try/catch. Then, the promise is rejected asynchronously. It can be very difficult to trace the source of the problem.

Another consideration is exception handling. If errors happen within the callbacks, you get a whole nother kind of debugging hell

Callbacks make debugging much easier in my opinion. Promises make handling exceptions much easier. But that is not something commonly needed from Syn. It doesn't "throw" exceptions. I wouldn't want syn's code to always be running within a try/catch.

I would certainly rather see that as well, but my opinion is that FuncUnit should be considered as separate, and the API for syn should be improved without assuming it will be wrapped with a different API. It seems to me that most functionality in Syn isn't fully synchronous, and in a lot of cases, users will have to know that an action is complete before running some other code (like assertions for example).

My point was if Syn's API was going to be developed, should reflect FuncUnit's. No promises or callbacks necessary.

Depending on a library isn't so bad is it?

The reason we extracted Syn from FuncUnit was to keep it simple as possible. I'd like to avoid adding other dependencies, even something as banal as a promise. Especially as I'd like to make this sharable via UMD / AMD / CJS, which can be very tricky when you have dependencies (and don't want to bundle them).

justinbmeyer commented 9 years ago

The reason we extracted Syn from FuncUnit was to keep it simple as possible.

The idea is that Syn ONLY does event simulation. If a nicer API is needed, someone can use FuncUnit.

fresheneesz commented 9 years ago

everything happens within a try/catch

I think this is also the case with callbacks. How do you do error handling without catching errors?

It doesn't "throw" exceptions.

But code written in the callback might.

Sounds like you already have a firm opinion about this, and its your project so its your call.

justinbmeyer commented 9 years ago

I think this is also the case with callbacks. How do you do error handling without catching errors?

In tests, I rarely "catch" exceptions. I want them to be thrown uncaught.

I guess I could see wanting to handle exceptions with promises in other use cases, where if an error happens you want to do something other than have the tests break.

Sounds like you already have a firm opinion about this, and its your project so its your call.

Well, would you be alright with making an promise adapter we could bundle? Similar to how Q can wrap node's fs methods. I'm fine including another version of Syn that supports this API in the download. I just think the core should remain using callbacks.

Thoughts?

fresheneesz commented 9 years ago

In tests, I rarely "catch" exceptions.

Usually true for me too - unless you're explicitly testing for errors. When I use promises in testing, I expect that too. The exceptions I get will show me the exact line of code where the exception came from, and so I'm just as happy with the exceptions.

I'm also of the mindset that you don't know everything people are going to want to use your system for. I would probably only use it for testing, but some people might find other uses for it, where good error would come in more handy.

I could certainly make a wrapper. I definitely agree with a lean core.

fresheneesz commented 9 years ago

Actually, your other comment is relevant to this. You mentioned you could do this:

it("foo", function(done){
  F("#abc").click(function(){
     assert.equal($("#abc").val(), "x", "X gon' give it to ya" );
  });
  F("#abc").type(function(){
     assert.equal($("#abc").val(), "x", "X gon' give it to ya" );
  });
})

However, I think this could cause very unintuitive behavior. Lets say you wanted to this:

it("foo", function(done){
  F("#abc").click(function(){
     var someStateINeed = someFunction($("#abc")) // needed in both asserts
     assert.equal($("#abc").val(), someStateINeed.Y, "X gon' give it to ya" );
     F("#abc").type(function(){
         assert.equal($("#abc").val(), someStateINeed.Y, "Y gon' give it to ya" );
     });
  });
  F("#abc").type("something", function(){
     assert.equal($("#abc").val(), "Z", "Z gon' give it to ya" );
  });
})

Without thinking about it, it looks like the asserts would go in the order X, Y, Z, but in reality, wouldn't they go in the order X, Z, Y?

justinbmeyer commented 9 years ago

No, it would go X, Y, Z. FuncUnit handles nesting.

fresheneesz commented 9 years ago

Ah ok, that's good

fresheneesz commented 9 years ago

I thought of a case where the unintuitive behavior does come out:

it("foo", function(done){
  var abc= F("#abc")
  abc.click(function(){
     var someStateINeed = someFunction(abc) // needed in both asserts
     assert.equal(abc.val(), someStateINeed.Y, "X gon' give it to ya" );
     doSomethingAsynchronous(function() {
         abc.type('something else', function(){
           assert.equal(abc.val(), someStateINeed.Y, "Y gon' give it to ya" );
         });
      });
  });
  abc.type("something", function(){
     assert.equal(abc.val(), "Z", "Z gon' give it to ya" );
  });
})

In this case, the click handler fully finishes before scheduling any new actions (because of doSomethingAsynchronous), and so the thing of "something" will happen before "something else". Its an edge case, but in complicated schenarios, it will trip people up.

justinbmeyer commented 9 years ago

I'm not sure why someone would doSomethingAsynchronous in that situation. But we can make it easy to prevent the next action from happening before doSomethingAsynchronous completes.

fresheneesz commented 9 years ago

Well I actually didn't just come up with it in my head, I came across a similar issue while doing exactly that in my test. Problem was a completely different test was running concurrently, so it was typing one character in one input, and then a character in a completely different input, and switching back and forth. Javascript is a very asynchronous language, you gotta expect that code can be run asynchronously at any time.

Anyways, its easy for me to fix since I'm using futures, without futures you'd need some fancy footwork.

justinbmeyer commented 9 years ago

Imo, I don't have to expect tests to be running async. No popular testing framework does this.

Sent from my iPhone

On Feb 19, 2015, at 7:13 PM, fresheneesz notifications@github.com wrote:

Well I actually didn't just come up with it in my head, I came across a similar issue while doing exactly that in my test. Problem was a completely different test was running concurrently, so it was typing one character in one input, and then a character in a completely different input, and switching back and forth. Javascript is a very asynchronous language, you gotta expect that code can be run asynchronously at any time.

Anyways, its easy for me to fix since I'm using futures, without futures you'd need some fancy footwork.

— Reply to this email directly or view it on GitHub.

fresheneesz commented 9 years ago

My case is just an example. There's an infinity of async possibilities in javascript. The fact is, its not only possible, but it happens.

In any case, I'm using a real simple wrapper. If you're interested in using the wrapper here it is:

// requires: syn (loaded as a global variable from an html script tag)

var Future = require('async-future')

exports.click = wrap(syn.click)
exports.type = wrap(syn.type)
exports.key = wrap(syn.key)

function wrap(fn) {
    return function() {
        var resultFuture = Future.wrapSingleParameter(fn).apply(this,arguments)

        // for chaining
        var target = arguments[0]
        resultFuture.click = function() {
            exports.click.apply(this, [target].concat(arguments))
        }
        resultFuture.type = function() {
            exports.type.apply(this, [target].concat(arguments))
        }
        resultFuture.key = function() {
            exports.key.apply(this, [target].concat(arguments))
        }

        return resultFuture
    }
}

That uses https://github.com/fresheneesz/asyncFuture

justinbmeyer commented 9 years ago

Why do your tests run at the same time?

Sent from my iPhone

On Feb 19, 2015, at 10:06 PM, fresheneesz notifications@github.com wrote:

My case is just an example. There's an infinity of async possibilities in javascript. The fact is, its not only possible, but it happens.

In any case, I'm using a real simple wrapper. If you're interested in using the wrapper here it is:

// requires: syn (loaded as a global variable from an html script tag)

var Future = require('async-future')

exports.click = wrap(syn.click) exports.type = wrap(syn.type) exports.key = wrap(syn.key)

function wrap(fn) { return function() { var resultFuture = Future.wrapSingleParameter(fn).apply(this,arguments)

    // for chaining
    var target = arguments[0]
    resultFuture.click = function() {
        exports.click.apply(this, [target].concat(arguments))
    }
    resultFuture.type = function() {
        exports.type.apply(this, [target].concat(arguments))
    }
    resultFuture.key = function() {
        exports.key.apply(this, [target].concat(arguments))
    }

    return resultFuture
}

} That uses https://github.com/fresheneesz/asyncFuture

— Reply to this email directly or view it on GitHub.

fresheneesz commented 9 years ago

The only reason to choose do it would be if you have tests that have a lot of asynchronous downtime - so your test suite doesn't take forever to run. So there's a reason to do it, even if its not common. The real reason is that I haven't implemented automatic completion-waiting in deadunit yet. I do plan to make that default, and have a switch allowing concurrent behavior.

But again, that's not the point. Almost any asynchronous behavior you can think of will not be the tests running asynchronously, but rather some other logic related to the code you're testing that is what's asynchronous.