Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coverlet.collect covering NUnit tests and resulting with 0% line coverage #1503

Closed
romainf-ubi opened this issue Jul 24, 2023 · 19 comments
Closed
Assignees
Labels
as-designed Expected behaviour duplicate This issue or pull request already exists

Comments

@romainf-ubi
Copy link

romainf-ubi commented Jul 24, 2023

Hi!

TL;DR: some tests are part of the code coverage and some aren't, I don't understand why (tests should not be part of the code coverage IMHO).

I can't share my code as it is proprietary, but I can explain my issue here.

I have several projects in the src/ directory, and each project has it's test counterpart in the tests/ directory. So, for instance, if I have src/MyProject/MyProject.csproj then I'll have tests/MyProject.Tests/MyProject.Tests.csproj.

Here are the packages I use for my test project:

  <ItemGroup>
    <PackageReference Include="coverlet.collector" Version="3.2.0">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
    <PackageReference Include="coverlet.msbuild" Version="3.2.0">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
    <PackageReference Include="FluentAssertions" Version="6.10.0" />
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.5.0" />
    <PackageReference Include="Moq" Version="4.18.4" />
    <PackageReference Include="NUnit" Version="3.13.3" />
    <PackageReference Include="NUnit.Analyzers" Version="3.6.1">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
    <PackageReference Include="NUnit3TestAdapter" Version="4.4.2" />
  </ItemGroup>

In my solution, I have 3 projects in the src/ directory, and 4 projects in my tests/ directory (the extra project is a shared one for common tests utilities).

I use reportgenerator to get a nice Cobertura file that I can integrate with GitLab later.

My issue is this: one test project reports that the tests themselves (i.e. with the [Test] attribute) is not covered and I don't understand why.

So I get this report:

MyApp.Application.Tests                                                                   11.7%
  AutoGeneratedProgram                                                                     0.0%
  MyApp.Application.Jobs.Commands.SubmitJobCommandTests                                    0.0%
  MyApp.Application.Jobs.Queries.GetAllJobsQueryTests                                      0.0%
  MyApp.Application.Jobs.Queries.GetJobByIdQueryTests                                      0.0%
  MyApp.Application.RandomizerModelExtensions                                            100.0%
  MyApp.Application.RandomizerJobExtensions                                              100.0%

I do understand that the Randomizer classes are covered as it is just utility methods without any [Test] attribute, but I don't understand why SubmitJobCommandTests, GetAllJobsQueryTests and GetJobByIdQueryTests have a 0% score for the line coverage.

Here's how SubmitJobCommandTests.cs looks like:

using FluentAssertions;
using Microsoft.Extensions.Logging;
using Moq;

namespace MyApp.Application.Jobs.Commands;

public sealed class SubmitJobCommandTests : TestsBase
{
    private Mock<IJobSource> _jobSourceMock = null!;
    private SubmitJobCommand.Handler _handler = null!;

    [SetUp]
    public void SetUp()
    {
        _jobSourceMock = new Mock<IJobSource>();

        _handler = new SubmitJobCommand.Handler(_jobSourceMock.Object, Mock.Of<ILogger<SubmitJobCommand.Handler>>());
    }

    [Test]
    public async Task HandleQuery_SubmissionValid_ReturnsJobId()
    {
        // Arrange
        var model = Random.GetModel();
        var jobId = Random.NextJobId();
        _jobSourceMock.Setup(x =>
            x.SubmitJobAsync(
                It.IsAny<JobType>(),
                It.IsAny<Model>(),
                It.IsAny<CancellationToken>()))
            .ReturnsAsync(jobId);

        var command = new SubmitJobCommand(JobType.HelloWorld, model);

        // Act
        var result = await _handler.Handle(command);

        // Assert
        result.JobId.Should().Be(jobId);
    }
}

As you can see, it's fairly simple and there are no other methods that could explain why this file would be part of the code coverage. Also, the other test projects don't appear in the code coverage (and I didn't add any [ExcludeFromCodeCoverage] attributes).

@Bertk
Copy link
Collaborator

Bertk commented Aug 24, 2023

Hi,
which package is actually used for code coverage? I see coverlet.collector and coverlet.msbuild in <ItemGroup />.

I use reportgenerator to filter source files as well because some generated files like version.cs appeared in the coverage report. This might be another option for a clean coverage report.

@Bertk Bertk added the discussion Generic discussion on something label Aug 24, 2023
@romainf-ubi
Copy link
Author

Thanks for the follow up!

I'm not 100% sure how to know which package I use, so tell me if my logic is wrong. My guess is that I use coverlet.collector since the command line I run is dotnet test ${PROJECT_SOLUTION_PATH} -c Release --collect="XPlat Code Coverage" --results-directory="TestResults/" (--collect means I use coverlet.collector, isn't it?)

@Bertk
Copy link
Collaborator

Bertk commented Aug 25, 2023

Yes, you are using coverlet.collector package and the identifier is "XPlat Code Coverage".

--collect is a VSTest.Console.exe option

image

@Bertk Bertk added question This issue is a question and removed discussion Generic discussion on something labels Aug 26, 2023
@Bertk Bertk added the waiting for customer Waiting for customer action label Sep 9, 2023
@daveMueller
Copy link
Collaborator

It's a bit hard to say why in your case the test classes are part of the coverage result. By default the coverage of the test assembly shouldn't be part of the result. If this still happens like in your case, you can try to use filters to reduce the noise in the overall coverage e.g. ExcludeByFile. For the collector this is described here.

When I look into your snipped I wonder a bit where your productive code is. I don't see any other reference than

using FluentAssertions;
using Microsoft.Extensions.Logging;
using Moq;

So does the type SubmitJobCommand exist in the same assembly as the test class SubmitJobCommandTests?

@romainf-ubi
Copy link
Author

It's a bit hard to say why in your case the test classes are part of the coverage result. By default the coverage of the test assembly shouldn't be part of the result. If this still happens like in your case, you can try to use filters to reduce the noise in the overall coverage e.g. ExcludeByFile. For the collector this is described here.

Yeah, I guess my questions would be:

  • Have you ever encounter this behavior?
  • Is it intended?
  • How can I help to track down why some of my test classes used by the collector and some aren't?

As a workaround, I think I can simply put a [ExcludeFromCodeCoverage] on the classes that don't behave as expected, but if I opened an issue here, it's that I prefer not to use workaround as a permanent solution. So if I can help debugging this issue, I'll be glad to!

So does the type SubmitJobCommand exist in the same assembly as the test class SubmitJobCommandTests?

Not the same assembly (tests are in their own project), but the same namespace, this is why I don't need a specific using for SubmitJobCommand.

@romainf-ubi
Copy link
Author

@daveMueller I made a synthetic test case: https://github.com/romainf-ubi/bug-tests-in-coverage

The steps to reproduce are explained the README.md

Here is the result:

image

As you can see, the tests class MyApp.Application.CommandTests is included in the coverage.

@romainf-ubi
Copy link
Author

@daveMueller @Bertk did you have time to look at the repo I made to reproduce the bug? It should be enough to remove the "question" and "waiting for customer" tags

@daveMueller daveMueller added with repro Issue with repro in progress and removed waiting for customer Waiting for customer action question This issue is a question labels Oct 26, 2023
@daveMueller
Copy link
Collaborator

This is actually the next issue I wanted to look into but was occupied with other things. 🙏 I don't have in mind how we distinguish test projects from productive projects right now but I will look into it. Could be that there are problems because the projects share the same namespace. But I will look into it and give you some feedback once I figured it out. The repro will definitely help. thanks a lot. 👍

@daveMueller daveMueller self-assigned this Oct 26, 2023
@romainf-ubi
Copy link
Author

Completely understand that you have other priorities right now, no worries 😉

Thanks for the heads up!

@Bertk
Copy link
Collaborator

Bertk commented Oct 27, 2023

I am not sure whether this is acceptable for your scenario but you can use assembly filters.

A) Using coverlet.collector Exclude option (special CLI syntax " -- " separator for command and settings)

dotnet test bug-tests-in-coverage.sln -c Release --collect="XPlat Code Coverage" --results-directory="TestResults/" -- DataCollectionRunSettings.DataCollectors.DataCollector.Configuration.Exclude="-[MyApp.Application.Tests]"

B) Using reportgenerator assemblyfilters option

reportgenerator -reports:"TestResults/*/coverage.cobertura.xml" -targetdir:"CoverageReports/" -reporttypes:"HtmlSummary;TextSummary" -assemblyfilters:"-MyApp.Application.Tests"

@romainf-ubi
Copy link
Author

romainf-ubi commented Oct 27, 2023

It could be a workaround, but I think this issue should still be considered as a bug since test methods should not be included in the test coverage.

Also imagine that our projects are a bit more complex than the repro given above 😅 We often have more than 5 test projects per solution, and we have around a dozen of solutions in our ecosystem, and we are sharing CI/CD scripts between repositories. So the workaround would quickly hinder the readability and maintainability of our projects.

Right now, we do with the "false" coverage score; we know the real score is a bit better than that. But I think that fixing this issue would allow coverlet users to have more deterministic results in the end.

@Bertk
Copy link
Collaborator

Bertk commented Oct 27, 2023

I am not sure. One of the test projects does not have any implementation (*.cs) and the SDK is generating one main method.
This is a kind of edge scenario and questionable. Why do you have a test project which just refer another test project?

@Bertk
Copy link
Collaborator

Bertk commented Oct 28, 2023

Duplicate of #604

Microsoft.NET.Test.Sdk has Microsoft.NET.Test.Sdk.Program.cs

// <auto-generated> This file has been auto generated. </auto-generated>
using System;
[Microsoft.VisualStudio.TestPlatform.TestSDKAutoGeneratedCode]
class AutoGeneratedProgram {static void Main(string[] args){}}

You can exclude this with

dotnet test bug-tests-in-coverage.sln -c Release --collect="XPlat Code Coverage" --results-directory="TestResults/" -- DataCollectionRunSettings.DataCollectors.DataCollector.Configuration.ExcludeByAttribute="TestSDKAutoGeneratedCode"

@daveMueller
Copy link
Collaborator

I try to explain what I could observe at least in your repro. In general coverlet runs per project and calculates a coverage report for this project. This is also how the dotnet test runners work.
In your repro the project MyApp.Infrastructure.Tests doesn't have any tests. In fact it doesn't have any code at all. Now when coverlet tries to instrument that project, it tries to instruments all assemblies that are somehow used by it. The test assembly itself (MyApp.Infrastructure.Tests.dll) won't be instrumented. But this project has a reference to MyApp.Application.Tests which again has a reference to MyApp.Application. So now imagine you start the test execution just for this project like:

dotnet test ".\tests\MyApp.Infrastructure.Tests\MyApp.Infrastructure.Tests.csproj" -c Debug /p:CollectCoverage=true
  1. We instrument all related assemblies which are:
    • MyApp.Application.Tests.dll
    • MyApp.Application.dll
    • (MyApp.Infrastructure.Tests.dll is excluded by default as it is the called test project)
  2. The test runner starts to execute the found tests in the project (MyApp.Infrastructure.Tests) and then we collect information about the instrumented code that the test run hit. Apparently in this repro the project doesn't execute any tests at all as it doesn't contain any. Nevertheless we compare this result with all the instrumented assemblies.

Thus the result of this is:
grafik

Now to answere your question, you can see the test MyApp.Application.CommandTests in the result because it was instrumented as part of the referenced assemblies.

There maybe is the misconception that coverlet can detect unit test methods and exclude them from the coverage calculation. This is not the case. We can't check the IL for attributes like [Test] or [Fact] as those are just custom attributes bound to a specific test library. Now imagine someone using NUnit for his unit tests and he has written a custom FactAttribute that he uses in his productive code. We then would automatically exclude productive code.

To your initial question. For me it seems that this report wasn't the result of dotnet test MyApp.Application.Tests.csproj as the tests weren't executed. I assume the assembly is just listed because another test (from another assembly) was using MyApp.Application.RandomizerModelExtensions and MyApp.Application.RandomizerJobExtensions.

MyApp.Application.Tests                                                                   11.7%
  AutoGeneratedProgram                                                                     0.0%
  MyApp.Application.Jobs.Commands.SubmitJobCommandTests                                    0.0%
  MyApp.Application.Jobs.Queries.GetAllJobsQueryTests                                      0.0%
  MyApp.Application.Jobs.Queries.GetJobByIdQueryTests                                      0.0%
  MyApp.Application.RandomizerModelExtensions                                            100.0%
  MyApp.Application.RandomizerJobExtensions                                              100.0%

@daveMueller
Copy link
Collaborator

As @Bertk already mentioned #1503 (comment). The class AutoGeneratedProgram is noise generated by the compile and this needs to be filtered out if it bothers.

@daveMueller daveMueller added the as-designed Expected behaviour label Oct 29, 2023
@romainf-ubi
Copy link
Author

@Bertk and @daveMueller:

I am not sure. One of the test projects does not have any implementation (*.cs) and the SDK is generating one main method. This is a kind of edge scenario and questionable. Why do you have a test project which just refer another test project?

In your repro the project MyApp.Infrastructure.Tests doesn't have any tests. In fact it doesn't have any code at all.

The test case is simple because I tried to make the simplest, smallest project to reproduce the issue. So it's not really an edge case as I can ensure you that it happens to me on a much more complex project with several test projects and hundreds of tests in them. Please understand that the test case I made here is but a mere test case and that, by definition, it will look like an edge case as it includes the strict minimum to trigger the issue. The goal of this test case is to make it easier for you to debug and find the issue, by removing all possible noise around it.

Just to be sure, I've added a test in MyApp.Infrastructure.Tests (which was empty before) and I can confirm that the issue is still there. So the presence of tests in the Infrastructure test project, or the lack of it, is not important in reproducing the issue.

@daveMueller: Thanks for the explanations. If I understand well, the root of the issue is that one test project is referencing another test project. If I create a third project which would contain the common code of the other two test project, it should fix the issue. Is that it?

For the rest of the explanations, I have a few questions:

There maybe is the misconception that coverlet can detect unit test methods and exclude them from the coverage calculation. This is not the case. We can't check the IL for attributes like [Test] or [Fact] as those are just custom attributes bound to a specific test library. Now imagine someone using NUnit for his unit tests and he has written a custom FactAttribute that he uses in his productive code. We then would automatically exclude productive code.

Wouldn't it be possible to exclude the full name? (i.e. NUnit.Framework.TestAttribute). Or, since I understand that you probably don't want to create dependencies with NUnit's implementations, would it be possible to add an attribute on the attribute class itself, so that NUnit could change their code and add the attribute saying basically that "any method using this attribute should be excluded from the code coverage"? So they'd have something like that:

namespace NUnit.Framework
{
    [ExcludeFromCodeCoverage] // <-- they'll need to add an attribute (this one or another one)
    [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
    public class TestAttribute : NUnitAttribute, ISimpleTestBuilder, IApplyToTest, IImplyFixture
    {
        /* ... */
    }
}

To your initial question. For me it seems that this report wasn't the result of dotnet test MyApp.Application.Tests.csproj as the tests weren't executed. I assume the assembly is just listed because another test (from another assembly) was using MyApp.Application.RandomizerModelExtensions and MyApp.Application.RandomizerJobExtensions.

You've kinda lost me here... But from your explanations, I can tell you that, in my private project, MyApp.Infrastructure.Tests references MyApp.Application.Tests to indeed use a method from the RandomizerJobExtensions class. So, if I understand well, dotnet test MyApp.Infrastructure.Tests.csproj will exclude MyApp.Infrastructure.Tests by default (as it's the called test project), but it will include MyApp.Application.Tests (as it is a mere reference). Is that it?

@romainf-ubi
Copy link
Author

About the AutoGeneratedProgram, I think that if I fix the issue with the test project referencing another test project, I'll also fix the AutoGeneratedProgram issue as it only exists in test projects as well, which should not have been added in the code coverage in the first place.

@daveMueller
Copy link
Collaborator

@daveMueller: Thanks for the explanations. If I understand well, the root of the issue is that one test project is referencing another test project. If I create a third project which would contain the common code of the other two test project, it should fix the issue. Is that it?

Yes exactly.

Wouldn't it be possible to exclude the full name? (i.e. NUnit.Framework.TestAttribute). Or, since I understand that you probably don't want to create dependencies with NUnit's implementations, would it be possible to add an attribute on the attribute class itself, so that NUnit could change their code and add the attribute saying basically that "any method using this attribute should be excluded from the code coverage"?

Yes this would work but we would only benefit from it if every test library would use the same attribute. Also there is the problem that some people want to collect coverage for their test classes.
Sorry for not mentioning this earlier but coverlet already has the possibility to filter out custom attributes via the ExcludeByAttribute switch. You can totally use it with attributes like Test, TestCase, SetUp, e.g.

dotnet test ".\MyApp.Infrastructure.Tests.csproj" -c Debug --collect:"XPlat Code Coverage;Format=cobertura" -- DataCollectionRunSettings.DataCollectors.DataCollector.Configuration.ExcludeByAttribute=TestAttribute,SetUpAttribute

or use the runsetting file.

So, if I understand well, dotnet test MyApp.Infrastructure.Tests.csproj will exclude MyApp.Infrastructure.Tests by default (as it's the called test project), but it will include MyApp.Application.Tests (as it is a mere reference). Is that it?

Yes exactly.

@romainf-ubi
Copy link
Author

Ok thanks!

I think this issue can be closed as it answered my questions and I don't think there is anything to implement as it is the intended behavior (except maybe a note in the documentation about what is automatically excluded and what's not?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as-designed Expected behaviour duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

3 participants