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

[MarcoRossignoli]

fixes #275

Similar issue fixed in #158
If we use merge options we need to remove uncoverable branch of async state machine.

For instance if we have 2 different tests project, and first test project doesn't cover async call but the second one yes we end with this situation:
we call branches b1 and b2 and we have(hints)
for first test b1 = 0 b2 = 0
for second test b1 = 1 and b2 = 0 -> in this case we remove b2 because false negative, it depends on async or sync execution(#158)
So if we merge results we have b1 = 1 and b2 = 0 and we need to remove b2 = 0 because will never be covered, it depends on async/sync execution.

Here you can find repro project(thank's to @pape77) https://github.com/MarcoRossignoli/marcorossignoli.github.io/tree/master/src/coverlet/Issue1
before fix

      "Keys.Data.Repositories.KeyRepository/<FindAsync>d__3": {
        "System.Void Keys.Data.Repositories.KeyRepository/<FindAsync>d__3::MoveNext()": {
          "Lines": {
            "27": 2,
            "28": 2,
            "29": 2
          },
          "Branches": [
            {
              "Line": 28,
              "Offset": 61,
              "EndOffset": 127,
              "Path": 1,
              "Ordinal": 1,
              "Hits": 2
            },
            {
              "Line": 28,
              "Offset": 61,
              "EndOffset": 63,
              "Path": 0,
              "Ordinal": 0,
              "Hits": 0
            }
          ]
        }
      },

after fix

      "Keys.Data.Repositories.KeyRepository/<FindAsync>d__3": {
        "System.Void Keys.Data.Repositories.KeyRepository/<FindAsync>d__3::MoveNext()": {
          "Lines": {
            "27": 2,
            "28": 2,
            "29": 2
          },
          "Branches": [
            {
              "Line": 28,
              "Offset": 61,
              "EndOffset": 127,
              "Path": 1,
              "Ordinal": 1,
              "Hits": 2
            }
          ]
        }
      },

/cc @tonerdo @pape77 @kevindqc

[codecov]

Codecov Report

Merging #277 into master will decrease coverage by 1.87%.
The diff coverage is 50%.

@@            Coverage Diff            @@
##           master    #277      +/-   ##
=========================================
- Coverage   89.37%   87.5%   -1.88%     
=========================================
  Files          16      16              
  Lines        1949    2041      +92     
=========================================
+ Hits         1742    1786      +44     
- Misses        207     255      +48

[pape77]

I would extract the logic to a method in a new class not to have duplicated code here and in Coverage.cs :)

[MarcoRossignoli]

I would extract the logic to a method in a new class not to have duplicated code here and in Coverage.cs :)

Logic is the same but objects and place are different, Branch vs BranchInfo, different collections objects and different if conditions. I'll try to understand if I can dedup.

[pape77]

@MarcoRossignoli I slightly modified the project that I sent you earlier in the issue #275 and the same seems to happen. I'll post it in the issue for you to take a look

[MarcoRossignoli]

I did some changes now I'm sure that MoveNext() comes from async state machine, the only way I found to recognize it on merge it was add one property on branche structure when persist it, otherwise I should reload lib and search method name literal. New "coverlet" format would be i.e.:

"Keys.Data.Repositories.KeyRepository/<FindAsync>d__3": {
        "System.Void Keys.Data.Repositories.KeyRepository/<FindAsync>d__3::MoveNext()": {
          "Lines": {
            "27": 2,
            "28": 2,
            "29": 2
          },
          "Branches": [
            {
              "Line": 28,
              "Offset": 61,
              "EndOffset": 127,
              "Path": 1,
              "Ordinal": 1,
              "Hits": 2,
              "IsAsyncStateMachineBranch": true
            }
          ]
        }
      },

/cc @pape77 @tonerdo

EDIT: Skip this message, I changed strategy

[MarcoRossignoli]

I did another amend, I found a way without touch json report. Forget last message.

/cc @pape77 @tonerdo

[pape77]

@MarcoRossignoli
You can also get rid of the IsMoveNext method in CecilSymbolHelper
My only doubt is, I see the GetBranchPoints in that class is still using the regex of moveNext , could this be an issue?

[MarcoRossignoli]

@MarcoRossignoli
You can also get rid of the IsMoveNext method in CecilSymbolHelper
My only doubt is, I see the GetBranchPoints in that class is still using the regex of moveNext , could this be an issue?

In past I only exposed IsMoveNext for my usage...we cannot remove that code...is foundamental for instrumentation...maybe we could make it private again.
EDIT: Actually you'are right...I exposed method...I forgot it, I'll remove it! Internal code use directly RegEx object.

[MarcoRossignoli]

@pape77 have you tried if this fix works?

[pape77]

@MarcoRossignoli yes! Works for me. I'm using it in my own fork of coverlet ;)

[tonerdo]

@pape77 are there any noticeable regressions in other branch related functionality?
@MarcoRossignoli do you think you can add a test or two for this fix?

[MarcoRossignoli]

@pape77 are there any noticeable regressions in other branch related functionality?

The only regression I could see is more memory pressure due to instrumentedResult.AsyncMachineStateMethod string list(one record for every async/await method more or less), because to avoid to "search" method(too slow to me) I use a memory list cache, another approach could be write to file and check on it(but more plumbing and maybe slower than memory approach).
Another "imperfect" approach is to check for method ends with ::MoveNext() but we could have false positive.

@MarcoRossignoli do you think you can add a test or two for this fix?

I'll take a look!

[pape77]

@tonerdo not that I noticed. We use this for several projects at my work. Everything looks great now

[tonerdo]

@MarcoRossignoli seems you need to fix a rebase. Will merge once that's done. Sorry for the delay on this

[MarcoRossignoli]

Will merge once that's done

Merged

Sorry for the delay on this

No problem!Thank's again for coverlet!

[topres]

Did this change get in the 2.5.1 release on NuGet? I still seem to have missing branch coverage on my await statements

[MarcoRossignoli]

@topres I think so https://github.com/tonerdo/coverlet/releases https://github.com/tonerdo/coverlet/blob/master/coverlet.msbuild.nuspec#L5
Can you provide a repro?