Try to fix branch coverage with async calls bug #158

MarcoRossignoli · 2018-07-19T10:35:38Z

fixes #113

The issue is related to compiler generated state machine:

...
TaskAwaiter awaiter;
if (num != 0)
{
    CoverageTracker.MarkExecuted(@"...\AppData\Local\Temp\CoverletAwaitBranchCoverage_04c15549-0ca8-4cd2-977b-01bd0a8ea52e", @"L,...\Git\CoverletAwaitBranchCoverage\CoverletAwaitBranchCoverage\Program.cs,9,9");
    CoverageTracker.MarkExecuted(@"...\AppData\Local\Temp\CoverletAwaitBranchCoverage_04c15549-0ca8-4cd2-977b-01bd0a8ea52e", @"L,...\Git\CoverletAwaitBranchCoverage\CoverletAwaitBranchCoverage\Program.cs,10,10");
    awaiter = Task.get_CompletedTask().GetAwaiter();
    if (!awaiter.IsCompleted)
    {
        CoverageTracker.MarkExecuted(@"...\AppData\Local\Temp\CoverletAwaitBranchCoverage_04c15549-0ca8-4cd2-977b-01bd0a8ea52e", @"B,...\Git\CoverletAwaitBranchCoverage\CoverletAwaitBranchCoverage\Program.cs,10,0");
        this.<>1__state = num = 0;
        this.<>u__1 = awaiter;
        Program.<Main>d__0 stateMachine = this;
        this.<>t__builder.AwaitUnsafeOnCompleted<TaskAwaiter, Program.<Main>d__0>(ref awaiter, ref stateMachine);
        return;
    }
}
...

If task complete synchronously this branch doens't hit.
I borrowed this idea from OpenCover.
I tested 2 strategies:
3edf519#diff-9eb1afa9bd8141e48d456c120df924caR185 this one force hit on not covered branch
06a5e59#diff-9eb1afa9bd8141e48d456c120df924caR199 this one remove completely the branch if not hitted(but other one yes)

I prefer remove the branch...is more real than force hit.
Let me know what do you think.

/cc @tonerdo

tonerdo · 2018-08-06T10:25:11Z

Hi @MarcoRossignoli, sorry for the delay in responding to this. Will take a look and get back to you

MarcoRossignoli · 2018-08-06T12:36:12Z

src/coverlet.core/Coverage.cs

+                    }
+                    foreach (var branchToRemove in branchesToRemove)
+                    {
+                        document.Value.Branches.Remove(branchToRemove.Key);


on next runtime version we can remove directly during enumeration(Dictionary<(int Line, int Ordinal), Branch>) dotnet/coreclr#18854 https://github.com/dotnet/corefx/issues/31459

I'm very interested in helping to get this pr merged - @MarcoRossignoli do you need to update this pr now that they released a new version of the clr last week?

@abe545 thank's for the interest. I'll verify if above amend has been released, however it's not essential to this PR. When @tonerdo will be able to review we'll know if it's good to merge or not!

NB. It's not my intention put pressure on maintainers!@tonerdo take all time you need, and thank's a lot for your work!

Would love to see this merged. Anything holding this back, @tonerdo ?

src/coverlet.core/Coverage.cs

tonerdo

Resolve merge conflicts and this is good to go! Apologies for the hold up on this

MarcoRossignoli · 2018-10-01T07:54:49Z

Apologies for the hold up on this

No problem.

Now result to this https://github.com/abe545/CoverletAwaitBranchCoverage/tree/master:

Starting test execution, please wait...

Total tests: 2. Passed: 2. Failed: 0. Skipped: 0.
Test Run Successful.
Test execution time: 4,6512 Seconds

Calculating coverage result...
  Generating report 'C:\Data\Source\Git\CoverletAwaitBranchCoverage\CoverletAwaitBranchCoverageTest\coverage.json'

+-----------------------------+--------+--------+--------+
| Module                      | Line   | Branch | Method |
+-----------------------------+--------+--------+--------+
| CoverletAwaitBranchCoverage | 100%   | 100%   | 100%   |
+-----------------------------+--------+--------+--------+


Total Line 100%
Total Branch 100%
Total Method 100%

abe545 · 2018-10-01T18:34:45Z

@tonerdo no need to apologize for the delay, and thanks for all your hard work on this project!

…overageasynccalls Try to fix branch coverage with async calls bug

kevindqc · 2018-12-18T15:53:33Z

Is this in coverlet.msbuild v2.4.0? I would assume so since this was done in October and 2.4.0 was published in November, but I get the uncovered branches when using await:

MarcoRossignoli · 2018-12-18T15:57:19Z

@kevindqc could you produce a simple repro?

pape77 · 2018-12-19T16:18:58Z

I have a similar issue with this "ghost branches" which are never covered.
My code looks like this

    public class KeyProvider : IKeyProvider
    {
        private readonly IKeyRepository _repository;
        private readonly IMemoryCache _cache;

        public KeyProvider(IKeyRepository repository, IMemoryCache cache)
        {
            _repository = repository;
            _cache = cache;
        }

        public async Task<Key> GetByUuidAndTypeAsync(Guid uuid, KeyType type, CancellationToken cancellationToken)
        {
            return await _cache.GetOrCreateAsync($"uuid-{uuid.ToString()}-key-{type}",
                async entry => await _repository.FindOrCreateAsync(uuid, type, cancellationToken));
        }

   public class Key
    {
        public int Id { get; set; }

        public Guid Uuid { get; set; }

        [Required]
        public Guid Kid { get; set; }

        [Required]
        public KeyType KeyType { get; set; }
    }

 public enum KeyType
    {
        Cenc,
        Fairplay,
        Aes
    }
}

This code generates 4 branches, 2 of which I'm never able to cover

my tests:

  public class KeyProviderTests
    {
        private readonly Mock<IKeyRepository> _keyRepositoryMock;
        private readonly MemoryCache _memoryCache;
        private readonly Guid _testGuid = Guid.NewGuid();
        private readonly KeyProvider _keyProvider;

        public KeyProviderTests()
        {
            _memoryCache = new MemoryCache(new MemoryCacheOptions());

            _keyRepositoryMock = new Mock<IKeyRepository>();

            _keyProvider = new KeyProvider(_keyRepositoryMock.Object, _memoryCache);
        }

        [Theory(DisplayName = "Should call repository method with correct values")]
        [InlineData(KeyType.Cenc)]
        [InlineData(KeyType.Aes)]
        [InlineData(KeyType.Fairplay)]
        public async Task GetByUuidAndTypeAsync01(KeyType keyType)
        {
            _keyRepositoryMock.Setup(kr =>
                    kr.FindOrCreateAsync(_testGuid, keyType, It.IsAny<CancellationToken>()))
                .ReturnsAsync(new Key());

            await _keyProvider.GetByUuidAndTypeAsync(_testGuid, keyType, new CancellationToken());

            _keyRepositoryMock.Verify(
                kr => kr.FindOrCreateAsync(_testGuid, keyType, It.IsAny<CancellationToken>()), Times.Once);
        }

        [Theory(DisplayName = "Should get key from cache")]
        [InlineData(KeyType.Cenc)]
        [InlineData(KeyType.Aes)]
        [InlineData(KeyType.Fairplay)]
        public async Task GetByUuidAndTypeAsync02(KeyType keyType)
        {
            var cacheKey = $"uuid-{_testGuid.ToString()}-key-{keyType}";
            _memoryCache.Set(cacheKey, new Key());
           
            await _keyProvider.GetByUuidAndTypeAsync(_testGuid, keyType, new CancellationToken());

            _keyRepositoryMock.Verify(
                kr => kr.FindOrCreateAsync(_testGuid, keyType, It.IsAny<CancellationToken>()), Times.Never);
        }
}

The generated branches in my coverage.json are (notice the MoveNext() thing):

     "Keys.Application.Services.KeyProvider/<>c__DisplayClass3_0/<<GetByUuidAndTypeAsync>b__0>d": {
        "System.Void Keys.Application.Services.KeyProvider/<>c__DisplayClass3_0/<<GetByUuidAndTypeAsync>b__0>d::MoveNext()": {
          "Lines": {
            "27": 9
          },
          "Branches": [
            {
              "Line": 27,
              "Offset": 8,
              "EndOffset": 14,
              "Path": 0,
              "Ordinal": 0,
              "Hits": 3
            },
            {
              "Line": 27,
              "Offset": 81,
              "EndOffset": 83,
              "Path": 0,
              "Ordinal": 2,
              "Hits": 0
            },
            {
              "Line": 27,
              "Offset": 8,
              "EndOffset": 119,
              "Path": 1,
              "Ordinal": 1,
              "Hits": 0
            },
            {
              "Line": 27,
              "Offset": 81,
              "EndOffset": 147,
              "Path": 1,
              "Ordinal": 3,
              "Hits": 3
            }
          ]
        }
      },

Line 27 in my code is precisely the async await one:
async entry => await _repository.FindOrCreateAsync(uuid, type, cancellationToken));

hope it's useful

MarcoRossignoli · 2018-12-20T20:07:00Z

@pape77 could create a full solution with you code?I see some references and would be great have buildable solution(sln) with simple 2 project similar to #113 (comment), you can attach it here if you don't want to create a new repo.

pape77 · 2018-12-20T21:15:26Z

@MarcoRossignoli
Here you go:
keys.zip

In folder "test-results" is the generated coverage.json that I get, you should get the same (50% branch coverage on Application proj, due to this two branches not covered in the async method)

Edit: I removed the usage of the cache to make it more clear and visible. Now you will see 2 branches on line 23. One of which is never hit by tests. And won't be possible to hit

MarcoRossignoli · 2018-12-20T21:21:58Z

@pape77 thank's now I'm OOF but I'll take a look ASAP

MarcoRossignoli added 4 commits July 19, 2018 12:18

fix false positive

3edf519

move code

af7b88d

nit: spaces

303a3b4

use remove branch strategy

06a5e59

MarcoRossignoli changed the title ~~Try to fix branch coverage with async calls~~ Try to fix branch coverage with async calls bug Jul 19, 2018

MarcoRossignoli commented Aug 6, 2018

View reviewed changes

tonerdo reviewed Sep 30, 2018

View reviewed changes

src/coverlet.core/Coverage.cs Outdated Show resolved Hide resolved

tonerdo approved these changes Sep 30, 2018

View reviewed changes

MarcoRossignoli added 2 commits October 1, 2018 09:45

merge from master

f005da3

resolve conflicts

5c596ef

tonerdo merged commit 1be9fce into coverlet-coverage:master Oct 2, 2018

MarcoRossignoli deleted the branchcoverageasynccalls branch October 2, 2018 08:50

NorekZ pushed a commit to NorekZ/coverlet that referenced this pull request Nov 8, 2018

Merge pull request coverlet-coverage#158 from MarcoRossignoli/branchc…

b84021a

…overageasynccalls Try to fix branch coverage with async calls bug

This was referenced Dec 21, 2018

False negative with "/p:MergeWith" async/await branches #275

Closed

Fix false negative with "/p:MergeWith" async/await branches #277

Merged

MarcoRossignoli mentioned this pull request Apr 15, 2019

[Bug fix]Incorrect coverage for statement lambda function #369

Closed

MarcoRossignoli mentioned this pull request Apr 30, 2019

Improve instrumentation testability #387

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to fix branch coverage with async calls bug #158

Try to fix branch coverage with async calls bug #158

MarcoRossignoli commented Jul 19, 2018 •

edited

Loading

tonerdo commented Aug 6, 2018

MarcoRossignoli Aug 6, 2018

abe545 Aug 27, 2018

MarcoRossignoli Aug 27, 2018

MarcoRossignoli Aug 27, 2018

seesharper Sep 10, 2018

tonerdo left a comment

MarcoRossignoli commented Oct 1, 2018

abe545 commented Oct 1, 2018

kevindqc commented Dec 18, 2018

MarcoRossignoli commented Dec 18, 2018

pape77 commented Dec 19, 2018 •

edited

Loading

MarcoRossignoli commented Dec 20, 2018

pape77 commented Dec 20, 2018 •

edited

Loading

MarcoRossignoli commented Dec 20, 2018 •

edited

Loading

Try to fix branch coverage with async calls bug #158

Try to fix branch coverage with async calls bug #158

Conversation

MarcoRossignoli commented Jul 19, 2018 • edited Loading

tonerdo commented Aug 6, 2018

MarcoRossignoli Aug 6, 2018

Choose a reason for hiding this comment

abe545 Aug 27, 2018

Choose a reason for hiding this comment

MarcoRossignoli Aug 27, 2018

Choose a reason for hiding this comment

MarcoRossignoli Aug 27, 2018

Choose a reason for hiding this comment

seesharper Sep 10, 2018

Choose a reason for hiding this comment

tonerdo left a comment

Choose a reason for hiding this comment

MarcoRossignoli commented Oct 1, 2018

abe545 commented Oct 1, 2018

kevindqc commented Dec 18, 2018

MarcoRossignoli commented Dec 18, 2018

pape77 commented Dec 19, 2018 • edited Loading

MarcoRossignoli commented Dec 20, 2018

pape77 commented Dec 20, 2018 • edited Loading

MarcoRossignoli commented Dec 20, 2018 • edited Loading

MarcoRossignoli commented Jul 19, 2018 •

edited

Loading

pape77 commented Dec 19, 2018 •

edited

Loading

pape77 commented Dec 20, 2018 •

edited

Loading

MarcoRossignoli commented Dec 20, 2018 •

edited

Loading