Added InstrumentModulesWithoutLocalSources setting

[TFTomSun]

There are already several issues regarding unexpected empty coverage results.
#1089
#1121

We found out, that there's an implicit filter that checks whether a pdb source location can be mapped to a local file. However that logic is not optimal for some use cases, for example when the build and test run happens on a different machines or when the sources are embedded in the assembly. Since the source files are not used anywhere in the whole instrumentation logic except for that filter logic, we added an option to disable that implicit filter. The previous behavior stays the default behavior. With that setting activated, the user has full control over the filtering and can still filter using Include / Exclude to filter out 3rdParty or include only the assemblies he explicitly wants to measure.

[dnfadmin]

All CLA requirements met.

[searoz]

Hi. I've just created a similar PR: #1363
The major difference is that I allow more precise control over which modules are going to be force-instrumented

[TFTomSun]

@searoz there are already include / exclude parameters available. I'd prefer the option to completely disable the implicit filtering (pdb without local sources), because it's not really intuitive - and use the existing filter possibilities instead to exclude or include certain modules. Are the existing filter options not sufficient for your case?

[searoz]

@TFTomSun yeah, I guess you are right, the combination of IncludeFilters + InstrumentModulesWithoutLocalSources should be enough for the vast majority of cases.

BTW, I don't see any changes to CoberturaReporter in your PR. If you instrument modules without local sources this class eventually calls Debug.Assert which fails dotnet test. Therefore, you have to fix some things in CoberturaReporter. Feel free to have a look at my PR

[petli]

@TFTomSun @searoz Thanks for both your PRs adressing this longstanding issue! I also think the approach of having a flag that controls the heuristic and then rely on normal include/exclude filtering is less complex both to understand as a user and to maintain in code.

Have you seen #1164 which also discusses the topic? Does it make more sense to you to have a flag that overrides the automatic exclusion of supposed third-party assemblies, as in both your PRs, or have a flag that instead allows finetuning the behaviour of that behaviour as in that issue?

[TFTomSun]

@petli I found the default filter based on local sources confusing, We have for example build packages that link additional files into the project, which are not available if only the sources are cloned. Additionally we usually don't do test runs in dev environments. We want to avoid issues in production that are caused by dev environment expectations. Therefore turning off that local source based filter completely is what we need. We want to distinguish between internal and external assemblies upon their names, not upon the environment.

[petli]

@TFTomSun I think the current behaviour works well for straightforward projects out-of-the-box, but as soon as you start having a more complex build process there's a need for being able to turn it off.

Since there is also the issue that this behaviour easily gets thrown off by generated sources, I was thinking it could be handled with a single parameter with multiple options, but I'm coming around to that it would be clearer for the user to have the option in this PR that completely disables the automatic filtering, and addressing the generated source files by relaxing the behaviour to include assemblies as long as there is at least one local source file. Any opinion, @MarcoRossignoli?

[MarcoRossignoli]

@petli I'm ok with the idea to have the option to exclude heuristic and move the "any" to "all".

The heuristic was in place to automatically exclude for instance xunit, test adapters etc...that are close to the user dll test.
But heuristic exclusion plus custom include/exclude are an acceptable b plan.

@TFTomSun I'm oof for a pair of week I'll take a look when I'll be back from the vacation. @petli feel free to review and approve and I'll merge.

For the "any" to "all" I expect some new "ticket issue" for unwanted included dll...but if are a lot we can add parameters to the new flag keeping current default.

World go on, code generators changed the game here.

[petli]

I think the code looks good, but can you take a look at the comment #1360 (comment) about the cobertura generator and see if that needs addressing with this solution too.

It would also be golden if you can add this new flag to the documentation for the different drivers too. :)

[TFTomSun]

@petli What's the expected behavior in the reporter? Do we expect the sources files to be available there? Our plan was to execute the tests in a close-to-production environment and create the report in a dev environment (with sources) and the source link case is already handled in there. As far as I know, the reporter needs the source files, right? So maybe that debug.assert still makes sense?

[petli]

@TFTomSun I'm not sure myself, I haven't worked with or used the cobertura reporter.

@searoz Do you have any input on this?

[searoz]

@TFTomSun @petli AFAIU, cobertura reporter only uses sources to "fix" file paths in the report. IMHO, this is a pretty minor feature that could be safely ignored. The only reason I brought this up is the fact that cobertura reporter has Debug.Assert right here:

coverlet/src/coverlet.core/Reporters/CoberturaReporter.cs

Line 231 in 2bb04f2

Debug.Assert(false, "Unexpected, we should find at least one path starts with one pre-build roots list");

During my tests this Debug.Assert caused dotnet test to fail, so it should be either ignored when InstrumentModulesWithoutLocalSources option is specified or just removed entirely

[petli]

Sounds like the Assert can be removed, given that circumstances have changed here.

[TFTomSun]

Sounds like the Assert can be removed, given that circumstances have changed here.

done

[petli]

The code looks good to me. It would be useful with adding the new parameters to the documentation for the three drivers, but I can also help out with that later in a separate PR.

[searoz]

@petli Do you know when can we expect new Coverlet version with these changes included? Our CI/CD depends on them (that's why I created my own PR in the first place)

[petli]

@searoz Once this is merged it would be included in the nightly builds, so you could target those until a release is published by @MarcoRossignoli.

[MarcoRossignoli]

@searoz I've merged, you can dogfood directly from your CI since tomorrow, pls let us know if it's working as expected

https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/ConsumeNightlyBuild.md

Thanks @petli