search

ChillBDD / Chill

Chill, a BDD style testing framework - "If you stick it in a container, Chill will keep it cool"
https://chillbdd.com
60 stars 20 forks source link

Chill should use the xUnit IAsyncLifetime #80

Open tomachristian opened 5 years ago

tomachristian commented 5 years ago

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 commented 5 years ago

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

tomachristian commented 5 years ago

The https://github.com/ChillBDD/Chill/issues/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 commented 5 years ago

True, but it will also tie Chill to xUnit.

tomachristian commented 5 years ago

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 commented 5 years ago

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

tomachristian commented 5 years ago

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.