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

Chill should use the xUnit IAsyncLifetime #80

Open
tomachristian opened this issue Jun 18, 2019 · 6 comments
Open

Chill should use the xUnit IAsyncLifetime #80

tomachristian opened this issue Jun 18, 2019 · 6 comments

Comments

@tomachristian
Copy link

Any async work declared inside a Given/When inside the ctor of test classes will be waited for by blocking the thread on which the constructor is executing. This is against all the asynchronous programming best practices. Chill should use the IAsyncLifetime to do real async work during initialization, see https://mderriey.com/2017/09/04/async-lifetime-with-xunit/. GivenWhenThen can easly implement IAsyncLifetime and it can "gather" work (from Given/When) to be executed during IAsyncLifetime.InitializeAsync

@dennisdoomen
Copy link
Collaborator

It could be improved indeed, but what problem are you facing with the current implementation?

@tomachristian
Copy link
Author

tomachristian commented Jun 18, 2019

The #77 was a hacky way of solving that particular issue, but if this (proposal in the current issue) were implemented in the first place we would not have these problems. It is just a suggestion for the proper way of solving it. At the time of #77 I didn't know about IAsyncLifetime.

PS: Also, asynchronous work would properly distribute on the thread pool and not block the threads executing the constructors of the test classes.

@dennisdoomen
Copy link
Collaborator

True, but it will also tie Chill to xUnit.

@tomachristian
Copy link
Author

True, but maybe an additional package can be provided for this functionality. With Async variants of GivenWhenThen particular to xUnit (eg AsyncGivenWhenThen). The main package can then discourage the use of Given(async) and When(async) and can suggest the use of the Async-test-fx-specific packages.

Just an idea/suggestion for the future, nothing mandatory.

@dennisdoomen
Copy link
Collaborator

I'm in doubt whether the extra complication will confuse people and is warranted. What problem do you think people are facing?

@tomachristian
Copy link
Author

tomachristian commented Jun 18, 2019

I'm in doubt whether the extra complication will confuse people and is warranted

Yes, I don't know either. For us, it isn't needed, but just felt it would be good to share this idea.

A concrete example (besides what was fixed in #77) is that we are doing very-long-running (5 minutes) operations in our setup for the specs (inside the constructors). We have a lot of these specs (lets say 10). xUnit limits the number of constructors of test classes that can run at a given time (lets say 5). Because these specs of ours take so damn long, we basically block any other test from starting up (Chill waits for the async work to finish by blockign the ctor threads). If this work would be delegated to IAsyncLifetime.InitializeAsync, other specs can start running because the constructor thread pool wouldn't be blocked.

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

2 participants