Coverage for "await foreach" loops and compiler-generated async iterators (issue #1104) #1107
Conversation
After attempting to use IAsyncEnumerable<T> in the coverlet.core.tests project, I was met with the following error message: The type 'IAsyncEnumerable<T>' exists in both 'System.Interactive.Async, Version=3.0.3000.0, Culture=neutral, PublicKeyToken=94bc3704cddfc263' and 'System.Runtime, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' After some research, I determined that the root cause was the dependency on an old version of LinqKit.Microsoft.EntityFrameworkCore, which, in turn, includes a dependency on an old version of Entity Framework. To solve that problem, I've upgraded the dependency on LinqKit.Microsoft.EntityFrameworkCore to the latest version in NuGet (5.0.23), as well as the dependency on Microsoft.Extensions.Logging.Abstractions, since it didn't appear to be possible to upgrade one without the other. Since it looks like Coverlet, generally, has moved on to .NET 5, this appears to be non-problematic. Afterward, all tests yielded the same result, but I was then able to begin using IAsyncEnumerable<T> normally.
This commit attempts to fix code coverage for the "await foreach" loop introduced in C# 8. The presence of an "await foreach" loop causes four new kinds of branch patterns to be generated in the async state machine. Three of these are to be eliminated, while the fourth is actually the "should I stay in the loop or not?" branch and is best left in a code coverage report. CecilSymbolHelper now eliminates the three branch patterns that need to be eliminated. There are tests both in CecilSymbolHelperTests as well as a new set of coverage tests in CoverageTests.AsyncForeach.cs.
|
On further review, it appears that issue #1104 is about more than just "await foreach". In particular, this PR fixes "await foreach", but #1104 is also about the code generated for a yield-based method that returns IAsyncEnumerable, which my current PR does not solve. I'll work on adding that to the mix; these go hand-in-hand. |
alexthornton1
changed the title
Fixes coverage for compiler-generated async iterators that arise from async methods that use the "yield" statement to return one element at a time asynchronously via an IAsyncEnumerable<T>. In combination with the previous commit that handles the "async foreach" loop, this should address issue coverlet-coverage#1104.
|
I'm unclear on why there were failures in Windows Debug and Windows Release. Looking at the raw logs, here's the failure that was reported, which boiled down to an integration test taking longer than five minutes to run: Is this an indication that there's something wrong with my changes, or is this an indication of something else? In the most recently-successful test run (from my previous commit, 039e615), those same integration tests took 11 minutes to run, but were deemed successful: I tried running the integration tests locally, but they all fail immediately for a different reason (Coverlet.Integration.Tests.BaseTest.GetPackageVersion finding more than one result on this line, so that the call to Single() fails: I've yet to figure out the right workaround for that to make the integration tests run locally. Happy to help, but not sure yet where to go from here. In the meantime, I took "[WIP]" out of the title of the pull request, as it's possible that this is finished, if the test failure here was spurious. |
alexthornton1
changed the title
alexthornton1
changed the title
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thx @alexanderkozlenko! cc: @daveMueller for help on review |
MarcoRossignoli
added
the
tenet-coverage
|
@alexthornton1 can you attach an image of the resulting report? |
|
@alexthornton1 feel free to take any other |
|
|
(1) The newly-added helper methods in `CecilSymbolHelper` from the
previous commit are now static methods.
(2) In a couple of places where we're checking for a branch following
the loading of compiler-generated fields (one in the `async foreach`
handling and another in the `IAsyncEnumerable` handling), we're now
requiring the loading of the field to be preceded by a "ldarg.0"
instruction.
|
@alexthornton1 sorry I'm on phone and struggling with correct row for comment, a question, is it possible move three helper as a local function of main one where we elide the three new branches?Or are they used also inside async enumerator?I mean if it's possible keep that helpers in the context to have more compact code, this class is becoming huge and hard to read. |
|
Anyway it's cosmetics, you did great we can go as-is thanks a lot! |
|
Great. I'm taking a crack at |
…tors (issue coverlet-coverage#1104) (coverlet-coverage#1107) Coverage for "await foreach" loops and compiler-generated async iterators (issue coverlet-coverage#1104) (coverlet-coverage#1107)
This is an attempt to fix code coverage for the "await foreach" loop introduced in C# 8, as reported in issue #1104.
The presence of an "await foreach" loop causes for new kinds of branch patterns to be generated in the async state machine. Three of these are to be eliminated, while the fourth is actually the "should I stay in the loop or not?" branch and is best left in a code coverage report.
CecilSymbolHelper now eliminates the three branch patterns that need to be eliminated. There are tests both in CecilSymbolHelperTests as well as a new set of coverage tests in CoverageTests.AsyncForeach.cs.