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

Code Coverage for Affected Projects only #12

Open
leonardochaia opened this issue Jul 10, 2021 · 10 comments
Open

Code Coverage for Affected Projects only #12

leonardochaia opened this issue Jul 10, 2021 · 10 comments
Labels
enhancement New feature or request

Comments

@leonardochaia
Copy link
Owner

When testing only what's affected, the coverage report will show incorrect results.

For example, given this project structure:

  1. Project.Shared
  2. Project (.1)
  3. Project.Tests (depends on .2)
  4. Shared.Tests (depends on .1)

If .2 has changes, .3 will be affected so we will build and test .2 and .3, but there's no need to test .4 since .1 has not changed.
Hence, the coverage report will show a low coverage percentage for .1 since its test have not run

We should be able to filter down the coverage report to only include the resulting projects of dotnet-affected.

@leonardochaia leonardochaia added the enhancement New feature or request label Jul 10, 2021
@nvsoares
Copy link

Hi Leonardo.
We are setting up a CI pipeline in our monorepo project and we would like to know if you will take care of this issue soon.
We like to use this tool but without this feature we will be getting incorrect data and that compromises the test coverage report.

@leonardochaia
Copy link
Owner Author

Hi @nvsoares , we are quite close of having this implemented.

  1. We have an abstraction, the IOutputFormatter which can be used to format the affected projects into an output file
  2. The TextOutputFormatter can be used as inspiration for writing new ones.

The plan is to be able to output Corvelet --include filter expression

I can't confirm when I will be able to implement this feature, so would you guys considering contributing an IOutputFormatter implementation for Corvelet or your testing processor of use?

@nvsoares
Copy link

Hi @leonardochaia.

We are implementing an OutputFormatter for Coverlet when used with MSBuild.
Since we are not yet very comfortable with the project, we would like you to say if this is what is intended.

First, we have changed the ProjectInfo class to include the AssemblyName.

public ProjectInfo(ProjectGraphNode node)
{
    this.Name = node.GetProjectName();
    this.AssemblyName = node.GetProjectAssemblyName();
    this.FilePath = node.ProjectInstance.FullPath;
}

Then, we have created the CoverletIncludeOutputFormatter like this:

internal class CoverletIncludeOutputFormatter: IOutputFormatter
{
    public string Type => "text";
    public string NewFileExtension => ".txt";
    public Task<string> Format(IEnumerable<IProjectInfo> projects)
    {
        List<string> assemblies = new List<string>();
        foreach (var project in projects)
        {
            assemblies.Add($"[{project.AssemblyName}]*");
        }
        return Task.FromResult($"{string.Join(",", assemblies)}");
    }
}

If I understand, the idea is to pass the content of the created file to /p:Include=.

@leonardochaia
Copy link
Owner Author

Hey @nvsoares , looking good 👍
I didn't notice we needed the assembly name, but adding to ProjectInfo sounds ok.

I think the output should be the filter expression (like [<Assembly>]*) ready to pass to corvelet.
I.e, usage would be

dotnet affected <....> -f corvelet
dotnet test affected.proj /p:CollectCoverage=true /p:Include=(cat corvelet.txt)

I haven't tested this to check the filter expression works but we want all types from all affected assemblies so I think it should be something like according to filter docs

[<assembly 1>]*, [<assembly2]*, <... etc>

@JoaoEdu93
Copy link

Hi @leonardochaia.
I'm a co-worker of @nvsoares and I'm also working on this issue.
I have a case here that fails with this strategy. For example we have the following projects:

  1. Project
  2. Project.Tests

If the changes are only to .2, then this is the only affected project. However, it is not this one that we should put in the include filter, but .1.

Do you have any idea what we should do?

@leonardochaia
Copy link
Owner Author

Hmm, interesting but yes makes sense. Basically it expects the "test covered project" as the filter target, instead of the "project that has the tests". Makes sense.

So, haven't really thought this trough but it looks like we need to compose a filter that includes all projects that .2 references

In which case it looks like the formatter needs to know the ReferencingProjects in order to compose the filter. I think we may need to expand ProjectInfo to include the flat list of ReferencingProjects. I may be able to take a look in the upcoming days.

@JoaoEdu93
Copy link

I think that include all references does not solve all the cases.
Let's look at the following scenario:

  1. Shared (no dependencies)
  2. Shared.Tests (depends on .1)
  3. Project (no dependencies)
  4. Project.Tests (depends on .3 and .1)

The only affected project is the .4. So, we will run the tests only for project .4.
Since project .4 references .1 and .3, these will be set in the include filter.
The results are as follows:

  1. Shared -> 0% (Tests didn't run)
  2. Project -> 100%

The total coverage will be 50%, since the testing of the .1 project has not been run.

The only solution I have in mind is to use some convention in the naming of the test projects or have some sort of configuration file.

@leonardochaia
Copy link
Owner Author

leonardochaia commented Mar 3, 2023

Hmmm I see..
Again, without thinking this through too much but:

The only affected project is the .4. So, we will run the tests only for project .4.
Since project .4 references .1 and .3, these will be set in the include filter.

What if the formatter filters out the 4. ReferencingProjects to only include the ReferencingProjects that were built?

Sorry for the vague responses but I think I need to sit down a bit and think this through with some dummy repo to see how it behaves.

EDIT: right they were not built so this does not work

@nvsoares
Copy link

Hello Leonardo.
Have you been able to take the time to look into this question? It is very important to us.
Thank you.

@leonardochaia
Copy link
Owner Author

Hi @nvsoares @JoaoEdu93 , sorry I have not been able to review this yet. Been having some busy weeks at work / life.
Will try to get back to it as soon as possible.

Regards,
Leo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants