Coverage for "await foreach" loops and compiler-generated async iterators (issue #1104) #1107

alexthornton1 · 2021-03-04T23:55:09Z

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.

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.

alexthornton1 · 2021-03-05T00:27:54Z

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.

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.

alexthornton1 · 2021-03-06T03:52:40Z

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:

2021-03-06T03:40:56.6123173Z [xUnit.net 00:05:54.17]     Coverlet.Integration.Tests.Msbuild.Test_MultipleTargetFrameworkReport_CoverletOutput_Folder_FileNameWithExtension [FAIL]
2021-03-06T03:40:56.6125752Z   Failed Coverlet.Integration.Tests.Msbuild.Test_MultipleTargetFrameworkReport_CoverletOutput_Folder_FileNameWithExtension [5 m]
2021-03-06T03:40:56.6126854Z   Error Message:
2021-03-06T03:40:56.6128307Z    Command 'dotnet test -c Debug "D:\a\1\s\test\coverlet.integration.tests\bin\Debug\net5.0\0c144e8" /p:CollectCoverage=true /p:Include="[coverletsamplelib.integration.template]*DeepThought" /p:IncludeTestAssembly=true /p:CoverletOutput="D:\a\1\s\test\coverlet.integration.tests\bin\Debug\net5.0\0c144e8"\file.ext' didn't end after 5 minute
2021-03-06T03:40:56.6130099Z   Stack Trace:
2021-03-06T03:40:56.6131191Z      at Coverlet.Integration.Tests.BaseTest.RunCommand(String command, String arguments, String& standardOutput, String& standardError, String workingDirectory) in /_/test/coverlet.integration.tests/BaseTest.cs:line 91
2021-03-06T03:40:56.6132655Z    at Coverlet.Integration.Tests.BaseTest.DotnetCli(String arguments, String& standardOutput, String& standardError, String workingDirectory) in /_/test/coverlet.integration.tests/BaseTest.cs:line 100
2021-03-06T03:40:56.6134080Z    at Coverlet.Integration.Tests.Msbuild.Test_MultipleTargetFrameworkReport_CoverletOutput_Folder_FileNameWithExtension() in /_/test/coverlet.integration.tests/Msbuild.cs:line 182

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:

Passed!  - Failed:     0, Passed:    30, Skipped:     0, Total:    30, Duration: 11 m 30 s - coverlet.integration.tests.dll (net5.0)

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:

            using Stream pkg = File.OpenRead(Directory.GetFiles($"../../../../../bin/{GetAssemblyBuildConfiguration()}/Packages", filter).Single());

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.

MarcoRossignoli · 2021-03-06T13:29:17Z

/azp run

azure-pipelines · 2021-03-06T13:29:27Z

Azure Pipelines successfully started running 1 pipeline(s).

MarcoRossignoli · 2021-03-06T13:31:07Z

Thx @alexanderkozlenko!

cc: @daveMueller for help on review

MarcoRossignoli

Some nits but looks georgeous!

src/coverlet.core/Symbols/CecilSymbolHelper.cs

MarcoRossignoli · 2021-03-06T14:49:42Z

@alexthornton1 can you attach an image of the resulting report?
You can run test passing true to show for extension method GenerateReport https://github.com/coverlet-coverage/coverlet/blob/master/test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs#L23

MarcoRossignoli · 2021-03-06T14:52:56Z

@alexthornton1 feel free to take any other tenet-coverage issue if you like it!
Are the most important ones https://github.com/coverlet-coverage/coverlet/issues?q=is%3Aissue+is%3Aopen+label%3Atenet-coverage

alexthornton1 · 2021-03-06T21:24:05Z

Instrumentation.AsyncForeach.cs

Note that we expect the uncovered portions of the above to be uncovered. SumWithATwist is called with only positive values in its parameter. SumEmpty never enters the loop, since there are no values being iterated.

Instrumentation.AsyncIterator.cs

(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.

MarcoRossignoli · 2021-03-06T21:55:02Z

@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.

MarcoRossignoli · 2021-03-06T21:59:55Z

Anyway it's cosmetics, you did great we can go as-is thanks a lot!

alexthornton1 · 2021-03-06T22:10:44Z

Great. I'm taking a crack at await using (#914), so I'll see about rearranging that code as part of that work.

…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)

alexthornton1 added 2 commits March 4, 2021 15:48

alexthornton1 changed the title ~~Coverage for "await foreach" loops~~ [WIP] Coverage for "await foreach" loops Mar 5, 2021

alexthornton1 changed the title ~~[WIP] Coverage for "await foreach" loops~~ Coverage for "await foreach" loops Mar 6, 2021

alexthornton1 changed the title ~~Coverage for "await foreach" loops~~ Coverage for "await foreach" loops and compiler-generated async iterators (issue #1104) Mar 6, 2021

MarcoRossignoli requested review from MarcoRossignoli and daveMueller March 6, 2021 13:30

MarcoRossignoli added the tenet-coverage Issue related to possible incorrect coverage label Mar 6, 2021

MarcoRossignoli approved these changes Mar 6, 2021

View reviewed changes

src/coverlet.core/Symbols/CecilSymbolHelper.cs Outdated Show resolved Hide resolved

src/coverlet.core/Symbols/CecilSymbolHelper.cs Show resolved Hide resolved

src/coverlet.core/Symbols/CecilSymbolHelper.cs Show resolved Hide resolved

MarcoRossignoli merged commit b5923ca into coverlet-coverage:master Mar 6, 2021

MarcoRossignoli mentioned this pull request Mar 6, 2021

Partially covered await foreach statement #1104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coverage for "await foreach" loops and compiler-generated async iterators (issue #1104) #1107

Coverage for "await foreach" loops and compiler-generated async iterators (issue #1104) #1107

alexthornton1 commented Mar 4, 2021

alexthornton1 commented Mar 5, 2021

alexthornton1 commented Mar 6, 2021 •

edited

Loading

MarcoRossignoli commented Mar 6, 2021

azure-pipelines bot commented Mar 6, 2021

MarcoRossignoli commented Mar 6, 2021

MarcoRossignoli left a comment

MarcoRossignoli commented Mar 6, 2021

MarcoRossignoli commented Mar 6, 2021

alexthornton1 commented Mar 6, 2021

MarcoRossignoli commented Mar 6, 2021 •

edited

Loading

MarcoRossignoli commented Mar 6, 2021

alexthornton1 commented Mar 6, 2021

Coverage for "await foreach" loops and compiler-generated async iterators (issue #1104) #1107

Coverage for "await foreach" loops and compiler-generated async iterators (issue #1104) #1107

Conversation

alexthornton1 commented Mar 4, 2021

alexthornton1 commented Mar 5, 2021

alexthornton1 commented Mar 6, 2021 • edited Loading

MarcoRossignoli commented Mar 6, 2021

azure-pipelines bot commented Mar 6, 2021

MarcoRossignoli commented Mar 6, 2021

MarcoRossignoli left a comment

Choose a reason for hiding this comment

MarcoRossignoli commented Mar 6, 2021

MarcoRossignoli commented Mar 6, 2021

alexthornton1 commented Mar 6, 2021

MarcoRossignoli commented Mar 6, 2021 • edited Loading

MarcoRossignoli commented Mar 6, 2021

alexthornton1 commented Mar 6, 2021

alexthornton1 commented Mar 6, 2021 •

edited

Loading

MarcoRossignoli commented Mar 6, 2021 •

edited

Loading