[WIP] Implement a VSTest data collector to write coverage data to disk

[sharwell]

Fixes #210

Still need to register the data collector using something like this.

[codecov]

Codecov Report

Merging #329 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #329   +/-   ##
=======================================
  Coverage   86.57%   86.57%           
=======================================
  Files          16       16           
  Lines        2011     2011           
=======================================
  Hits         1741     1741           
  Misses        270      270

[MarcoRossignoli]

Is this possible?Maybe this is "in process" code...binding flag etc...

[tonerdo]

I remember trying to do this as the initial implementation. I found it hard to achieve the intended end user experience due to that RunSettings file. So resorted to injecting Coverlet into the MSBuild pipeline

[ViktorHofer]

@sharwell can I somehow help you getting this done?

[MarcoRossignoli]

FYI we've added some new logging infos and seems that on my local I hit issue also with coverlet build #353

[MarcoRossignoli]

Still need to register the data collector using something like this.

@sharwell I did some test with collectors https://github.com/Microsoft/vstest-docs/blob/master/docs/extensions/datacollector.md and I saw that it works(deriving from DataCollector not InProc)) using simply command line

dotnet test ... --collect name --test-adapter-path path

question, why do we need config file?

[sharwell]

why do we need config file?

No one previously knew how to enable it otherwise :)

[yousifh]

I was testing this PR locally to see how it fixes the issue and I have a question about the implementation. With CoverletDataCollector calling UnloadModule when the test session is over, is the registration of the unload events in the Instrumenter.cs file required anymore?

In particular this block I think and maybe the code block below it as well? I'm not too familiar with this file

// Fixup the custom tracker class constructor, according to all instrumented types
if (_customTrackerRegisterUnloadEventsMethod == null)
{
    _customTrackerRegisterUnloadEventsMethod = new MethodReference(
        nameof(ModuleTrackerTemplate.RegisterUnloadEvents), module.TypeSystem.Void, _customTrackerTypeDef);
}

Instruction lastInstr = _customTrackerClassConstructorIl.Body.Instructions.Last();

if (!containsAppContext)
{
    // For "normal" cases, where the instrumented assembly is not the core library, we add a call to
    // RegisterUnloadEvents to the static constructor of the generated custom tracker. Due to static
    // initialization constraints, the core library is handled separately below.
    _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Call, _customTrackerRegisterUnloadEventsMethod));
}

I added few log statements and I can see UnloadModule being called twice, but I'm not sure if there is any consequences to that?

[kamilon]

Is there an easy way to get a PreRelease NuGet of this (preferably in coverlet.msbuild package) so I can test this on a project that has these symptoms?

[MarcoRossignoli]

@kamilon usually I clone repo, checkout branch and build

msbuild build.proj /t:CreateNuGetPackage /p:Configuration=Release

you find nuget package C:\git\coverlet\build\Release\coverlet.msbuild.2.6.0.nupkg
After that you can use a Nuget.Config(custom on the root of your project or global) to refer to local coverlet package.

[MarcoRossignoli]

@sharwell let me know if you would like some help to go on with this, if you don't have free cycles I could give it a try!

[sandermvanvliet]

Just to chime in here, we're seeing the error from #210 pop up with both 2.5.0 and 2.6.0 on Azure DevOps. I'll see if I can run a test with a version in our private NuGet feed

[jmezach]

@MarcoRossignoli It looks like using the --collect and --test-adapter-path flags isn't going to work for this collector since it is an InProc one. Looking at this code from VSTest it looks like the only way to inject an in-process data collector is through the runsettings file.

[MarcoRossignoli]

@jmezach yes I saw, as I said above I did test deriving from DataCollector that isn't in proc, I wonder if it could work like in proc one to avoid settings file and use switches, it seems a simpler/more elegant approach to me.

[jmezach]

@MarcoRossignoli I tried that today, but it looks like I don't have access to the AppDomain where the instrumented code is running, so I can't call the Unload method from there. Or at least not that I can see.

[MarcoRossignoli]

I don't have access to the AppDomain where the instrumented code is running

I think that is the reason to use InProc...or maybe some with out of proc we need a way to "call test process", something like pipes or block TestSessionEnd with some shared event if possible in a cross plat way, but it seems overkill.

[MarcoRossignoli]

@sharwell we can close this we'll implement collectors as part of #395

[MarcoRossignoli]

Implemented in #410