stryker-mutator / stryker-net

Mutation testing for .NET core and .NET framework!
https://stryker-mutator.io
Apache License 2.0
1.77k stars 184 forks source link

Support dotnet core 2.1 global tool installs for the stryker cli runner #192

Closed rouke-broersma closed 5 years ago

rouke-broersma commented 5 years ago

Currently stryker is installed by adding the following in the test project csproj:

<ItemGroup>
    <DotNetCliToolReference Include="StrykerMutator.DotNetCoreCli" Version="*" />
    <PackageReference Include="StrykerMutator.DotNetCoreCli" Version="*" />
</ItemGroup>

Since dotnet core 2.1 there exists the concept of global dotnet tools. Currently I believe our package is not completely compatible with this install model, and will fail to install (not 100% sure). I am also not certain if a stryker run will successfully start if the tool is installed globally due to file path handling.

Nevertheless, it would be nice if we could install stryker globally using dotnet install tool --global StrykerMutator.DotNetCoreCl instead of manually editing the csproj.

richardwerkman commented 5 years ago

This would be cool! I don't think we would have to do lots of changes. For now the global tool can still use its currentdirectory as running location.

nicojs commented 5 years ago

I agree that it should be possible to install globally. However, this shouldn't be the recommended way of working. Global installations of tools are a pain to maintain on build servers and projects with more than one developer.

rouke-broersma commented 5 years ago

@nicojs Unfortunately there is currently no easy way to install tools locally in a single project. It is either a simple global install using a single command, or manually edit the csproj and manually add the <DotNetCliToolReference Include="StrykerMutator.DotNetCoreCli" Version="*" /> reference.

rouke-broersma commented 5 years ago

@nicojs @richardwerkman Do we want to add this to the readme?

nicojs commented 5 years ago

I think editing a file is pretty easy. Same as editing a pom.xml file for example. If we add it to the readme, I would also add that it is not recommended.

rouke-broersma commented 5 years ago

Imo csproj files are a lot less human editing friendly than a pom file. Especially since visual studio likes to hide the fact such a file even exists and is editable and because node names are often non-obvious. This is less true on dotnet core of course. I do agree we should at least point out that using the global tool install might make it less obvious that an update is available.

On Sun, Oct 21, 2018, 20:41 Nico Jansen notifications@github.com wrote:

I think editing a file is pretty easy. Same as editing a pom.xml file for example. If we add it to the readme, I would also add that it is not recommended.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stryker-mutator/stryker-net/issues/192#issuecomment-431693210, or mute the thread https://github.com/notifications/unsubscribe-auth/ADbenIDh8ru3kTry4pDt1CeBzxbNjzMsks5unL_ogaJpZM4Xl_f3 .

nicojs commented 5 years ago

That's all a problem of the code editor you use. In principle, editing a file is editing a file. So same ball game. I think installing it globally solely for that reason is a bad idea.

Global installations are great for hello-world and testing it out. When using it in the real world on real projects, always go for local installation. Just a small warning in the readme is fine by me.

rouke-broersma commented 5 years ago

Can be closed after readme has been updated

rouke-broersma commented 5 years ago

This change was reverted in #221 because this broke the dotnet CLI extension method of installing the stryker cli. We need to find a better way to support global tools now, perhaps by publishing a separate package for it.

ptoonen commented 5 years ago

Linking this issue here too as this will solve the issue for us. Although it would mean either implementing a dual strategy, or dropping support for the DotNetCliToolReference - which is (imho) deprecated by the time that the aforementioned issue has been resolved :-)

ptoonen commented 5 years ago

@Mobrockers did you actually test this? Because playing around with local tools, I run into the issue that the targets aren't imported. Which isn't that strange seeing as you never actually install the package into the project meaning that targets files aren't imported during build either. This would suggest a two-phase approach would be necessary:

  1. dotnet tool install StrykerMutator.DotNetCoreCli
  2. dotnet add package StrykerMutator.MSBuild
ptoonen commented 5 years ago

Just did a few tests and although we can actually have StrykerCLI do that for you (add the reference) - I'm not sure whether that is a good idea ;-) I'd rather have an explicit add than an implicit one that may potentially lead to conflicts.

Packages like coverlet also have this 2-tiered approach.

rouke-broersma commented 5 years ago

I did test this but it is entirely possible I failed to properly clean up my environment and had the targets left over from another Stryker run.

richardwerkman commented 5 years ago

@ptoonen dotnet add package StrykerMutator.MSBuild could be a temporary fix but would not have my preference.

I just opened a PR that should remove the need for the .targets file. This should make the global tool possible.

rouke-broersma commented 5 years ago

Looks like local tools will be able to use the same package as global tools, which is not possible with dotnet cli extensibility tools. However, to run a local tool instead of a global tool the user will have to run dotnet tool run stryker <options> instead of how to run stryker currently which is dotnet stryker <options>. Local tools will also require a so called tools manifest file in the project root where local tools are referenced, instead of in the csproj as it works now with extensibility tools. Global tools will keep the currently used format of dotnet stryker.

The new user flow for local tools would be as follows:

dotnet new tool-manifest | Create tool manifest file dotnet tool install dotnet tool run stryker

For global tools it would be this:

dotnet tool install -g dotnet stryker OR stryker (dotnet global tools no longer need to be called prefixed with dotnet)

We may at some point want to build some functionality to warn a user that their installed version is out of date, because at this time there does not seem to be any way for the user to be notified for example in visual studio package manager.