stryker-mutator / stryker-net

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

DebuggerDisplay shouldn't be tested #2307

Closed Marvin-Brouwer closed 1 year ago

Marvin-Brouwer commented 1 year ago

Describe the bug I have the following code:

[DebuggerDisplay("{Name}: {GetDebugValue(), nq},")]
public readonly partial struct JsonProperty : IJsonProperty
{
    [DebuggerHidden, DebuggerNonUserCode, DebuggerStepThrough]
    private string GetDebugValue()
    {
        if (_children.Count == 0) return JsonCharacterConstants.NullValue;
        var value = _children[0];
        if (value is JsonValue jsonValue)
            return jsonValue.Value ?? JsonCharacterConstants.NullValue;
        return value.Name;
    }

    // Code removed for brevity ...
}

This code is not visible nor accessible for people consuming the class and is only ever referenced by the DebuggerDisplayAttribute. I don't think this should be mutation-tested.

Expected behavior I'd expect these kind of debugging enhancements to be ignored.

Additional context Perhaps it's too complicated to figure out that the DebuggerDisplayAttribute is the only thing referencing this method. So maybe use one of these [DebuggerHidden, DebuggerNonUserCode, DebuggerStepThrough]

Reference You can see the stryker report here (for now): https://dashboard.stryker-mutator.io/reports/github.com/Marvin-Brouwer/FluentSerializer/refs/heads/93-add-manual-pipeline-for-stryker#mutant/FluentSerializer.Json/DataNodes/Nodes/JsonProperty.cs

dupdob commented 1 year ago

Thanks for the suggestion. I honestly see no reason why debugging only functions should be tested as well as the rest of the code base, as such I do not think Stryker should implicitly skipped those, but this could be discussed.

I would suggest to use Stryker comments to ignore this part of the code.

[DebuggerDisplay("{Name}: {GetDebugValue(), nq},")]
// Stryker disable once all: no need to test DebuggerDisplay
public readonly partial struct JsonProperty : IJsonProperty
...

Little known feature: Stryker comments are actually scope aware, so here once means ignore the whole method and restore normal behavior for the next ones.

Regarding the other attributes, I am pretty sure we must not ignore them by default.

That being said, using configurable attributes to ignore part of the code may be an interesting feature.

Marvin-Brouwer commented 1 year ago

Hmm, I seem to be on the rare team here. But, I don't think configuring tools like ReSharper, Stryker, StyleCop, VS code styles, etc. should be done by code comments. IMO they're not part of the code and distracting. So I'd rather have a way to exclude these by config or something like that.

The reason I think this shouldn't be tested is that this is a private method that never leaves this class. It's purely esthetic and it doesn't touch any "functionality". So in that case it's out of scope.

Regarding the other attributes, I am pretty sure we must not ignore them by default.

Yeah that's probably not smart.

That being said, using configurable attributes to ignore part of the code may be an interesting feature.

Yeah maybe have an ignore list in the config json?

dupdob commented 1 year ago

That being said, ExcludeFromCodeCoverage attribute is already interpreted as an ignore command. And I think it is consistant with what you are saying.

Marvin-Brouwer commented 1 year ago

Hmm, it's still something that's "not code" being put into code if you think about it. However, I like your suggestion. Let me give it a try!

dupdob commented 1 year ago

ExcludeFromCodeCoverage is the .Net built-in marker for code which does not need extensive testing (or even any). It is no different than DebuggerDisplay or the other attributes you are using here. And this is definitely an attribute of the code, not an option for some tool.

Marvin-Brouwer commented 1 year ago

The fact that it's made by dotnet doesn't really counters why I don't like having these kinds of annotations. However, I think that's off topic here. Your suggestion worked fine, so I'm happy. If it really bothers me I could've also moved the functionality to a debug proxy and ignored that file from Stryker.

Either way, thanks!