Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract common Library to enable consumption from other tools #31

Closed
nlowe opened this issue Feb 26, 2018 · 20 comments · Fixed by #135
Closed

Extract common Library to enable consumption from other tools #31

nlowe opened this issue Feb 26, 2018 · 20 comments · Fixed by #135

Comments

@nlowe
Copy link

nlowe commented Feb 26, 2018

I want to write a Cake addin for MiniCover like the one that exists for OpenCover. I cannot easily instruct users to add MiniCover as a tool as the way cake downloads tools from Nuget is not compatible with the DotnetCliTool package type. I could tell users to create a tools project as instructed in the README and pass that to my addin, but I think an easier solution is if we separate the actual instrumentation and reporting logic from the dotnet cli tool frontend. This would allow me to reference it as a dependency in my package and enable users to get simple coverage with something like:

#addin "Cake.MiniCover"

// ...

Task("Coverage")
    .IsDependentOn("build")
    .Does(() => 
{
    MiniCover(tool =>
        {
            foreach(var project in GetFiles("./test/**/*.csproj"))
            {
                tool.DotNetCoreTest(project.FullPath, new DotNetCoreTestSettings()
                {
                    // Required to keep instrumentation added by MiniCover
                    NoBuild = true,
                    Configuration = configuration
                });
            }
        },
        new MiniCoverSettings()
            .WithAssembliesMatching("./test/**/*.dll")
            .WithSourcesMatching("./src/**/*.cs")
            .GenerateReport(ReportType.CONSOLE | ReportType.XML)
    );
});

// ...

Then, generating coverage becomes as simple as

./build.sh -t Coverage

# Or on Windows:

./build.ps1 -t Coverage
@lucaslorentz
Copy link
Owner

lucaslorentz commented Feb 26, 2018

That sounds great.

Splitting assembly is a very good move for Minicover.
I'm just not sure how much I should split right now. I don't want to split again in a near future because that's usually a breaking change.

One simple approach would be:

  • Minicover.Tools.DotNet (breaking change)
  • Minicover (instrumentation and reports logic)
  • Minicover.Hits (assembly copied to instrumented assemblies folder, today it copies the Minicover assembly)

A more complete approach would also split reports into:

  • Minicover.Report.Console
  • Minicover.Report.Html
  • Minicover.Report.NCover
  • Minicover.Report.OpenCover

Then some reports can be managed on separate repositories by different maintainers.

What do you think?

@lucaslorentz
Copy link
Owner

lucaslorentz commented Feb 26, 2018

I'm looking forward to see a Cake integration after splitting assemblies.

@nlowe
Copy link
Author

nlowe commented Feb 26, 2018

Thanks! In regards to your comments:

One simple approach would be:

  • Minicover.Tools.DotNet (breaking change)
  • Minicover (instrumentation and reports logic)
  • Minicover.Hits (assembly copied to instrumented assemblies folder, today it copies the Minicover assembly)

I need to familiarize myself with the codebase more but that sounds like a sane approach to me.

A more complete approach would also split reports into:

  • Minicover.Report.Console
  • Minicover.Report.Html
  • Minicover.Report.NCover
  • Minicover.Report.OpenCover

I'm not sure what your plans are report-wise bit I would be concerned about assembly sprawl if you go this route. Sure, most end users will only be producing one or two types of reports and thus need to add one or two additional references, but it adds a lot of complexity to maintain so many assemblies and nuget packages. The other thing to consider is I'm not entirely familiar with how the DotNetCliTool references work. Can you have optional dependencies for the tool? Do you have to do discovery manually? Or would the solution be "If you want third-party reports, use the tool manually or via the cake addin"?

I know on the cake side of things they have Cake.Core that addins reference which contains the core logic and Cake.Common for built-in tools and addins. Perhaps:

  • MiniCover: dotnet cli tool
  • MiniCover.Core: Instrumentation & Coverage tracking logic
  • MiniCover.Reporting.Common: Common reports (console, html, ncover, opencover, etc., basically any report maintained by the main MiniCover developers)
  • MiniCover.Internal.Hits: assembly copied to instrumented assemblies folder (this could even be MiniCover.Core, I don't know that it needs to be a separate assembly; again, I need to familiarize myself with the code base more)

This provides a route to eventually include popular report types while still freeing developers to develop their own report types. In my specific use case, I would reference MiniCover.Core and MiniCover.Reporting.Common to use in a cake addin.

@lucaslorentz
Copy link
Owner

About reports. I think the simplest solution would be each report be a separate tool that reads Minicover output files and generates the report. We could provide a library to support those separate tools. It is indeed something like "If you want third-party reports, use the tool manually or via the cake addin".

I liked your suggestions and comparison to Cake. I will keep reports together for now.

I think MiniCover.Internal.Hits doesn't have to be a nuget package. It could be just an assembly packed inside Minicover.Core nuget package. Today we copy the whole minicover assembly, but I'm not very happy with that, because only the HitService.cs is actually used by instrumented assemblies.

I will try to create a PR with those changes. Then we can discuss further on the PR, I will add you as reviewer.

@nlowe
Copy link
Author

nlowe commented Feb 26, 2018

I see, that makes sense to me. Thanks for the speedy replies!

@nlowe
Copy link
Author

nlowe commented Feb 28, 2018

I was able to get it working using the DotNetCoreTool alias from cake:

const string SOLUTION = "./Harbormaster.sln";
const string MINICOVER = "./minicover/minicover.csproj";

// ...

Task("Restore")
    .Does(() =>
{
    DotNetCoreRestore(SOLUTION);
    DotNetCoreRestore(MINICOVER);
});

// ...

Task("Test")
    .IsDependentOn("Clean::Test")
    .IsDependentOn("Build")
    .Does(() =>
{
    DotNetCoreTool(MINICOVER, "minicover", "instrument --workdir ../ --assemblies test/**/bin/**/*.dll --exclude-assemblies test/**/bin/**/*Test*.dll --sources src/**/*.cs");
    DotNetCoreTool(MINICOVER, "minicover", "reset");

    foreach(var project in GetFiles("./test/**/*.csproj"))
    {
        DotNetCoreTest(project.FullPath, new DotNetCoreTestSettings
        {
            Configuration = configuration,
            NoRestore = true,
            NoBuild = true
        });
    }

    DotNetCoreTool(MINICOVER, "minicover", "uninstrument --workdir ../");
    DotNetCoreTool(MINICOVER, "minicover", "report --workdir ../ --threshold 90");
});

Prints some nice basic results:

========================================
Test
========================================
Instrumenting assembly "Harbormaster"
Test run for C:\Users\nlowe\Projects\Harbormaster\test\Harbormaster.Tests\bin\Release\netcoreapp2.0\Harbormaster.Tests.dll(.NETCoreApp,Version=v2.0)
Microsoft (R) Test Execution Command Line Tool Version 15.5.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
[xUnit.net 00:00:01.0291225]   Discovering: Harbormaster.Tests
[xUnit.net 00:00:01.2831358]   Discovered:  Harbormaster.Tests
[xUnit.net 00:00:01.3796672]   Starting:    Harbormaster.Tests
[xUnit.net 00:00:02.0982015]   Finished:    Harbormaster.Tests

Total tests: 63. Passed: 63. Failed: 0. Skipped: 0.
Test Run Successful.
Test execution time: 3.0903 Seconds
+-------------------------------------------------------+-------+---------------+------------+
| File                                                  | Lines | Covered Lines | Percentage |
+-------------------------------------------------------+-------+---------------+------------+
| src\Harbormaster\Extensions\EnumerableExtensions.cs   |    3  |        3      |  100.00 %  |
| src\Harbormaster\Extensions\FlurlRequestExtensions.cs |    6  |        5      |   83.33 %  |
| src\Harbormaster\HarborClient.cs                      |   24  |       22      |   91.67 %  |
| src\Harbormaster\HarborClient.Project.cs              |   68  |       64      |   94.12 %  |
| src\Harbormaster\HarborClient.Repo.cs                 |    8  |        8      |  100.00 %  |
| src\Harbormaster\HarborClient.System.cs               |    6  |        6      |  100.00 %  |
| src\Harbormaster\HarborClient.Tag.cs                  |   23  |       23      |  100.00 %  |
| src\Harbormaster\HarborClient.User.cs                 |   15  |       15      |  100.00 %  |
| src\Harbormaster\Models\CreateProjectModel.cs         |    2  |        2      |  100.00 %  |
| src\Harbormaster\Models\Project.cs                    |   31  |       31      |  100.00 %  |
| src\Harbormaster\Models\ProjectLog.cs                 |   12  |       12      |  100.00 %  |
| src\Harbormaster\Models\Repository.cs                 |   19  |       19      |  100.00 %  |
| src\Harbormaster\Models\Role.cs                       |    9  |        9      |  100.00 %  |
| src\Harbormaster\Models\SearchResults.cs              |    2  |        0      |    0.00 %  |
| src\Harbormaster\Models\ServerStats.cs                |   13  |       13      |  100.00 %  |
| src\Harbormaster\Models\SystemInfo.cs                 |   23  |       23      |  100.00 %  |
| src\Harbormaster\Models\Tag.cs                        |   19  |       19      |  100.00 %  |
| src\Harbormaster\Models\User.cs                       |   23  |       23      |  100.00 %  |
| src\Harbormaster\Util\IntToBoolConverter.cs           |    3  |        2      |   66.67 %  |
+-------------------------------------------------------+-------+---------------+------------+
| All files                                             |  309  |      299      |   96.76 %  |
+-------------------------------------------------------+-------+---------------+------------+

I'm not certain why it's showing coverage for my tests as well, and I'd still like to write a cake addin to wrap the setup for me, but it's a good first step!

@bhugot
Copy link
Contributor

bhugot commented Feb 28, 2018

While I agree the libs should be splitted, you know you can ref the nuget as a packagereference and then use the code directly.

@lucaslorentz
Copy link
Owner

That's great! Build script looks very clean!

I'm working on the assembly split. Then I will work on having release versions.

@nlowe
Copy link
Author

nlowe commented Feb 28, 2018

@bhugot Good point, I guess I got caught up trying to do it the way a lot of other cake addins are written in that they call out to the tool (which isn't restorable by cake). I'll have to look into that.

@nlowe
Copy link
Author

nlowe commented Feb 28, 2018

@bhugot Actually, now that I think about it, that won't work (at least not as a standard cake addin). Cake addins target netstandard2.0 or net46. However, the current Minicover assembly targets netcoreapp2.0. It appears you can't ref a netcoreapp assembly from a netstandard assembly:

Package MiniCover 2.0.0-ci-20180226192006 is not compatible with netstandard2.0 (.NETStandard,Version=v2.0). Package MiniCover 2.0.0-ci-20180226192006 supports: netcoreapp2.0 (.NETCoreApp,Version=v2.0) 

@bhugot
Copy link
Contributor

bhugot commented Feb 28, 2018

@nlowe oh yeah you right didn't thought about that

@day01
Copy link
Contributor

day01 commented Mar 2, 2018

@lucaslorentz @nlowe
Would not it be a good idea to extract Minicover not as AddinTool, but as a native Cake module?
In this situation, we will have the common module and native support for Cake. What do you think?

@nlowe
Copy link
Author

nlowe commented Mar 2, 2018

@day01 That's definitely something to consider, but I'm not aware of any other cake addin that works like that, and I would hate to have people who want to use MiniCover but not cake to pull in a transient reference to Cake.Core.

@day01
Copy link
Contributor

day01 commented Mar 4, 2018

@nlowe, At the early stage of Cake nuget had process wrapper.
Now it is native NuGet module, but ok :)

nlowe added a commit to nlowe/Cake.MiniCover that referenced this issue Mar 10, 2018
@nlowe
Copy link
Author

nlowe commented Mar 10, 2018

So I got around to hacking together Cake.MiniCover which wraps the DotNetCoreTool alias. I think that's as good as it's going to get until I have a Library I can reference. It's not bad, but I still don't like having to have a separate tools project. Example Usage:

#addin "Cake.MiniCover"
SetMiniCoverToolsProject("./minicover/minicover.csproj");

// ...

Task("Test")
    .Does(() =>
{
    MiniCover(tool =>
        tool.DotNetCoreTest("./test/Sample.MyLib.Tests/Sample.MyLib.Tests.csproj", new DotNetCoreTestSettings
        {
            Configuration = configuration,
            NoRestore = true,
            NoBuild = true
        }),
        new MiniCoverSettings()
            .WithAssembliesMatching("./test/**/*.dll")
            .WithSourcesMatching("./src/**/*.cs")
            .GenerateReport(ReportType.CONSOLE | ReportType.XML)
            .WithNonFatalThreshold()
    );
});

// ...

This will print coverage with the default threshold of 90%, but will not fail the build. This is useful if you have a different tool parsing the coverage and blocking PR's. The default behavior will still fail the build, or you can be explicit and call .WithFatalThreshold() on your settings.

@day01
Copy link
Contributor

day01 commented Mar 10, 2018

@nlowe Looks really good.
@lucaslorentz do you plan to create a project on github?
That would be nice to have kanban with tasks. Maybe the common board with Cake.MiniCover (@nlowe)?

@bhugot
Copy link
Contributor

bhugot commented Mar 20, 2018

@lucaslorentz I think a github project would be great to plan changes

@ffMathy
Copy link
Collaborator

ffMathy commented Dec 15, 2020

What is the status on this? As soon as this is done, I can start using (and contributing to) Minicover!

@lucaslorentz
Copy link
Owner

lucaslorentz commented Dec 16, 2020

@ffMathy
I'm going to merge a PR for that soon: #135

Can you please take a look at the readme? Let me know if it's what you expect:
https://github.com/lucaslorentz/minicover/pull/135/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R132

@ffMathy
Copy link
Collaborator

ffMathy commented Dec 16, 2020

I love it! 😍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants