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

Branch coverage with async calls #113

Closed
coryflucas opened this issue Jun 7, 2018 · 23 comments · Fixed by #158 or #549
Closed

Branch coverage with async calls #113

coryflucas opened this issue Jun 7, 2018 · 23 comments · Fixed by #158 or #549
Assignees
Labels
bug Something isn't working tenet-coverage Issue related to possible incorrect coverage

Comments

@coryflucas
Copy link

I have a project using EF Core with async methods that shows a branch at each await call. One of the branches always shows as uncovered. I've created a sample repo that repoduces the problem here. You can see the branch in the coverage report but the referenced line does not have a branch.

Here is the output of dotnet test:

$ dotnet test /p:CollectCoverage=true
Build started, please wait...
Build completed.

Test run for /Users/clucas/Source/clucas/coverlet-test/Test/bin/Debug/netcoreapp2.1/Test.dll(.NETCoreApp,Version=v2.1)
Microsoft (R) Test Execution Command Line Tool Version 15.7.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...

Total tests: 1. Passed: 1. Failed: 0. Skipped: 0.
Test Run Successful.
Test execution time: 2.0845 Seconds

Calculating coverage result...
  Generating report '/Users/clucas/Source/clucas/coverlet-test/Test/coverage.json'

+---------+--------+--------+--------+
| Module  | Line   | Branch | Method |
+---------+--------+--------+--------+
| Example | 90.9%  | 50%    | 80%    |
+---------+--------+--------+--------+

I tired another example with await HttpClient.GetAsync(...) but that did not seem to produce the same issue so this doesn't appear to apply to all awaited calls.

@mgw854
Copy link

mgw854 commented Jun 7, 2018

I've noticed the same thing. My hunch (totally a guess here, so take it for what it's worth) is that the branch inserted by the compiler for awaiting code (such that you don't actually wait if the task has already completed) is being interpreted as a branch in your code.

If this is true, I don't think it's a meaningful distinction in 99+% of code. There are only two cases where I can think of that this matters. The first is using .ConfigureAwait, which doesn't do anything if the call was never awaited (such that you can accidentally pass on the captured state to a subsequent call that doesn't use .ConfigureAwait). The other is a timing bug where you expecting the current execution to yield, but it doesn't because the task was completed.

@abe545
Copy link

abe545 commented Jun 27, 2018

We've noticed the same thing, and I put together a repro where await Task.CompletedTask causes it to report a partial branch: https://github.com/abe545/CoverletAwaitBranchCoverage/tree/master

@tonerdo tonerdo added bug Something isn't working help wanted labels Jun 28, 2018
@tonerdo
Copy link
Collaborator

tonerdo commented Jun 28, 2018

Thanks for reporting this guys. I'm currently working on general improvements to the coverage logic so will add this to the list. Thanks for the test project @abe545

@rmlira
Copy link

rmlira commented Aug 29, 2018

+1

@sugarjig
Copy link

I'm seeing this issue still in version 2.5.1 (that's the version number in Nuget).

@MarcoRossignoli
Copy link
Collaborator

@sugarjig can you provide a repro?

@soljarka
Copy link

Same with 2.6.0

@MarcoRossignoli
Copy link
Collaborator

@soljarka can you provide a repro?

@Rjae
Copy link

Rjae commented Aug 29, 2019

I don't know if this is old news, but I think I figured out a way through. By adding a simple continuation I was able to get Coverlet to consider all branches hit. For example:

return await _service.SendAsync(command).ContinueWith(x => x.Result);

I hope that helps others.

@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Aug 29, 2019

Thanks @Rjae for the comment...can you provide a repro of the issue (I mean a piece of code without continuation that leads to invalid async branch coverage)?

@Rjae
Copy link

Rjae commented Aug 30, 2019

Sure thing bro...literally the same line without the ContinueWith. So...

return await _service.SendAsync(command);

I've used same approach in several more lines with same success.

@MarcoRossignoli
Copy link
Collaborator

Thanks I'll take a look asap

@MarcoRossignoli MarcoRossignoli self-assigned this Aug 30, 2019
@Rjae
Copy link

Rjae commented Aug 30, 2019

Wow...thanks! Additional "discovery": ContinueWith does not appear to solve the coverage reporting for await async Task, only for await async Task<T>.

@Rjae
Copy link

Rjae commented Aug 31, 2019

BTW, I'm looking for a temporary ignore for these cases. I've tried following attribute-excludes: GeneratedCodeAttribute,CompilerGeneratedAttribute,AsyncStateMachineAttribute but no luck. @MarcoRossignoli if you know a way to ignore these cases from analysis I'd be interested to know.

@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Aug 31, 2019

@Rjae add CompilerGeneratedAttribute should work on next version thank's to #477 but it was merged on 3 July and last verson was released 1 July.
For the moment you can try to doogfood coverlet using this guide https://github.com/tonerdo/coverlet/blob/master/Documentation/ConsumeNightlyBuild.md
When we'll release next version you can revert.
Sorry but for the moment I don't have better choice...we need to fix issue with async(some investigation but I've already an idea on fix) and do a release 😞 so for the moment you can skip using dogfooding and after using new release.
You could also try to skip using filters https://github.com/tonerdo/coverlet/blob/master/Documentation/MSBuildIntegration.md#filters

@Rjae
Copy link

Rjae commented Aug 31, 2019

@MarcoRossignoli again, thanks! Will watch for async fix.

@MarcoRossignoli MarcoRossignoli added tenet-coverage Issue related to possible incorrect coverage and removed help wanted labels Sep 1, 2019
@brad
Copy link

brad commented Feb 24, 2022

Seeing this in the latest coverlet (3.1.2). Using Cake.Coverlet 2.5.4. Happens with all the HttpClient async requests I'm using

  • GetAsync
  • PostAsync
  • PustAsync

ContinueWith does get it fully covered, but it's not a great solution

@brad
Copy link

brad commented Feb 24, 2022

Another workaround I found is to change await httpClient.GetAsync(url) to httpClient.GetAsync(url).Result

@petli
Copy link
Collaborator

petli commented Feb 25, 2022

Hi @brad, thanks for the report. Can you create a new issue on it, to make it possible to track? If you have a small example reproducing the issue it's really useful too.

@nycdotnet
Copy link

@brad note - your suggestion also changes the functionality of the code with regards to asynchrony. Depending on the performance requirements of your code, it may be a really bad tradeoff to work around a coverage issue.

https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#avoid-using-taskresult-and-taskwait

@brad
Copy link

brad commented Feb 25, 2022

@nycdotnet Thanks for the heads up @petli Sure, no problem

@brad
Copy link

brad commented Feb 25, 2022

@petli Is there a forum/chat group to get support? I'm testing this with a minimal application and am not able to reproduce the issue there so clearly I am doing something wrong ☹️

@petli
Copy link
Collaborator

petli commented Feb 25, 2022

@brad There's a discussion forum here: https://github.com/coverlet-coverage/coverlet/discussions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tenet-coverage Issue related to possible incorrect coverage
Projects
None yet