Flow ILogger to InstrumentationHelper 2

[daveMueller]

closes #559

[daveMueller]

Mhh when running the tests locally on my machine they pass.

[daveMueller]

I'm on vacation for a week and will check this when I'm back. If someone wants to take over please feel free to do so.

[MarcoRossignoli]

@daveMueller thank's and have a nice holiday!

[daveMueller]

Mhh when running the tests locally on my machine they pass.

For whatever reason the LazyInitializer.EnsureInitialized initialized a new container for the two tests. The tests only failed if another test that initialized the container was run before them. When I ran the tests isolated they passed as seen in the screenshot.

[MarcoRossignoli]

@daveMueller can you rebase you branch?There were some error on that tests...now they run out of process maybe fix your issue, try to use the ensure initializer #735

[daveMueller]

Yes sorry if it wasn't clear. It is ready to review.

[MarcoRossignoli]

During review I had some doubt on msbuild tasks.
With this update we should remove also InitDefaultServices() from DependencyInjection.cs, when we do it we MUST be sure that CoverageResultTask will be hosted in the same process as InstrumentationTask and also that Log instance passed to InstrumentationTask is the same of CoverageResultTask otherwise we could get exception if the task log instance used by IInstrumentationHelper(if first Log instance is disposed by msbuild engine).

We're sure with .net tool(console) and collector(datacollector process) but I'm not sure about one or more msbuild Task. We need to understand well if when we inject task in msbuild through https://github.com/tonerdo/coverlet/blob/master/src/coverlet.msbuild.tasks/coverlet.msbuild.targets they run in same process, I think so because otherwise also restore dll https://github.com/tonerdo/coverlet/blob/master/src/coverlet.core/Helpers/InstrumentationHelper.cs#L24 could race with possible CoverageResultTask loaded in another process.

What do you think?

[daveMueller]

Yes I agree with removing InitDefaultServices().

We need to understand well if when we inject task in msbuild through https://github.com/tonerdo/coverlet/blob/master/src/coverlet.msbuild.tasks/coverlet.msbuild.targets they run in same process,

That's what I would expect but I'm also not sure. I try to do some research here.

[daveMueller]

According to the question I raised here dotnet/msbuild#5153 we shouldn't have a problem as long as the tasks are only used in the coverlet.msbuild.tasks project.
When I was thinking about this the last couple of days I also don't really see the advantage of separating this into two different tasks. E.g. the coverlet.console project is also doing both, instrumenting and computing the results in the Program.cs class.

[MarcoRossignoli]

Thanks a lot for the investigation on msbuild repo, great work Dave!

It's separated from the beginning to be usable in different stages(before buildproject/vstest and after) orchestrated by msbuild.
Btw seems ok from DI perspective, but that sentence on Log is problematic, we need to pass log out of container because we register only the one of instrument task.

[MarcoRossignoli]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).