ContextPropertiesCrossAsyncCalls test failure fix on CI:
#1113
Conversation
- force schedule async continuations on a new thread
| Task t; | ||
| try | ||
| { | ||
| t = testAction(); |
There was a problem hiding this comment.
Not sure at all about what I'm saying, but ... shouldn't there be an await here (i.e. in the try instead of after the finally ?
There was a problem hiding this comment.
Thanks for the feedback! The problem with reverting sync context after await is that on await point I'm not owning thread anymore and this thread will lose its original context (Xunit.Sdk.AsyncTestSyncContext) so I'm not sure that this will be safe for other tests or xunit. After await point I'll be already likely on a different thread anyway. You can set/reset safely while you own the thread, for example, for sync implementations. That's why I can't use here using statement.
| @@ -383,4 +401,9 @@ public bool IsCallable() | |||
| } | |||
| } | |||
| #endif | |||
|
|
|||
| class ForceNewThreadSyncContext : SynchronizationContext | |||
There was a problem hiding this comment.
Mostly a personal opinion, but maybe having a IDisposable class would make the test code a bit more readable (less aync lambdas , and less indentation generally) , a bit like https://github.com/serilog/serilog/blob/dev/test/Serilog.Tests/Support/TemporarySelfLog.cs and its usage in
It would be nice to have test code that looks like :
using(NewThreadSynchronizationContext.Start()){
// the code that needs to be tested with that SynchronizationContext
}There was a problem hiding this comment.
couldn't use await inside disposable block, see explanation above
|
Awesome stuff, I didn't notice we'd taken an implicit dependency on the test runner implementation, here. |
What issue does this PR address?
fixes 1112
Does this PR introduce a breaking change?
Please check if the PR fulfills these requirements