Add ExcludeFromCodeCoverage to template for MSTest, NUnit, XUnit

[MattKotsenas]

This PR is operating under the principle that "defaults matter".

By default, code coverage tools calculate code coverage for all loaded assemblies, including the test assemblies themselves. This tends to artificially inflate the code coverage %, as the test code is included in the calculation. This behavior is often surprising to devs.

There's a mild consensus that test code shouldn't be included in % calculations (see https://about.codecov.io/blog/should-i-include-test-files-in-code-coverage-calculations and https://softwareengineering.stackexchange.com/questions/309015/when-does-it-make-sense-to-include-test-code-in-coverage as examples), and among devs that are calculating %, the prevailing method to do that appears to be setting [ExcludeFromCodeCoverage] on the test assembly (see https://github.com/search?q=%22assembly%3A+ExcludeFromCodeCoverage%22+language%3AC%23&type=code for some hits).

For this change I added the assembly attribute to our templates for MSTest, NUnit, and XUnit to the settings file (adding it for the other projects). This strikes a good balance of leading devs towards the Pit of Success, while letting them remove it if they disagree, similar to what we do for parallelism in MSTest.

[Copilot]

Copilot reviewed 3 out of 11 changed files in this pull request and generated no comments.

Files not reviewed (8)

• template_feed/Microsoft.DotNet.Common.ProjectTemplates.10.0/content/MSTest-FSharp/MSTestSettings.fs: Language not supported
• template_feed/Microsoft.DotNet.Common.ProjectTemplates.10.0/content/MSTest-VisualBasic/MSTestSettings.vb: Language not supported
• template_feed/Microsoft.DotNet.Common.ProjectTemplates.10.0/content/NUnit-FSharp/Company.TestProject1.fsproj: Language not supported
• template_feed/Microsoft.DotNet.Common.ProjectTemplates.10.0/content/NUnit-FSharp/NUnitTestSettings.fs: Language not supported
• template_feed/Microsoft.DotNet.Common.ProjectTemplates.10.0/content/NUnit-VisualBasic/NUnitTestSettings.vb: Language not supported
• template_feed/Microsoft.DotNet.Common.ProjectTemplates.10.0/content/XUnit-FSharp/Company.TestProject1.fsproj: Language not supported
• template_feed/Microsoft.DotNet.Common.ProjectTemplates.10.0/content/XUnit-FSharp/XUnitSettings.fs: Language not supported
• template_feed/Microsoft.DotNet.Common.ProjectTemplates.10.0/content/XUnit-VisualBasic/XUnitSettings.vb: Language not supported

[MattKotsenas]

/cc @baronfel for template changes
/cc @Evangelink for MSTest changes

if there's anyone else I should include or should sign off, please let me know. Thanks!

[MattKotsenas]

Note that there don't appear to be any tests for these templates. I'm happy to add some if someone can clarify where they should go

[Evangelink]

We should also update the playwright templates but let's decide first if we want to have this feature or not.

Personnally I don't really like having coverage on test files but I have seen that many big companies and internal big teams are relying on this feature to understand if their tests were correctly executed or not.

Pinging testing PMs @smithkaylee and @webreidi
Pinging MS Code Coverage owner @fhnaseer (and @jakubch1 FYI)
Pinging Coverlet owner @MarcoRossignoli

[MattKotsenas]

Personnally I don't really like having coverage on test files but I have seen that many big companies and internal big teams are relying on this feature to understand if their tests were correctly executed or not.

I also asked around internal teams for the "looking for dead tests" scenario. It's far from scientific, but what I found was:

1. A few folks cited it as a theoretical issue, but never did it in CI
2. One team that suggested wrapping the attribute in a #if to turn it on / off for a specific test leg

So I agree that some teams may be getting value out of covering tests too. However, I think that would be favoring the smaller percentage of devs with advanced testing scenarios at the expense of the larger group of devs that are overestimating their code coverage and don't realize it. (If we have evidence of the relative size of these groups please share!)

My position is that we're merely providing better defaults for the common use case (and only for new projects). Devs that want test coverage data can always delete the attribute the same way they can delete the parallelization attribute for MSTest.

[MattKotsenas]

We should also update the playwright templates but let's decide first if we want to have this feature or not.

Fixed anyways, just so the PR is complete if accepted.

[fhnaseer]

Changing default behavior could be confusing for users.

For example, a user has an existing solution with test projects

1. They collect coverage, and coverage report contains coverage for test projects.
2. They add a new test project (after this change)
3. They collect coverage, and coverage report contains coverage for old test projects and missing for the newly created test project.

Runsettings file can be used to exclude test projects from the coverage report. https://github.com/microsoft/codecoverage/blob/main/docs/configuration.md#example
<IncludeTestAssembly>True</IncludeTestAssembly>

[KalleOlaviNiemitalo]

In coverlet.collector, the default value of IncludeTestAssembly seems to be false already.

[MattKotsenas]

Changing default behavior could be confusing for users.

The same could be said about changing the default parallelism for MSTest, which we did. The benefits outweighed the drawbacks in that case, so I'd like to at least entertain the possibility that it does here as well.

I also feel like we should give the user more credit than that. If they're looking at code coverage numbers in test projects, they're likely advanced enough to review the test project template before checking it in.

Runsettings file can be used to exclude test projects from the coverage report. https://github.com/microsoft/codecoverage/blob/main/docs/configuration.md#example <IncludeTestAssembly>True</IncludeTestAssembly>

I wasn't aware of IncludeTestAssembly, this might be an alternative, however:

1. https://github.com/microsoft/codecoverage/blob/main/docs/configuration.md claims that the default for Microsoft.Testing.Extensions.CodeCoverage is false, but that's not matching my experience (I can file a separate bug here)
2. runsettings is a VSTest-centric conept; it doesn't work for any other runner except Microsoft.Testing.Platform with the VSTest bridge (let me know if I'm wrong here)
3. It appears that Microsoft.Testing.Extensions.CodeCoverage itself only respects values under the CodeCoverage node, and IncludeTestAssembly isn't one of those, meaning this can only be configured in M.T.P if devs use the bridge forever?
4. VS only respects .runsettings files and the attribute, not the new coverage settings xml (please correct me if I'm wrong)
5. Coverlet is already doing this by default (I'd argue as a result of this missing default in the templates)

In short, there might be a better way to make this easy for users, but it's unclear to me what that is. The nice thing about the template is that devs are provided a reasonable default via the attribute, and they're free to change it without any magic. The place we find ourselves in today is that every code coverage tool has a different opinion and configuration format, and devs are often using multiple across their IDE, command line, and CI.

[fhnaseer]

1. https://github.com/microsoft/codecoverage/blob/main/docs/configuration.md claims that the default for Microsoft.Testing.Extensions.CodeCoverage is false, but that's not matching my experience (I can file a separate bug here)

Yes, please create a bug in the coverage repo if its different.

2. runsettings is a VSTest-centric conept; it doesn't work for any other runner except Microsoft.Testing.Platform with the VSTest bridge (let me know if I'm wrong here)
3. It appears that Microsoft.Testing.Extensions.CodeCoverage itself only respects values under the CodeCoverage node, and IncludeTestAssembly isn't one of those, meaning this can only be configured in M.T.P if devs use the bridge forever?
4. VS only respects .runsettings files and the attribute, not the new coverage settings xml (please correct me if I'm wrong)

Runsettings file can be passed to all Microsoft Code Coverage tooling (vstest, MTP, dotnet-coverage, Microsoft.CodeCoverage.Console.exe). You can also send coverage settings xml. Coverage settings file is a subpart of runsettings file containing only Configuration element which has IncludeTestAssembly in it.

5. Coverlet is already doing this by default (I'd argue as a result of this missing default in the templates)

I believe that exclusion should happen at the application level (coverlet, MTP.CodeCoverage, vstest). User is using coverlet, they are aware that test projects will not be part of the coverage report. Users can have runsettings file and they need to update it only once and tests projects will be excluded/included. With this change, users will always have to remove [ExcludeFromCodeCoverage] when they create a new project if they want coverage of their test projects.

[MattKotsenas]

Thanks for the additional info!

Yes, please create a bug in the coverage repo if its different.

Filed microsoft/codecoverage#156 and can continue the discussion there.

You can also send coverage settings xml. Coverage settings file is a subpart of runsettings file containing only Configuration element which has IncludeTestAssembly in it.

The learn.microsoft.com link you provided doesn't appear to document IncludeTestAssembly (in fact, I can't find it anywhere on learn.microsoft.com).

I filed microsoft/codecoverage#157 to follow up on documenting the property.

Runsettings file can be passed to all Microsoft Code Coverage tooling (vstest, MTP, dotnet-coverage, Microsoft.CodeCoverage.Console.exe). You can also send coverage settings xml. Coverage settings file is a subpart of runsettings file containing only Configuration element which has IncludeTestAssembly in it.

How does that work if, for instance, someone is using both coverage settings xml (i.e. with MTP's --coverage-settings) and Visual Studio? Is there a way for VS to use the same single definition of test settings? Or does it need a .runsettings file to have the same behavior as CI? I'd like to drop our use of .runsettings given that a fair portion of it doesn't apply to MTP and we're mostly just configuring these code coverage settings.

Thanks again for all your help!

[fhnaseer]

Runsettings file can be passed to all Microsoft Code Coverage tooling (vstest, MTP, dotnet-coverage, Microsoft.CodeCoverage.Console.exe). You can also send coverage settings xml. Coverage settings file is a subpart of runsettings file containing only Configuration element which has IncludeTestAssembly in it.

How does that work if, for instance, someone is using both coverage settings xml (i.e. with MTP's --coverage-settings) and Visual Studio? Is there a way for VS to use the same single definition of test settings? Or does it need a .runsettings file to have the same behavior as CI? I'd like to drop our use of .runsettings given that a fair portion of it doesn't apply to MTP and we're mostly just configuring these code coverage settings.

VS needs a runsettings file as the solution can contain tests for both vstest and MTP. I believe that If you pass coverage settings file to VS, then it will report an error that this is not a valid runsettings file.

[MattKotsenas]

VS needs a runsettings file as the solution can contain tests for both vstest and MTP. I believe that If you pass coverage settings file to VS, then it will report an error that this is not a valid runsettings file.

According to this section: https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-platform-extensions-vstest-bridge#datacollectors-element

data collectors aren't supported when using Microsoft.Testing.Platform. When I try a simple .runsettings file in VS I also see this in the test output window:

Runsettings datacollectors are not supported by Microsoft.Testing.Platform and will be ignored

So now I'm stuck again. It doesn't seem that there's a way to exclude test assemblies and have it work across the command line and VS when using M.T.P.

Is there some alternative I'm missing?

[MattKotsenas]

@smithkaylee, @webreidi, @fhnaseer, @jakubch1

Friendly ping here for next steps, or let me know if I'm missing something.

In short I haven't found a way to pass IncludeTestAssembly to M.T.P in both CI and VS without fully duplicating the settings files.

If we could solve this problem then I could abandon this template change as unnecessary.

[fhnaseer]

VS needs a runsettings file as the solution can contain tests for both vstest and MTP. I believe that If you pass coverage settings file to VS, then it will report an error that this is not a valid runsettings file.

According to this section: https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-platform-extensions-vstest-bridge#datacollectors-element

data collectors aren't supported when using Microsoft.Testing.Platform. When I try a simple .runsettings file in VS I also see this in the test output window:

Runsettings datacollectors are not supported by Microsoft.Testing.Platform and will be ignored

So now I'm stuck again. It doesn't seem that there's a way to exclude test assemblies and have it work across the command line and VS when using M.T.P.

You need to pass runsettings or coverage config as --coverage-settings path/to/runsettings/file. Are you using multiple data collectors in your runsettings?
In VS, do you have multiple test projects where some of them are MTP and some use vstest?

[Youssef1313]

In coverlet.collector, the default value of IncludeTestAssembly seems to be false already.

Yes, and there was even complaints about it. coverlet-coverage/coverlet#618