Fix for Issue-133 #138

Closed
wants to merge 5 commits into
from

Projects

None yet

2 participants

@TEHEK

Almost done :)

Please review changes and tests

@TEHEK

Not sure what to do about this one. If function throws in callback, it's not possible to catch it here anyway.

Same applies to tests on lines 1425, 1522, 1637

@cjohansen cjohansen commented on an outdated diff Jun 9, 2012
test/sinon/stub_test.js
+ assert.exception(function () {
+ stub.callsArgAsync({});
+ }, "TypeError");
+ }
+ },
+
+ "callsArgWithAsync": {
+ setUp: function () {
+ this.stub = sinon.stub.create();
+ },
+
+ "asynchronously calls argument at specified index with provided args": function (done) {
+ var object = {};
+ this.stub.callsArgWithAsync(1, object);
+
+ var callback = sinon.spy(function (arg) {
@cjohansen
cjohansen Jun 9, 2012

Can be written more succinctly as

var callback = sinon.spy(done(function (arg) {
    assert(callback.calledWith(object);
});
@cjohansen

This looks pretty good! I'm happy with the implementation. I am however, slightly worried about adding that many new tests. I think I'd prefer if you just tested that the methods asynchronously pass control to the synchronous variants. Repeating all these tests that many times is going to be a pain if any of this ever changes (i.e., lots of test updating).

@TEHEK

Changed implementation a bit to call stub's instance methods rather than prototype's methods. Removed couple of redundant tests.

Would you like me to remove other tests as well?

Currently async versions are not calling sync-versions asynchronously, but rather set flag for callCallback method to run given callback asynchronously. So technically just testing that async versions pass arguments to sync versions does not really test asynchronous functionality. But if you think that should suffice, I can just remove the rest. They were primarily used for development :D

@TEHEK

I can also replace

            var callback = sinon.spy(function () {
                assert(callback.calledWith());
                done();
            });

with

            var callback = sinon.spy(done(function () {
                assert(callback.calledWith());
            }));

if you decide to keep those tests :D

@cjohansen

This looks good, you've removed quite a bit of duplicated testing. Btw I didn't suggest to not test the asynchronicity, but rather restrict the tests to 1) make sure nothing happens immediately, 2) after timeout it calls the sync version.

Anyway, I'm happy with the current solution. Could I bother you to make that last done switch? I like consistency :)

@TEHEK

Will do tonight :)

@TEHEK

Btw I didn't suggest to not test the asynchronicity, but rather restrict the tests to 1) make sure nothing happens immediately, 2) after timeout it calls the sync version.
I can leave just one async test per method and test just the most general case then

@cjohansen

Cool. You agree that that covers it well enough?

@TEHEK

Phew... sorry for going missing for so long :D

Yep, I agree, that would cover the current implementation, kinda balances between being test-driven and flexible for possible changes.

I removed all but one async test per async method. Waiting for final approval.

@cjohansen

Thanks! The commit says "WIP", is it ready for pull?

@TEHEK

It says "WIP" because its waiting for your approval :)
I can close this pull request and rebase everything in new one, or you can squash rebase before merging things in :)

@cjohansen

Ah, right. Well, then, approved. Sorry for being difficult, I really appreciate your contribution! Care to do the rebase?

@cjohansen cjohansen closed this Jun 21, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment