Shutdown msbuild driver innovations?

[MarcoRossignoli]

Started from #678 (comment)

@petli comment

A reflection somewhat from the side here is that a lot of these issues around merging, thresholds etc comes from attempts to do coverage collection and analysis being as a single build step. A more unixy way of looking at it is that coverage gets collected during unit tests and written to files, and a separate tool is then run to analyse the coverage in whichever way the user needs. These tools can be combined with a script to get a single command line or build step to run.

(Incidentially, this is how I use coverlet. A script runs all tests with the vstest collector to get coverage files, and then filter and combine in three different ways with reportgenerator to get slightly different views on the results. Partly this is because the early coverlet didn't support anything else, but also because I much prefer the fine-grained control I get this way.)

A major new version av coverlet could look like this:

• The only way to collect coverage is the vstest collector (avoiding all issues with ProcessExit etc)
• The coverlet command line tool holds functionality to merge and analyse coverage (e.g. thresholds). It could also be used to run tests using the vstest collector, and also both run tests and analyse the coverage files in a single step to retain the one-stop-shop command for those who don't need any complex processing.
• The msbuild collector is removed completely.

I think this would make it easier to use coverlet, and simplifying the code a lot. There would be less need for the more complex switches or command line flag combinations, as less common use cases (like the multi-target analysis in this issue or the Linux/Windows combined coverage) could be handled by scripting the coverlet command line tool.

cc: @tonerdo

EDIT: I've added a documentation draft for new verbs here #704 please take a look!

[MarcoRossignoli]

The only way to collect coverage is the vstest collector (avoiding all issues with ProcessExit etc)

Eh...this is my "dream"...I'm working on vstest side to understand well what missing there, for instance fail on threshold, show data on console etc...that environment lack of some support, but ms team seem available to accomodate our needs.
BTW there are a lot of users that are confortable with msbuild and I'm not sure(need to verify) vstest can run also all netfx(old framework) workload with no issue.

https://nugettrends.com/packages?months=12&ids=coverlet.msbuild&ids=coverlet.collector&ids=coverlet.console

The coverlet command line tool holds functionality to merge and analyse coverage (e.g. thresholds). It could also be used to run tests using the vstest collector, and also both run tests and analyse the coverage files in a single step to retain the one-stop-shop command for those who don't need any complex processing.

This was also one of my idea...or some new "subcommand" coverlet reports --merge...--Threeshold... etc...

The msbuild collector is removed completely.

At the moment a lot of usage are on msbuild...maybe we could remove in 1/2-1 year announcing to community to move to collectors...but we need to be sure to support all needs.

[MarcoRossignoli]

We could simply stop to release msbuild(flow only important core fix related to coverage alg and increase only fix part of semver 2.8.X) and move coverlet.console to 2.0(with new commands) and keep collectors with current versioning schema.

[MarcoRossignoli]

I opened an issue on vstest repo microsoft/vstest#2299 for some info.

[PureKrome]

I would like to +1 the obsoleting of the msbuild collector. Having 1/2 <-> 1 year time window is plenty of time to have it flagged as obsolete IMO.

Reasons: makes it 'easier' to learn how to consume/use this library.

Of course, the vstest collector needs to be feature parity with msbuild collector.

Also, while we're speaking of collectors ... I'd also vote for a name change for the vstest collector because it could suggest it's tied to visual studio. Yep, this is a branding/advertising/marketing concern .. not a tech concern. I don't have a suggested name, though :P

[tonerdo]

Many thanks for driving this @MarcoRossignoli and @petli. Removing support for msbuild is gonna be a big change especially because it's still the most used flavour. I'm not against going in this direction, but I think this is something that needs to be carefully thought through. Here are some of my thoughts/concerns:

• We still need to give the collector feature parity with msbuild if we want that to be the new default
• I'm concerned with the overhead of getting new changes or improvements into the mainstream collectors package, because some of that may involve making accompanying PRs to the vstest repo which could slow things down
• For the global tool to be a good alternative to the collectors package (since it'll most likely get new improvements first), we need to really work on the out of the box user experience and make it less complicated/verbose to get started with

Thoughts?

[MarcoRossignoli]

Many thanks for driving this @MarcoRossignoli and @petli. Removing support for msbuild is gonna be a big change especially because it's still the most used flavour

I started this issue with Shutdown but it was a bit provocative, my real idea was flow only core lib updated(mainly coverage improvements) and does not invest any more effort on it(new switch parameter or complex feat fix like with misused merge https://stackoverflow.com/questions/53255065/dotnet-unit-test-with-coverlet-how-to-get-coverage-for-entire-solution-and-not).

We still need to give the collector feature parity with msbuild if we want that to be the new default

Yes this was my idea for next release, these are the main used/requested feat https://github.com/tonerdo/coverlet/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+label%3Adriver-collectors+label%3Afeature-request and yes we need to understand how to support these on vstest plat.

I'm concerned with the overhead of getting new changes or improvements into the mainstream collectors package, because some of that may involve making accompanying PRs to the vstest repo which could slow things down

Yes I agree also with this, I'm in contact with vstest team to trying to enter in that space to gain some trust and speed up things(@AbhitejJohn @vagisha-nidhi @singhsarab @nohwnd). We provide cross coverage tool to .NET cross world(collectors are growing https://nugettrends.com/packages?months=24&ids=coverlet.collector&ids=coverlet.msbuild&ids=coverlet.console) and for what I see they're very supportive, thank's guys. We can help to make coverage usage more msbuild friendly(with no runsettings file).

https://github.com/pulls?utf8=%E2%9C%93&q=is%3Apr+author%3AMarcoRossignoli+-repo%3AMarcoRossignoli%2Fmarcorossignoli.github.io+repo%3Amicrosoft%2Fvstest+sort%3Aupdated-desc+
https://github.com/issues?utf8=%E2%9C%93&q=is%3Aopen+is%3Aissue+author%3AMarcoRossignoli+archived%3Afalse+repo%3Amicrosoft%2Fvstest
dotnet/test-templates#78

For the global tool to be a good alternative to the collectors package (since it'll most likely get new improvements first), we need to really work on the out of the box user experience and make it less complicated/verbose to get started with

My main concern about .NET tool/Msbuild is that are not integrated into vstest plat workflow and I'm scared about this kind of issues https://github.com/tonerdo/coverlet/blob/master/Documentation/KnowIssues.md#1-vstest-stops-process-execution-earlydotnet-test that we cannot control.
Collectors are integrated and will be supported "forever" and has a predictable behaviour in every context(for instance it works if I use RabbitMQ but with other 2 drivers...meh depends on timing).
Also we should avoid to add features that are not well supported by tools ecosystem(like merge) directly on the workflow if we aren't sure of how it will work inside build/test context(msbuild for instance dotnet/msbuild#4898 dotnet/msbuild#4954)
So I'm in agreement with @petli that we could add/move all features not well supported yet by tools ecosystem(vstest/msbuild) under some coverlet .net tool verb(coverlet mergereports --reports c:\... --format cobertura.
For instance we should/could move all "solution related" features to .net tools because as I explained a lot of time to our users we build/run projects and not solution and we don't have a great support from ecosystem on it, we cannot sync code to wait "ends of all tests"(without complex architecture like span new process and syncs stuff or things like that) to run merge/thresholds check and likely we won't have this in future(check msbuild issue linked above).

I'm aligned with https://blog.marcgravell.com/2020/01/net-core-net-5-exodus-of-net-framework.html and support as default usage dotnet test --collect:"XPlat Code Coverage" will be also less/easy work for us.

[petli]

The biggest problem I see with vstest now is that it's difficult to keep track of the output files unless wrapping the usage in a script that sets up a clean output directory (see https://github.com/ncalc/ncalc-async/blob/master/test/NCalcAsync.Tests/run-code-coverage.ps1 e.g.)

I see a role for the tool here to run tests on a solution or one or more projects, capturing the reports in a temporary directory and moving them to the final output, with options to merge them, output a summary on the console, check thresholds etc.

[MarcoRossignoli]

I see a role for the tool here to run tests on a solution or one or more projects

You mean launch a command uses collectors(maybe some new verb) and at the end(I need to understand how the first dotnet process know when all it's end and exits, I think that are all children process) do merge/check thresholds etc...

[petli]

Yes, it could take command line flags or an existing runsettings file and then:

1. Create a temp directory for output files
2. Create a temp runsettings file that encodes the union of settings and points to the temp directory for the output files
3. Runs the specified tests with the vstest collector and the temp runsettings file
4. Processes the output files in the temp directory according to the verb or its flags

Running subprocesses should be straightforward, the main process will be able to get notifications or wait for all children to finish.

[MarcoRossignoli]

Personally I'm not convinced of "coverlet" command for run all ceremony. I don't like the idea to "hide" dotnet test command to users. That commands could change in future and we have to follow all changes every release.
I prefer add to tool features not supported by design. I'd like more granular approach and a dev pick up what he wants i.e.:

Scenario: I've only one test proj

dotnet test --collect:"XPlat Code Coverage" --settings:runsettings

Scenario: I want to run for big sln and check thresholds

dotnet test solution.sln --collect:"XPlat Code Coverage" --settings:runsettings --results-directory:C:\SolutionResults

coverlet mergereports --coverletJsonReportFiles C:\SolutionResults\**\*coverage.json --targetCoverletJsonReport C:\SolutionResults\coverage.sln.json 
Merging C:\SolutionResults\7f85887f-af4a-4a0c-a3ae-53a6d3edca67\coverage.json to C:\SolutionResults\coverage.sln.json 
Merging C:\SolutionResults\7f85887f-af4a-4a0c-a3ae-53a6d3edca68\coverage.json to C:\SolutionResults\coverage.sln.json 
Reports merged in C:\SolutionResults\coverage.sln.json 

coverlet checkthresholds --threshold 80 --thresholdtype line --reportFile  C:\SolutionResults\coverage.sln.json 
Check threshold failed!

coverlet generatereport --coverletJsonReportFile C:\SolutionResults\coverage.sln.json --targetReport C:\SolutionResults\coverage.cobertura.xml --format cobertura

// Send report to Report Generator, https://coveralls.io/ etc...

runsettings file

<?xml version="1.0" encoding="utf-8" ?>
<RunSettings>
  <DataCollectionRunSettings>
    <DataCollectors>
      <DataCollector friendlyName="XPlat code coverage">
        <Configuration>
          <Format>coverlet</Format>  <- new alias for coverlet json format coherent only inside specific test run(we could update structure in future)                 
        </Configuration>
      </DataCollector>
    </DataCollectors>
  </DataCollectionRunSettings>
</RunSettings>

Supporting old coverlet commands

coverlet run /path/to/test-assembly.dll --target "dotnet" --targetargs "test /path/to/test-project --no-build"

This approach could be first "portable" step that doesn't need to know how testing is done and we could avoid new command when vstest will support(maybe never) this behaviour.

[petli]

I mostly agree, but there is one issue I see with

Scenario: I want to run for big sln and check thresholds

The user need to make sure the results directory doesn't contain coverage files from an old run. Maybe this requirement is ok since the tool verb will primarily be used in build steps or scripts or interactively when trying to get coverage testing running in the first place, but it could cause confusion if used repeatedly by interactive users.

[MarcoRossignoli]

The user need to make sure the results directory doesn't contain coverage files from an old run. Maybe this requirement is ok since the tool verb will primarily be used in build steps or scripts or interactively when trying to get coverage testing running in the first place, but it could cause confusion if used repeatedly by interactive users.

Yes, could happen...btw the issue today is present for official dotnet test ...--results-directory:C:\SolutionResults I mean if someone wants coverage or generate stuff under that folder vstest will add random guid to not override repeated runs, but if I want interactively run and generate something I need to use some /**/(for instance for reportgenerator) and the issue is still here, right?
Peter let me know if I misunderstood your use case.

[petli]

Yes, the issue exists today, but is handled by the ability of the msbuild/coverlet.console collectors to merge coverage files for non-parallel test runs. Though a similar situation exists there, if accidentially merging a new test run into the coverage file for an old run.

The reason I suggested a coverlet test command is to hide that messiness from the user, instead of having to always clean out result dir, then run dotnet test foo.sln --collect:"XPlat Code Coverage" followed by coverlet mergereports. Interactive usage may not be the relevant target here, and in a script or build step it is of course easy to ensure you're in clean environment before running the different commands.

The reason I suggested this is that I understood the issue as that getting rid of the one-stop-shop that coverlet.msbuild mostly is would require an equivalent, preferably better, replacement. A coverlet test could do those three steps, but not necessarily checking thresholds or anything else. (I'm not committed to it personally, as I will anyway keep running the individual steps from a script.)

[MarcoRossignoli]

Clear, @tonerdo (or every other person following this discussion) what do you think?

[MarcoRossignoli]

Added draft on new verbs #704 please take a look!

[MarcoRossignoli]

Close because after discussion we decided to now remove msbuild support, but maybe change hits gather strategy using memory mapped file to avoid vstest plat issue and some other trick to allow solution report merge(setup common file with msbuild target and lock file access on merge).

#808