Replace public async void with public async Task for XUnit

[MichelZ]

Fixes #2947

[MichelZ]

@dotnet-policy-service agree

[benrr101]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 72.29%. Comparing base (699f6ab) to head (0fb95ae). Report is 30 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #2948 +/- ## ========================================== + Coverage 72.23% 72.29% +0.06% ========================================== Files 291 288 -3 Lines 59769 59660 -109 ========================================== - Hits 43172 43131 -41 + Misses 16597 16529 -68 ``` | [Flag](https://app.codecov.io/gh/dotnet/SqlClient/pull/2948/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | Coverage Δ | | |---|---|---| | [addons](https://app.codecov.io/gh/dotnet/SqlClient/pull/2948/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `92.58% <ø> (-0.33%)` | :arrow_down: | | [netcore](https://app.codecov.io/gh/dotnet/SqlClient/pull/2948/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `75.38% <ø> (-0.42%)` | :arrow_down: | | [netfx](https://app.codecov.io/gh/dotnet/SqlClient/pull/2948/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `70.69% <ø> (+0.09%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[MichelZ]

Hmm. I upgraded XUnit and it gave Warnings about these. I‘m not sure why it wouldn‘t warn about the others, but of course I/we can change them as well

[MichelZ]

I think the reason the others are not flagged is that they don't use the [Fact] or [Theory] attributes, but the [ConditionalTheory]. The analyzer probably ignores those. I'll update them as well

[MichelZ]

Done.

There are also async void's in the BenchmarkRunners for Benchmark.NET, I'm not too familiar with Benchmark.NET, so this might be on purpose? If not, I can also fix them in a separate PR.

Additionally, there is one async void in SNIPacket.cs here: https://github.com/dotnet/SqlClient/blob/9d5ca32e666b3da1cc27b4ab145367308998b147/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs#L330

Not sure if that needs fixing, too (in another PR)

[MichelZ]

Thanks for both reporting the issue and fixing the issue :D I think the likelihood that we'll upgrade xunit versions anytime soon is sorta slim - iirc there's a dependency issue with a library provided by the dotnet team, that won't work with a newer version. It's been like 7 months since I looked at that, so it might not be a concern anymore. But, regardless, this is the proper way to do async tests, so we should be doing this.

I'll kick off a CI build, and assuming it passes without issue (no reason it shouldn't), we'll get it merged 🚢

You mean this comment? https://github.com/dotnet/SqlClient/pull/2379#issuecomment-1979544650

Wasn't this fixed by upgrading to .NET8 now? (.NET 6 was removed) Do you think I can add a PR to upgrade to the latest versions?

[edwardneal]

Thanks. I don't think the BenchmarkRunners will make much difference, but it's worth testing with/without.

Additionally, there is one async void in SNIPacket.cs here:

Unfortunately this is a different case. It's intentional - you can see that WriteToStreamAsync invokes the callback itself. WriteToStreamAsync is called by SNITcpHandle.SendAsync, which supplies the handle's send callback. SendAsync effectively starts the task and abandons it until the callback is invoked. It does that because that's how the native SNI layer works.

In theory, I think it'd be possible to move to a Task-first approach; (where the managed SNI layer uses Tasks in line with normal .NET, and the native SNI layer takes on the responsibility for marshalling between its master "write complete" callback and TaskCompletionSources) I think this'd be an improvement and if practical, I'd help with that. In practice, I'd want to do that on a merged and stable TdsParser and TdsParserStateObject layer; it's possible that SqlClientX is a more promising option by the time that the merge/stabilisation is done.

[MichelZ]

Let's see how TdsParser goes... I'm curious about SqlClientX and what the roadmap/timeframe looks for that

[benrr101]

Do you think I can add a PR to upgrade to the latest versions?

Yknow what, I think you're right. Since we dropped net6 we should be able to take the latest version of Microsoft.DotNet.XUnitExtensions and probably take the latest of XUnit (might even be a requirement for the aforementioned). I'm always a bit hesitant with this codebase, but once this one is checked in, I would support a PR that upgrades XUnit/extensions

I'm curious about SqlClientX and what the roadmap/timeframe looks for that

... same 😅

[benrr101]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).