Code coverage drops after updating xunit.runner.visualstudio to v2.5.0

[Bond-009]

After updating xunit.runner.visualstudio our code coverage dropped significantly. Looking at the classes where coverage dropped it shows 2 files one locally and one on GitHub. Based on this I'm assuming that xunit.runner.visualstudio v2.5.0 somehow includes links from Microsoft.SourceLink.GitHub as source files.

Example: https://cov.b0n.dev/jellyfin/Emby.Naming_AlbumParser.html

[bradwilson]

The only changed we've made regarding code coverage was to ensure that the test adapter wasn't included in the coverage result: https://github.com/xunit/visualstudio.xunit/commit/86c0e57f3d8458b8b0f5e436f732dd9546d563dd

That should not impact your code at all. How are you collecting coverage? Can you provide a repro project?

[ValMati]

We have detected the same problem in two different applications. In both cases there are projects that have no tests and some of them are being excluded from the coverage calculation.

We run the tests and generate the code coverage reports in Azure DevOps. These are the tasks:

        # Test Solution
        - task: DotNetCoreCLI@2
          displayName: "Unit Tests"
          inputs:
            command: 'test'
            projects: "$(solution)"
            publishTestResults: false
            arguments: '--filter DisplayName!~MyApplication.IntegrationTests --configuration Release --settings $(Build.SourcesDirectory)\CodeCoverage.runsettings --logger trx --results-directory $(Build.SourcesDirectory)\TestData\'
            nobuild: true
          continueOnError: false

        # Merge code coverage files
        - task: reportgenerator@5
          displayName: "Generate Reports"
          inputs:
            reports: '$(Build.SourcesDirectory)\TestData\**\*.Cobertura.xml'
            targetdir: '$(Build.SourcesDirectory)\coverlet\reports'
            reporttypes: 'HtmlInline_AzurePipelines_Dark;Cobertura'
            verbosity: 'Verbose'

        # Publish code coverage files
        - task: PublishCodeCoverageResults@1
          displayName: "Publish Code Coverage Report"
          inputs:
            codeCoverageTool: 'Cobertura'
            summaryFileLocation: '$(Build.SourcesDirectory)\coverlet\reports\Cobertura.xml'
            failIfCoverageEmpty: true  

[bradwilson]

@ValMati This is still not enough information. Please provide a complete project that illustrates the issue.

[Bond-009]

The Emby.Naming project in the jellyfin solution has this problem

I use this command to get code coverage for the whole solution dotnet test --configuration Release --collect:'XPlat Code Coverage' --settings tests/coverletArgs.runsettings --verbosity minimal

Historic coverage can be seen here: https://cov.b0n.dev/jellyfin/

[ValMati]

@ValMati This is still not enough information. Please provide a complete project that illustrates the issue.

I'm sorry, but the project where I detected the bug is private, from the company I work for, and I can't share it. I have tried to reproduce the bug in an solution as simple as possible, but I have not been able to. It seems that the problem appears with a specific combination of "variables".

[bradwilson]

I have narrowed down the problem to fixing this issue: https://github.com/xunit/visualstudio.xunit/issues/317

The real world implication here is that we used to load every assembly in your test assembly's folder into memory to look for test runner reporters, whereas now we do not (we only load those that have a name matching *reporters*.dll).

The implication is that some of your tests were previously unintentionally dependent on this behavior of pre-loading all assemblies. A quick search of your source I see two instances of AppDomain.CurrentDomain.GetAssemblies() and this one is immediately ringing alarm bells for me: https://github.com/jellyfin/jellyfin/blob/27076755af9474547ea5112d6bc5edb9e6399e4e/tests/Jellyfin.Server.Implementations.Tests/TypedBaseItem/BaseItemKindTests.cs#L10-L31

public static TheoryData<Type> BaseItemKind_TestData()
{
    var data = new TheoryData<Type>();

    var loadedAssemblies = AppDomain.CurrentDomain.GetAssemblies();
    foreach (var assembly in loadedAssemblies)
    {
        if (IsProjectAssemblyName(assembly.FullName))
        {
            var baseItemTypes = assembly.GetTypes()
                .Where(targetType => targetType.IsClass
                                     && !targetType.IsAbstract
                                     && targetType.IsSubclassOf(typeof(MediaBrowser.Controller.Entities.BaseItem)));
            foreach (var baseItemType in baseItemTypes)
            {
                data.Add(baseItemType);
            }
        }
    }

    return data;
}

I believe this is probably the source of the coverage drop issue. (There is a second instance in TypeMapper.GetType() that could also be an issue, though the test one seems like the easier thing to validate first.)

[bradwilson]

Note: My description above applies to the Jellyfin source linked by @Bond-009. Without having access to code from @ValMati the best I can hope for is that you're afflicted by the same type of bug.

[bradwilson]

@ValMati There is an easy way to test if this is your problem: if you have normal code coverage with xunit.runner.visualstudio version 2.5.0-pre.20 but it drops using 2.5.0-pre.22, then it's definitely caused by the assembly loading behavior change. That's the only new code introduced into pre.22.

[ValMati]

@ValMati There is an easy way to test if this is your problem: if you have normal code coverage with xunit.runner.visualstudio version 2.5.0-pre.20 but it drops using 2.5.0-pre.22, then it's definitely caused by the assembly loading behavior change. That's the only new code introduced into pre.22.

I've run the tests with those versions and the result is as you predicted. With version 2.5.0-pre.20 the coverage is the same as with 2.4.5 and with 2.5.0-pre.22 is the same as with 2.5.0.

[ValMati]

I am sorry, but I disagree with the closure of this issue. I think the change in behaviour can be considered a regression as it changes the outcome without changing the inputs.

Moreover, the coverage it gives now is not correct as there are projects that are not accounted for.

[bradwilson]

@ValMati The bug is in your code, not in our code. You were unintentionally depending on behavior that's not longer true. The only way for me to "fix" this would be to revert back to our previous behavior which had performance problems.

You're going to have to fix your code instead. Sorry.