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

Use domains to track which test threw an error. #214

Closed
jamestalmage opened this issue Nov 14, 2015 · 10 comments
Closed

Use domains to track which test threw an error. #214

jamestalmage opened this issue Nov 14, 2015 · 10 comments
Labels

Comments

@jamestalmage
Copy link
Contributor

The goal would be to track which test threw a given uncaughtException.

Domains have been "soft deprecated". Unfortunately, there is no clear alternative at this time.

See this discussion in the node repo.

The goal is to replace the functionality before fully removing it or recommending specific alternate approaches; this is an attempt to telegraph that any new code written against this feature will likely be subject to some churn in the near future, and that folks who can avoid that churn, should. My hope is that fewer folks directly depend on domain, and instead indirectly depending on the functionality though another package -- hapi, express, et al -- and that that would move the onus of updating domains-based code to fewer folks. Also, by not redirecting folks who need this functionality to other APIs, we cut down on both the number of times users have to change their code to a different API and the number of folks we "lose" by diverting their attention to a different API. It keeps the focus on domains and the eventual successor API, that is.

TL;DR: No specific suggestions yet (by design); just clearly labeling the footgun and mentioning that it's there if you need [it] to separate some feet from some legs, but soon there will be a better way to practice podiatry. If you'd like to take a stab at the wording in a separate PR I'd totally welcome it!

The gist is that the API is not going to disappear until a viable alternative exists.

The major arguments for deprecating domains is that people have tried to use them the wrong way (to prevent an http server from shutting down, etc). The danger with doing so in a production app is that the engine is left in an unknowable state that can be impossible to reliably recover from. Test frameworks face no such danger. When an uncaughtException is encountered, we are already marking the test as failed, and the process (by definition) will be bailing shortly anyways.

AVA is at a particular advantage here over other test frameworks since concurrent testing basically enforces zero shared state between tests. Looking at the domain API, it seems relatively straight forward, and anything we come up with should be fairly simple to whatever replacement comes along.

Given that the deprecation discussion took place nearly a year ago, and that the domain implementation is currently being actively contributed to - I think it's a good play. Especially if we stay mindful of the impending deprecation and work to minimize it's impact.

@sindresorhus
Copy link
Member

I don't have anything against using domains. It's fine if you use it correctly and it will not go away for a long time. I do, however, wonder if it's worth doing this. Promises and async functions are the future and we already handle those nicely. This really only applies to async exceptions in callback-style APIs, right?

From what I can remember, async listener is the intended domains replacement, but it's much more low-level and basically meant to be something you could build a domains implementation on in user-land, and many other use-cases too. Some use-cases: https://www.npmjs.com/browse/depended/async-listener

@sindresorhus
Copy link
Member

@vdemedes ?

@madbence
Copy link

fyi continuation-local-storage is built on top of async-listener, which provides a simple get/set api to track data across async boundaries.

@jamestalmage
Copy link
Contributor Author

I noticed in the bluebird codebase that it publishes unhandledRejections to a specific domain (at least I thought that was what it was doing), so we should be able to trace promise rejections back to a specific test.

There are still multiple ways to find yourself in a callback that is not wrapped in a promise. (event-emitters, setTimeout).

I'm certainly not married to Domains (I've never played with them), if there are better solutions, lets do that.

@sindresorhus
Copy link
Member

Yeah, I just don't want us to waste too much time on legacy use-cases.

setTimeout

https://github.com/sindresorhus/delay

event-emitters

https://github.com/zenparsing/es-observable (I know, I know :p)

@jamestalmage
Copy link
Contributor Author

Is async-listener landed? Doesn't seem to be.

@vadimdemedes
Copy link
Contributor

As for me, I don't think it's worth doing it at the moment. As @sindresorhus stated, it only applies to "async" errors/exceptions. For now, we have more important stuff to focus on. Maybe in the future, who knows.

@sindresorhus
Copy link
Member

@jamestalmage Read some weeks ago on an issue that it was close to landing. That's all I know.

We can continue discussing this, but let's defer actually doing it until later. There are lot of other more important issues to attend to.

@sindresorhus
Copy link
Member

@novemberborn novemberborn removed this from the Future milestone Aug 13, 2017
@novemberborn
Copy link
Member

Best I can tell, the lack of this feature hasn't really been an issue. There's only a select few use cases where user code that is actually called by a test (rather than setup code) later causes an uncaught exception. Given our detection of tests that remain pending or do not run any assertions, and our t.throws() handling, we can cover most cases. A good approach to debug other cases is to run tests serially.

I'm closing this for now. It'll be interesting to see where Async Hooks ends up and whether it's useful for us, but that's for future Node.js releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants