Add failing spec for When with done() and Then with no done()

[ronen]

In my app, I tried something like

When (done) -> _.defer -> done()
Then   -> ...my expectation...

But somehow my expectation never gets run, and Jasmine writes an error to the console:

Spec '...my expectation...' has no expectations.

I thought I'd try to debug it in jasmine-given, so started by adding a spec to test When(done) followed by Then [no done]. I thought that the spec would fail because the then() spy wasn't called. The spec does fail, but it reports that the done() spy wasn't called!

I'm not sure what's going on, and unfortunately don't have the time right now to dive into debugging it.
But I figured I'd at least bring it to your attention.

[searls]

We've definitely seen some weirdness around this in the past. I'm genuinely curious to know what the root cause is.

I won't have time this week to look at it, would you be willing to take a stab at it?

Also, are you interested in just being made a contributor to the project? I think you're the biggest contrib after myself.

[ronen]

I won't have time this week to look at it, would you be willing to take a stab at it?

Unfortunately no time for me this week either. Maybe later...

Also, are you interested in just being made a contributor to the project? I think you're the biggest contrib after myself.

I don't really expect to contribute much more, mostly just keep using it :) Unless I do get a chance to look into this done() weirdness before you do. But in any case if I did have something to contribute I'd want to run it by you first, so little difference whether ultimately I'm the one to press "merge this PR" or you are. So whatever you think best. Thanks.

[fionawhim]

I've just run into this. I think the problem is that declareJasmineSpec calls doneWrapperFor unnecessarily. The Waterfall should always be given done for its finalCallback, regardless of expectationFunction’s arity.

[searls]

Thanks @phopkins, would you have a few minutes to attempt a PR to refactor it?

[fionawhim]

Fix should be just to remove the call from that place. I'll spend some time
to try and get coverage of it, though.

On Thursday, January 7, 2016, Justin Searls [email protected]
wrote:

Thanks @phopkins https://github.com/phopkins, would you have a few
minutes to attempt a PR to refactor it?

—
Reply to this email directly or view it on GitHub
#45 (comment).

[searls]

@phopkins -- well, that's certainly enough to pass @ronen's test.

I have to admit I don't remember writing any of this implementation at all and find it super dense to look at now. If I know me, the reason I wrapped the top layer was to avoid the situation that every it would be itself async from the perspective of Jasmine.

I'm trying to think back on the original intent here — does the async support only work if you're on Jasmine 2.x, since that's the one which introduced the done() function argument?

[searls]

Landed in 2.6.4

Prithy that nothing blows up i guess.

[ronen]

Thanks @phopkins & @searls !

[fionawhim]

Sweet, thanks for the quick turnaround!

I don't know how Jasmine 2.0 will react with everything being async, whether that's going to be slower or worse or not.

This change should be a no-op for 1.3, since it wasn't affected by the arity at all.

[searls]

Hey @phopkins @ronen is it possible that merging this in broke Master under Node.js? https://travis-ci.org/searls/jasmine-given#L407

[fionawhim]

Looks like this change fixed the wrong behavior that the test was measuring. It seems to have been saying that if you had a synchronous “Then” followed by an async “Done,” that the “Done” would never be evaluated, which I think is counter-intuitive.

[searls]

@phopkins so what do you think we should do?

[fionawhim]

Probably update the test. The new behavior seems better, no?

On Tue, Feb 16, 2016 at 8:19 PM, Justin Searls [email protected]
wrote:

@phopkins https://github.com/phopkins so what do you think we should do?

—
Reply to this email directly or view it on GitHub
#45 (comment).