search

testcontainers / testcontainers-dotnet

A library to support tests with throwaway instances of Docker containers for all compatible .NET Standard versions.
https://dotnet.testcontainers.org
MIT License
3.76k stars 271 forks source link

chore: Ensure that ConfigureAwait is always called on awaited tasks #1117

Closed 0xced closed 7 months ago

0xced commented 7 months ago

What does this PR do?

This pull requests enables code quality analysis and turns CA2007: Do not directly await a Task into an error.

Why is it important?

Testcontainers for .NET calls ConfigureAwait(false) everywhere so this pull request enforces this rule so that authors contributors can't forget about it.

Follow-ups

Note that this was only enabled in the src directory and not at the root of the project to avoid enabling it for the test projects. It becomes problematic in TestcontainersContainerTest because of await using of IAsyncDisposable where it would generate an error.

And now the big question: is it really necessary to always call .ConfigureAwait(true) everywhere in the tests? I understand why calling .ConfigureAwait(false) is important in library code but I don't think it matters at all for the test project. Does it? I think we could suppress all .ConfigureAwait(true) in the tests and increase the readability of the tests.

netlify[bot] commented 7 months ago

Deploy Preview for testcontainers-dotnet ready!

Name Link
Latest commit f20d2873f00eec6d22bdd3b4cf112efb9880f25d
Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/65d795e4e20ade000865e6f6
Deploy Preview https://deploy-preview-1117--testcontainers-dotnet.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

0xced commented 7 months ago

I will just do the cleanup afterwards, and that is fine.

👍

It depends on the test framework, see: xUnit1030.

Thanks for that interesting piece of information!