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

Async detection through promises #14

Closed
josh-egan-ps opened this issue Jun 7, 2016 · 9 comments
Closed

Async detection through promises #14

josh-egan-ps opened this issue Jun 7, 2016 · 9 comments

Comments

@josh-egan-ps
Copy link

I just stumbled across this repo and haven't played with it yet, but it looks pretty cool. One idea I had upon reading the README is that instead of requiring a callback parameter for async tests, performing promise detection would be a convenient way to also detect an async test. Perhaps using a package like is-promise

@searls
Copy link
Member

searls commented Jun 8, 2016

A couple folks have asked about this. I wonder if it isn't a good opportunity for testing the extensibility of the teenytest API to allow plugins that augment runner behavior.

I would somewhat slightly rather this be a plugin than built-in for two reasons:

  1. I don't use promise-style tests myself, mostly because of:
  2. The high number of fantasy tests I've seen where a test is left green because a promise stopped being returned by a subject (a test that passes unconditionally because the subject is broken is a bad look). I've also seen lots of user confusion when a return statement was forgotten or when multiple chai-as-promised assertions are used in a single test and only the last triggers async mode.

@searls
Copy link
Member

searls commented Jun 8, 2016

Cc/ @schoon

@josh-egan-ps
Copy link
Author

Ya, those are some interesting thoughts. I'd be interested to see an example of a test that incorrectly passes because a promise is no longer returned. That's not something I've run into before.

I've definitely run into the confusion that comes from forgetting a return statement on a promise, but I've probably run into that confusion an equal number of times because I've either forgotten to add the done callback as a parameter in my function or because I've forgotten to invoke the done callback. So at least for me personally, there's a tripping point either way and over time I have learned not to trip up there, and if I do, I recognize the error message and can fix it quickly.

With the final point about multiple chai-as-promised assertions being used in a single test, I ran into that too when I was first getting into promises. However, I don't think that's an issue with callbacks vs. promises as much as understanding what promises are and then properly structuring the tests. If multiple promises are going to be used in a single block, then by the nature of promises, they would need to be chained in order to work reliably. That chain can either be returned or you would have to add a final then to the chain to call the done callback when everything is finished. If it's ok for them to all run in parallel, then Promise.all could be used, but then you'd still either need to return that promise or add a chain onto it to call done.

In my mind, the above gotchas are present either way. And in the course of practice, I find it more convenient to have the last line in a block return the promise rather than introduce and use a done callback. It kind of boils down to:

let result
beforeEach(() => {
  //arrange

  return subject.doAsyncThing().then(r => result = r)
}

vs.

let result
beforeEach(done => {
  //arrange

  subject.doAsyncThing().then(r => result = r).then(done)
}

@josh-egan-ps
Copy link
Author

And just to add some context, the reason I prefer the return is that it is easier and faster to type return and convert the block to async by doing so than to navigate to the first line of the block and add done as a parameter and then go to the last line of the block and invoke done.

Also, I have code templates for all of these things, so it is more convenient to have a single template that will give me the skeleton for a block, and then if I need it to be async I just add the return.

@searls
Copy link
Member

searls commented Jun 9, 2016

I still feel like I want this to be solved via a plugin that promisifies the callback API via a test transformer (this is an extension point hook I plan on implementing soon). By saying I want this to be a plugin doesn't mean I am dismissing your perspective as not valid, it means I want to divest this repo of as much as I can (the same would go for implementing a describe/it or given DSL or implementing an alternative reporter.

aside

By the way I was really hopeful that is-promise was going to be a really cleverly designed module that did more than simply check for the presence of a function named then on the object. Alas. In the past, one annoyance I've dealt with is writing tests in CoffeeScript (for which all functions are expressions evaluating to the last statement), in which the final line may just incidentally be an object with then on it, causing mocha tests to mysteriously start blocking and timing out.

@josh-egan-ps
Copy link
Author

I think that sounds reasonable.

@schoon
Copy link

schoon commented Jun 9, 2016

Am I the schoon you are looking for? Not familiar with this repo. Thanks.
On Jun 9, 2016 1:33 PM, "Josh Egan" [email protected] wrote:

I think that sounds reasonable.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#14 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AEL8WgFjnKIDoTP65ASrSsX1Y4KfDdCyks5qKE5rgaJpZM4Iwcmv
.

@searls
Copy link
Member

searls commented Jun 9, 2016

Whoops, sorry @schoon. I meant @Schoonology

@searls searls mentioned this issue Jun 19, 2016
10 tasks
@searls
Copy link
Member

searls commented Jun 24, 2016

@josh-egan-ps ok this is shipped as of [email protected] and [email protected]

@searls searls closed this as completed Jun 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants