search

stryker-mutator / stryker-net

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

Static code analysis #164

Closed simondel closed 5 years ago

simondel commented 5 years ago

SonarCloud has an (supposedly) easy to use integration with Azure Pipelines. This could help us identify code smells and potential bugs.

ptoonen commented 5 years ago

@simondel are you still actively working on this, otherwise I can pick it up from here?

simondel commented 5 years ago

@ptoonen go ahead! I ran into issues SonarCloud. It only parsed code from one project as being C# code.

There is a task based (not YAML) pipeline on Azure Pipelines with SonarCloud. I think it's called stryker-net OLD or something.

simondel commented 5 years ago

The current project can be found here: https://sonarcloud.io/dashboard?id=stryker-net.stryker-net

As you can see, SonarCloud only detected 115 lines of C# code. None from the Core package.

The code is there though: https://sonarcloud.io/component_measures?id=stryker-net.stryker-net&metric=lines&selected=stryker-net.stryker-net%3Asrc%2FStryker.Core%2FStryker.Core%2FTesting%2FProcessExtensions.cs

ptoonen commented 5 years ago

@simondel can you authorize me on the sonar project?

ptoonen commented 5 years ago

Okay, so I can successfully analyze the solutions 1-by-1. The problem is that SonarQube doesn't like code that's referenced by multiple projects (it does correctly mark the files as 'shared' but won't analyze them for some reason). I'm guessing the real problem is that Core and Core.UnitTest are referenced by two solutions in the repository.

Now the solution here is a bit tougher but we've got a few options:

  1. Split the repo into multiple repo's - one for core, one for CLI (kind of like the javascript version). This wouldn't be a strange option considering the fact that they're actually two separate things and you may want to version treat them as such.
  2. Combine all projects into one solution. Also a valid option because of the tight coupling between them at the moment. This is seems to be the approach sonarsource uses with the msbuild scanner.
  3. Build the csproj's rather than the solutions. This is also what happens in the newer azure-pipelines.yml file. This would however mean that we'd need to add a to all the csproj files we would want to analyze otherwise SonarQube doesn't support it.

@richardwerkman @Mobrockers @simondel what are your thoughts on this? Do you see any other viable options?

ptoonen commented 5 years ago

ps. I prefer option 2

simondel commented 5 years ago

@ptoonen I'm not sure if solution 2 would work. There is already a solution that contains all projects. It's the CLI solution.

PS I've granted you and @Mobrockers permissions on SonarCloud (github login). I couldn't rant @richardwerkman any permissions since he never signed in.

ptoonen commented 5 years ago

@simondel it works, look at the current results. Do mind that it doesn't include everything because as of right now there are 3 solutions in the repo: Stryker.CLI.sln, Stryker.Core.sln and Stryker.CLI.IntegrationTests.sln

This sounds a bit redundant to me. If we add the integration tests to the CLI solution, we should be good to go.

rouke-broersma commented 5 years ago

@ptoonen The integration test solution includes only some known-bad projects which we run stryker on as an integration test. I don't think they should be included in the sonar analysis anyway

richardwerkman commented 5 years ago

@ptoonen I'm also in for option 2. However I split the integrationtest project in a new solution intentionally to make live unit testing possible in the CLI solution. Otherwise each line that changes would trigger a 45 second testrun.

richardwerkman commented 5 years ago

@simondel I've logged into SonarCloud so I guess you can grant me permissions now 👍

ptoonen commented 5 years ago

However I split the integrationtest project in a new solution intentionally to make live unit testing possible in the CLI solution.

We could keep them separate, or exclude them from live unit testing: https://docs.microsoft.com/en-us/visualstudio/test/live-unit-testing?view=vs-2017#include-and-exclude-test-projects-and-test-methods

rouke-broersma commented 5 years ago

There is no actual used code in the integration test projects so I suggest we just keep them as it is and not include in sonar analysis

ptoonen commented 5 years ago

Okay, so I've got most of it done. Biggest issue we have now is that Sonar will only accept one .trx file. That means we either have to merge (which is tough seeing as trxmerger is not perfect), or pick which one we want to use.

For the work in progress, look here: https://github.com/ptoonen/stryker-net/blob/164-static-code-analysis/azure-pipelines.yml

@simondel @richardwerkman what do you guys think? I'm guessing coverage on the Core project is the most useful if we'd have to pick one. The reason we have multiple .trx files is because they're multiple testruns because dotnet test doesn't support testing a full solution.