MicrosoftPremier / VstsExtensions

Documentation and issue tracking for Microsoft Premier Services Visual Studio Team Services Extensions
MIT License
56 stars 14 forks source link

Branch Coverage Uses Wrong Total Branches Value #120

Closed ardalis closed 3 years ago

ardalis commented 3 years ago

We have recently added the extension to our builds which have been using code coverage for quite some time. For one project we are getting this behavior:

image

Total branches: 341 Covered branches: 106

However, looking at the code coverage tab for this same build, we see this:

image

106 of 113 branches.

My initial thought was that the issue might be related to 0/0 branches, since the code coverage report shows quite a few of these. However, after updating our YAML as follows, the behavior remains unchanged:

                - task: BuildQualityChecks@7
                  displayName: 'Check build branches quality'
                  inputs:
                    checkCoverage: true
                    coverageThreshold: 85
                    coverageType: branches
                    coverageFailOption: fixed
                    treat0of0as100: true

What else should we try?

ReneSchumacher commented 3 years ago

Hi Steve,

I assume you're using IstanbulJS to create the Cobertura report for code coverage, right? If so, this is a known bug in the cobertura report created by IstanbulJS. The Cobertura report contains a summary value for coverage as well es details for every class, function, and line of code. Azure DevOps, and thus BQC, only looks at the summary values, which have the correct value for coverage, while ReportGenerator ignores the summary values and looks at the details. Unfortunately, IstanbulJS does not correctly mark some of the branches in the Cobertura report, which leads to wrong numbrers.

I already fixed the issue in a PR months ago (see https://github.com/istanbuljs/istanbuljs/pull/518), but it still hasn't been accepted. I'm not even sure it will be accepted as the project is not very actively maintained anymore :( Nevertheless, I'll try my best to bring the maintainer to consider my PR.

ardalis commented 3 years ago

We're not using istanbuljs anywhere directly - does ReportGenerator use it?

Here's what we have (after running dotnet test to generate code coverage files):

    - task: PublishTestResults@2
      displayName: Publish Test Results
      inputs:
        testRunner: 'VSTest'
        testResultsFiles: '**/*.trx'

    - task: Palmmedia.reportgenerator.reportgenerator-build-release-task.reportgenerator@4
      displayName: Test Code Coverage Report Generator
      inputs:
        reports: '$(Build.SourcesDirectory)/tests/{unittests}/coverage.cobertura.xml;$(Build.SourcesDirectory)/tests/{integrationtests}/coverage.cobertura.xml;$(Build.SourcesDirectory)/tests/{functionaltests}/coverage.cobertura.xml;'
        targetdir: '$(Build.SourcesDirectory)/CodeCoverage'
        reporttypes: 'HtmlInline_AzurePipelines;Cobertura;Badges'
        assemblyfilters: '-xunit*'  

    - task: PublishCodeCoverageResults@1
      displayName: 'Publish code coverage'
      inputs:
        codeCoverageTool: Cobertura
        summaryFileLocation: '$(Build.SourcesDirectory)/CodeCoverage/Cobertura.xml'
        reportDirectory: '$(Build.SourcesDirectory)/CodeCoverage'
ReneSchumacher commented 3 years ago

Interesting. So you use dotnet test to run your tests and generate code coverage with coverlet?

ardalis commented 3 years ago

Correct. Running tests:

dotnet test ./tests/Project.UnitTests/Project.UnitTests.csproj -c Release -l:trx /p:CollectCoverage=true /p:CoverletOutputFormat=cobertura /p:Exclude="$ExcludeList"

test csproj:

<PropertyGroup>
    <TargetFramework>netcoreapp3.1</TargetFramework>
    <IsPackable>false</IsPackable>
    <IsTestProject>true</IsTestProject>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="coverlet.collector" Version="1.0.1" />
    <PackageReference Include="coverlet.msbuild" Version="2.9.0">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
    </PackageReference>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.4.0" />
    <PackageReference Include="Moq" Version="4.13.1" />
    <PackageReference Include="xunit" Version="2.4.1" />
    <PackageReference Include="xunit.runner.visualstudio" Version="2.4.1">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
    </PackageReference>
  </ItemGroup>
ReneSchumacher commented 3 years ago

Alright, let me check the coverlet Cobertura report generator then.

ardalis commented 3 years ago

Thanks. The generated cobertura.xml file has this:

<coverage line-rate="0.2936689549961861" branch-rate="0.31085043988269795" lines-covered="385" lines-valid="1311" branches-covered="106" branches-valid="341" complexity="168" version="0" timestamp="1602605803">

So I think you're off the hook, but I still need to figure out why the HTML and XML generated don't match.

ardalis commented 3 years ago

This may be related: https://github.com/danielpalme/ReportGenerator/issues/191

ReneSchumacher commented 3 years ago

Hmm, interesting. I know a few cases where branches might not be reported in the details at all. One example from the BQC code is this:

    public static build(logger: ILogger): BuildApiHelperSettings {
        let logRawData = getVariable('BQC.LogRawData') != undefined && getVariable('BQC.LogRawData').toLowerCase() == 'true';
        let provider = getVariable('Build.Repository.Provider');
        let branchRef = (provider && provider.toLowerCase().indexOf('git') >= 0) ? (getInput('baseBranchRef') || getVariable('Build.SourceBranch')) : null;
        return <BuildApiHelperSettings> {
            baselineBranchRef: BuildApiHelperSettingsBuilder.fixBranchRef(branchRef, logger),
            baselineDefinitionId: Number(getInput('baseDefinitionId')) || Number(getVariable('System.DefinitionId')),
            includePartiallySucceeded: getBoolInput('includePartiallySucceeded', true),
            logRawData: logRawData
        };
    }

Within the return there's clearly a branch on the line setting baselineDefinitionId. Yet, in the Cobertura report I see generated by (in our case IstanbulJS) the whole return statement is treated as a single line. I start to suspect that those are the types of branches that are actually seen by the instrumentation process but not reported in Cobertura. I haven't taken a deeper look into this yet, though.

ReneSchumacher commented 3 years ago

I set up a small sample and could reproduce the issue with TypeScript and IstanbulJS. The instrumentation process apparently knows about the lines and branches in the return statement and correctly calculates the branch coverage in the summary. However, the report details don't contain any line information for the lines within the return statement, which leads to ReportGenerator not seeing that information as well. Hence, the values shown in the generated report are wrong. The root cause is IstanbulJS in that case as it clearly doesn't create a correct cobertura report.

I couldn't reproduce this with .NET Core, though. I basically used the same code (to the possible extent), but the generated cobertura report has all the necessary details. So I'm not exactly sure where the root cause is in your case. Could be lambda expressions (I've seen coverage issue with those as well).

ardalis commented 3 years ago

Solved my client's problem - details here: https://github.com/danielpalme/ReportGenerator/issues/393.