Fix failure to cover test code

[sharwell]

Filtering of test modules is a per-project policy applied during report creation. It should not be assumed or applied as a default configuration.

Users who rely on the current behavior for performance reasons may exclude the test modules via instrumentation properties, though this might not be required since other recent performance improvements in coverlet would be adopted at the same time an upgrade caused this change to take effect.

[codemzs]

@tonerdo We need this change as this is causing incorrect reporting in dotnet/machinelearning repo. I will appreciate if you can approve this soon and release a nuget.

[sharwell]

@tonerdo Sorry, I built and ran tests within VS but forgot to run the tests again after a command line build. I'll fix the build issues and update this PR.

[MarcoRossignoli]

We need this change as this is causing incorrect reporting in dotnet/machinelearning repo

I'm corious, do you check threshold also of test module?

[codecov]

Codecov Report

Merging #318 into master will increase coverage by 0.88%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
+ Coverage    87.5%   88.38%   +0.88%     
==========================================
  Files          16       31      +15     
  Lines        2041     3066    +1025     
==========================================
+ Hits         1786     2710     +924     
- Misses        255      356     +101

[sharwell]

@MarcoRossignoli The intent was to remove all hard-coded special handling of test modules. This is the responsibility of downstream operations, not the core instrumentation. Test modules can still be excluded from instrumentation using existing Coverlet features.

[MarcoRossignoli]

The intent was to remove all hard-coded special handling of test modules.

I agree, maybe there was a real sample, only curiosity

[ViktorHofer]

Why would you instrument a test assembly?

[sharwell]

@ViktorHofer coverage of test assemblies has the same advantages as coverage of production assemblies for the purpose of guided code review (which is by far the most valuable feature of code review tooling). Having worked with a wide variety of coverage tools, developer policies, and management policies over the years, coverage of test assemblies has become a fundamental requirement of code coverage tools for me on all projects.

[ViktorHofer]

I'm not opposed to the feature but I don't like the breaking change.

[sharwell]

@ViktorHofer It's not a breaking change so much as a bug fix. The current release fails to instrument the named input assembly to the instrumentation request. It does not exclude test code specifically (e.g. test helper assemblies referenced by the unit test assembly), leading to reports that contain incomplete information about tests.

[MarcoRossignoli]

@tonerdo code LGTM to me
BTW I understand @ViktorHofer's concern, you've more experience on breaking changes on coverlet, maybe you can evaluate better the consequences.

[tonerdo]

I don't think this is a breaking change. All this will do is include the test assembly coverage in the final coverage output. Basically like when I added support for embedded PDBs and xunit assemblies started showing up in the results.

I wonder if we should put this behind a flag (IncludeTestAssembly?) or have the wrapper programs (msbuild and global tool) exclude it by default but make it overridable with the existing exclude or include variants.

Thoughts? @MarcoRossignoli @ViktorHofer @sharwell

[sharwell]

The main problem with <IncludeTestAssembly> triggering the current exclusion behavior is it doesn't correctly handle test code in multiple assemblies. For example, if a test project has two project references: a production project and a different test project, the referenced test project will appear in the coverage results even though it's not production code. IMO the best way to exclude test assemblies if users wish to do so is the existing assembly and/or path-based filtering.

[MarcoRossignoli]

Personally I wouldn't add another flag, @sharwell sample is pertinent, this is slight change in behaviour, should be advertised(on release note), with this change coverlet will be more useful(#318 (comment)) add test assembly exclusion to scripts doesn't seem a great problem, if this will break something...well we'll fix it. I'd like to listen @ViktorHofer's final words...good opinions are never enough.

[ViktorHofer]

I agree with others that we shouldn't introduce an option for that but definitely outline it in the release notes. Giving it a second thought I agree with Sam that the current behavior is the broken one and this change is mere a fix to it.