tag @dotnet/roslyn-ide @CyrusNajmabadi

👍. Are there existing ConfigureAwaits that can now be removed?

What's the motivation for this change? In general I prefer to hold shipping code and test code to the same quality bar. Hence my first thought when reading this was "why is this necessary for source code but not test code?"

Test code you want ConfigureAwait(true) (i.e. the default) so we're clearly running on the test scheduler if it's WpfFact. Product code we never want that. So yes, this isn't a matter of quality bars, but rather the domains are actually different.

Why are the domains different though? It's not immediately obvious to me.

Perhaps your response should be attached to a comment on top of the disable.

In our product code for the workspaces and higher, we always want to ConfigureAwait(false) to ensure all continuations run on the thread pool. That way people can do .Result on a task rather than risk deadlocks. This is an API guarantee we make. For tests, they're either already running on the thread pool (so ConfigureAwait(false) doesn't do anything) or they're running on our STA thread stuff, and we want to run the continuation there. So ConfigureAwait(false) would be a bug.

Make sense?

Makes sense. I still think this should be documented somewhere.

@davkean, can you add a comment to the ruleset file?

Added a comment and sorted it, but make note this is UI edited file - so now everyone's going to have to edit it by hand.

Whoops, pushed again with the changes I said I made.

I'm not happy with this change. I prefer ConfigureAwait always be used, whether true or false, so the intention is clear. I don't see test code as an exception to this.

@sharwell Your concern is noted, but as a team, we've decided to move tests away from having to specify this, given the behavior is almost always correct.

I'm not happy with this change. I prefer ConfigureAwait always be used, whether true or false, so the intention is clear. I don't see test code as an exception to this.

I disagree (mainly because I have to maintain this code). In test code, 100% (or really, like 99.9999999%) of the code is supposed to use ConfigureAwait(true). That means a massive amount of clutter and obfuscation of code just to maintain the status-quo.

In test code, the default is correct and never changed. This is vastly different from the rest of our product code. For that code, almost all code needs ConfigureAwait(false), and some code critically does not need that. As such, we want to call out explicitly both cases and we want people to always have to think about this.

In test code you don't need to think about it, and it is just a tax.

@CyrusNajmabadi that reasoning makes sense.

However we're now sending a bit of a mixed message. It's not required anymore but there is a large amount of test code out there which makes it appear that it's required (I know as I added quite a bit of it). Have we thought about quickly cleaning this up and removing the unnecessary uses?

@jaredpar Yep we've talked about, if you only you were in our team room. :)

I'll file a issue to track removing it.

@davkean I'm down the hall. If you all would just shout your discussions we wouldn't have to communicate via github.

@jaredpar But then I would have to hunt you down for excluding the community! 😆

Don't worry, I can shout pretty loudly.