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

Move to an async first API #770

Closed
wants to merge 17 commits into from
Closed

Move to an async first API #770

wants to merge 17 commits into from

Conversation

SimonCropp
Copy link
Contributor

No description provided.

@egil
Copy link
Member

egil commented Jun 23, 2022

You might want to check #757. Seems related.
I assume this is not for merging.

@SimonCropp
Copy link
Contributor Author

@egil

So this was an experiment to see is i could get it all working in an async manner. There are still a few more items to do, but i think this stands as a proof of concept.

Why:

  • we already have async tests. so the value of bunit being "mostly not async" has little value for us
  • we get intermittent deadlocks when calling bunit APIs. usually when using the WaitFor* methods. I havnt raised them since it is very difficult to repro
  • Most commonly, when we do a render, we dont want to execute the next line until the render is complete. so we want to await it

I assume this is not for merging.

So yes i think this should be merged, and think is should be the approach used in the next major of Bunit

@SimonCropp SimonCropp changed the title Checking if async works in CI Move to an async first API Jun 24, 2022
@SimonCropp
Copy link
Contributor Author

note i took a few liberties to get it building on my machine. if you decide to accept this approach. i will clean them up

@egil
Copy link
Member

egil commented Jun 24, 2022

Ok, we have a few major things right now planned for v2 (code will live in the v2 branch).

  • drop support for netstandard2.1 and NET5
  • build and update DOM tree directly in AngleSharp
  • use single synchronization context for both test and renderer. That should solve race conditions between test code and renderer code, for which we currently have a whole bunch of complex logic around, like locks.
  • switch to async versions of WaitForXxx methods and introduce a async version of Render (see linked draft PR).

The last point is related to what you are doing, but I have started with building and updating DOM directly, since that will have the biggest impact on the TestRenderer.

I appreciate your efforts, but won't promise we'll take this PR as is and merge it. But we'll definitely look through it and see what changes you have made and include the parts we can reuse.

@egil
Copy link
Member

egil commented Jun 24, 2022

we get intermittent deadlocks when calling bunit APIs. usually when using the WaitFor* methods. I havnt raised them since it is very difficult to repro

Does this still happen with the latest release?

If so, I would appreciate it if you can run your test in CI with the --blame-dump switches (see our verification workflow) and save any dumps you get so we can investigate and fix these issues for V1 as well.

This dump files allows me to see the exact place where two or more threads are stuck and usually gives me a good idea for how to fix it.

@egil
Copy link
Member

egil commented Aug 15, 2022

Im going to close this Simon. Thanks for your suggestions and input on this. For V2 of bUnit, which are in the works, there will be async APIs added where it makes sense. In particular, there will be async Render/RenderComponent methods as well as event handler trigger methods such as Click, where the behavior will be adjusted to work better.

Once the V2 branch have these changes, we'll see about back porting them to V1, if that's possible to do in a non-breaking manner.

@egil egil closed this Aug 15, 2022
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

Successfully merging this pull request may close these issues.

2 participants