Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confusing warning when mixing Promise and callback style #580

Closed
hulbert opened this issue May 13, 2016 · 3 comments
Assignees
Milestone

Comments

@hulbert
Copy link

@hulbert hulbert commented May 13, 2016

Discovered this on accident while upgrading our large test suite to lab 10. Some authors had occasionally returned a promise to lab.test and were calling done—we normally use Bluebird's .asCallback(done) at the end of a promise chain.

Since lab 10 treats those returned promises as something to observe for done-ness, it warns about multiple callbacks in practice. The way it does this is not very descriptive and it would be awesome to find a way to make this more explanatory.

Here is an isolated case (test_slash_index.js goes in /test/index.js): https://gist.github.com/hulbert/7839b37035906b562c7f48f03af3de64#file-test_slash_index-js

And here is the test run:

> npm test

> @scoop/lab-test@1.0.0 test /Users/scott/src/piggyback/lab-test
> lab --verbose --debug

my test suite
  ✔ 1) should give me a better warning (1008 ms)


Test script errors:

Multiple callbacks or thrown errors received in test "my test suite should give me a better warning" (undefined)


There were 1 test script error(s).

1 tests complete
Test duration: 1014 ms
No global variable leaks detected

Ideally this would say something immediately and fail the test.

Alternatively, it would be great if it warned on any callbacks with args that also return a promise (I believe this is what mocha does)—i.e. disallow mixed styles up front.

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented May 13, 2016

I disagree that it should "say something immediately", this is just a confusing output if the other callback calls happen several tests after. But it should probably fail the test.

@hulbert

This comment has been minimized.

Copy link
Author

@hulbert hulbert commented May 18, 2016

I disagree that it should "say something immediately", this is just a confusing output if the other callback calls happen several tests after.

Just to make sure I understand, do you mean in the following case that if this failed on test start and then done() was called, that it would be confusing?

it('should work', function(done) {
    setTimeout(function() {
        done()
    }, 2000)
   return Promise.resolve()
})

So your solution would mean when the done() was called lab would say something like "Promise was returned in this test, calling done is not valid"? This makes sense to me. I was initially (wrongly) thinking you could inspect the function for its parameters and also check if a promise was returned but not invoke the callback. But I don't see how you could check if a Promise was returned from the callback w/o invoking it

What would a PR for this look like?

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented May 19, 2016

Now that I think of it, we could still test the arity of the function if it returns a promise, but we can't just move on to the next test cause this one would still be running. So we need to mark it failed whatever the result, but let it run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.