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

feat: adding dotnet metric report #3192

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phmonte
Copy link

@phmonte phmonte commented May 16, 2024

Description:

This is a draft for a metrics solution for dotnet.
The initial idea was to create a nuget package capable of interpreting a stryker-report.json file and returning the corresponding metrics.

I took advantage and created a CLI with the aim of interpreting a file and returning it to the console in a structured way.

I would like your opinion on this implementation, I looked at the PR for scala and tried to do something similar.

I had questions about nomenclature and folder organization.

Just linking the initial PR

@rouke-broersma
Copy link
Member

rouke-broersma commented May 25, 2024

I think this implementation goes beyond the scope of what we want to provide with mutation testing elements, by providing a CLI that reads a json file and prints the score to the console.

I assume that this is the specific requirement you have for a package like this so I understand why you modeled it like this. However imo mutation testing elements should not go beyond the core of providing the programmatic schematic (model) of the report and the calculation methods that could be performed on this model. Any I/O should be provided by the consuming program.

I think a case can be made to provide json serialization like metrics-scala did, but even that might go too far.

However if I'm misinterpreting the intention here and the CLI package is only meant as an example instead of a release product then I think that's fine.

@hugo-vrijswijk @nicojs since you both have existing implementations I would ask that you weigh in on the scope of packages we want to provide.

<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>net8.0</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this package should have zero dependencies I think we can target .NET Standard 2.0 without issue and achieve the highest level of compatibility.

public double Valid { get; private set; }
public double Invalid { get; private set; }
public double TotalMutants { get; private set; }
public double MutatioScore { get; private set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public double MutatioScore { get; private set; }
public double MutationScore { get; private set; }

public double Invalid { get; private set; }
public double TotalMutants { get; private set; }
public double MutatioScore { get; private set; }
public double MutatioScoreBasedOnCoveredCode { get; private set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public double MutatioScoreBasedOnCoveredCode { get; private set; }
public double MutationScoreBasedOnCoveredCode { get; private set; }

@@ -0,0 +1,19 @@
namespace Mutation.Metrics.Core.Models
{
public struct JsonReport
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public struct JsonReport
public struct MutationTestResult

Since this is an implementation of the schema we should try to follow the schema as closely as possible:

https://github.com/stryker-mutator/mutation-testing-elements/blob/master/packages/report-schema/src/mutation-testing-report-schema.json

public IDictionary<string, SourceFile> Files { get; set; }
}

public struct SourceFile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public struct SourceFile
public struct FileResult

public ISet<JsonMutant> Mutants { get; set; }
}

public struct JsonMutant
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public struct JsonMutant
public struct MutantResult

{
public string SchemaVersion { get; set; }
public IDictionary<string, int> Thresholds { get; set; }
public IDictionary<string, SourceFile> Files { get; set; }
Copy link
Member

@rouke-broersma rouke-broersma May 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see TestFiles added as well:

https://github.com/stryker-mutator/mutation-testing-elements/blob/master/packages/report-schema/src/mutation-testing-report-schema.json#L113

As well as all other schema properties, so the model completely encompasses the schema.

Since this is not necessary for your use case I would understand if you don't have the time to add them, in that case I can add them myself. You can see this comment as a reminder to reviewers that this needs to be added before completing this PR.

Comment on lines +21 to +22
using StreamReader reader = new(fullPath);
var reporterFile = reader.ReadToEnd();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done by the CLI

using StreamReader reader = new(fullPath);
var reporterFile = reader.ReadToEnd();

JsonReport report = JsonSerializer.Deserialize<JsonReport>(reporterFile, new JsonSerializerOptions { PropertyNameCaseInsensitive = true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this belongs here, CalculateMetrics should probably receive a concrete class. We could perhaps provide a helper class for deserializing depending on the conversation on if this would be within scope for this package.

@hugo-vrijswijk
Copy link
Member

hugo-vrijswijk commented May 25, 2024

since you both have existing implementations I would ask that you weigh in on the scope of packages we want to provide.

The main reason for the metrics packages is to have a single place per language that implements the metrics interface. IMO this is at least having a model for a result and being able to (correctly) calculate metrics based on the result. And each language might add features that are useful for it. Like aggregating reports for JS, or a type-safe JSON codec for Scala.

Non-Stryker programs probably get the most use out of the first part to calculate score or other metrics from a report. I agree that reading JSON from disk or a CLI that writes to stdout is maybe too specific for a package here

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 this pull request may close these issues.

3 participants