[WIP] Enable CoreLib coverage hits

[ViktorHofer]

Enable CoreLib coverage

@ericstj I need to replace the System.Private.CoreLib.dll in the testhost folder with the one in the testhost/il folder (for netcoreapp). I know that the runtime restore is happening in external/runtime.depproj but I'm not entirely sure where I should plug in because of the additional binplacing from the runtime to the testhost directory. I will ping you offline to unblock this PR.

[stephentoub]

generating a report with ReportGenerator is now an opt-in

I'm not following. In what situation would a dev enabling code coverage then not want a report?

[ViktorHofer]

As noted in the docs a report is always generated: coverage.xml and a summary is printed onto the console. A more comprehensive html report can be generated by adding the flag.

[stephentoub]

coverage.xml isn't human readable, and the summary isn't very meaningful. I've never once asked for code coverage and not wanted the HTML report.

[stephentoub]

as it takes a considerable amount of time for System.Private.CoreLib because of fetching its files via the source link url from GitHub

Are these cached locally? At least in my experience of working with code coverage, you generally iterate and thus run lots of code coverage reports after incremental changes. During that time, CoreLib isn't changing, and thus the source associated with it shouldn't either.

[ViktorHofer]

Are these cached locally?

Opened danielpalme/ReportGenerator#196

This PR won't go into master soon as coverlet is again broken when instrumenting System.Private.CoreLib because of a PR which made it into the newly released version: coverlet-coverage/coverlet#276. The PR adds a dependency to MemoryMappedFile which isn't available in S.P.CoreLib and causes the following exception:

 Unhandled Exception: System.TypeLoadException: Could not load type 'System.IO.MemoryMappedFiles.MemoryMappedFile' from assembly 'System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e'.
     at Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_fe0c420e-d1a5-4807-81a8-76eb8e6a46a7.UnloadModule(Object , EventArgs )
     at System.AppContext.OnProcessExit()

If somebody has an idea how to change https://github.com/tonerdo/coverlet/blob/master/src/coverlet.template/ModuleTrackerTemplate.cs#L55 to not depend on MemoryMappedFile or any other type outside of S.P.CoreLib without regressing performance please let me know.

cc @sharwell

[stephentoub]

to not depend on MemoryMappedFile or any other type outside of S.P.CoreLib without regressing performance

How often is this UnloadModule method called? From the name and OnProcessExit in the call stack, I wouldn't expect performance to be an issue. In which case reflection could be used, and if it actually was an issue, reflection could be used to then create delegates which could be cached.

[petli]

@stephentoub It is only called once, the performance issue is that the method has very limited time to run before the process shuts down and that causes problems on loaded build servers. A bit of reflection shouldn't be too heavy, though.

[ViktorHofer]

Performance is an issue as OnProccessExit doesn't allow much work to be done before the application is terminated. Consumers hit that when their CI system was overloaded. Thanks for the suggestion, I will look into it.

[petli]

If the delegates can be created in the constructor any issues with the time taken to create them is moot.

[jkotas]

OnProccessExit doesn't allow much work to be done before the application is terminated.

Why? You should be able to do as much work as you would like in OnProcessExit. Nothing (in our control) should be terminating the process while it is running.

[jkotas]

MemoryMappedFile

The fastest way to dump bulk data to disk is via regular sequential file I/O.

The MemoryMapped mapped files help you if you need random access (they save the expensive seeks in the file), but they are slower than sequential I/O.

Is this random access really necessary? Can the data be dumped to disk via sequential writes instead?

[petli]

@jkotas The purpose isn't to dump bulk data to file, but to transfer hit counts from the unit test process over to the coverlet process. (On Windows a memory-only map is used, but limitations in mapping the MemoryMappedFile APIs to the POSIX ones means there has to be a file on non-Windows.) On non-loaded tests the mmap is much quicker, but that might not matter under load so a more reliable way of controlling that all data gets passed over would be preferable.

For the limit on OnProcessExit see coverlet-coverage/coverlet#210 (comment) which links to https://docs.microsoft.com/en-us/dotnet/api/system.appdomain.processexit?redirectedfrom=MSDN&view=netcore-2.2 via Stackoverflow. It talks about an unmanaged host being able to raise that limit, is that what you think of?

[jkotas]

https://docs.microsoft.com/en-us/dotnet/api/system.appdomain.processexit?redirectedfrom=MSDN&view=netcore-2.2

This documentation applies to .NET Framework. It does not apply to .NET Core. There are no limits like this in .NET Core.

[jkotas]

Fixing the docs in dotnet/dotnet-api-docs#1632

[ViktorHofer]

Thanks Jan. Do you think there's a better way to communicate the hit counts back to the coverlet process?

[petli]

This documentation applies to .NET Framework. It does not apply to .NET Core. There are no limits like this in .NET Core.

Thanks for that clarification! If anything's killing the processes it's outside the .NET Core env itself, then.

[jkotas]

Do you think there's a better way to communicate the hit counts back to the coverlet process?

I do not know enough about coverlet architecture to tell. Maybe what you are doing is the best tradeoff given the constraints, I do not know.

As I have said above, if you care about performance of bulk data transfer, the sequential transfer using regular I/O tends to be the fastest.

[karelz]

@ViktorHofer what is status of this PR? There were not updates for more than 1 month.
If it is blocked for longer time, let's close it and reopen later when we can move it forward. Thanks!

[ViktorHofer]

Since last week this isn't blocked anymore and I plan to finish this by the end of next week.

[karelz]

@ViktorHofer gentle ping? If you don't have time now, let's close it, until the time is available again :)

[ViktorHofer]

Will try to finish this this week.

[karelz]

Closing for now (3 months no progress). Please reopen it when it is ready for more attention, thanks!