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

Why is "a"+"b" mutated to "a"-"b" and compiles? #304

Closed cs-util closed 5 years ago

cs-util commented 5 years ago

How comes that the mutation report has lines like this: [Survived] Binary expression mutation on line 26: '"" + toString(x)' ==> '"" - toString(x)'

Mutating "a"+"b" to "a"-"b" is not valid C# code if I am not totally wrong? So why is this a valid mutation that even passes the execution of my tests?

richardwerkman commented 5 years ago

Could you please provide the original code as well?

Some background info: Stryker mutates all code it finds. Then compiles and rollbacks all mutations that caused compile errors. If this mutation somehow passed the rollback process it probably compiled correctly.

cs-util commented 5 years ago

Here is one of the many lines in the report that looks suspicious to me since they can't produce compiling code:

[Survived] Binary expression mutation on line 18: 'utcString.Substring(0, "UTC", false) + "GMT"' ==> 'utcString.Substring(0, "UTC", false) - "GMT"'

Here is the code with the line and I did the mutation manually to check if it really does not compile: image

cs-util commented 5 years ago

I also found other lines in the report that are very strange:

For example, I have an extension method in a class DelegateExtensions.cs:

public static bool InvokeIfNotNull(this Action a) { 
        if (a == null) { return false; } else { a.Invoke(); return true; } 
}

image

And a unit test for it:

[Fact]
public void DelegateExtensionTests() {
    Action a = null;
    Assert.False(a.InvokeIfNotNull());
    var wasCalled = false;
    a = () => { wasCalled = true; };
    Assert.True(a.InvokeIfNotNull());
    Assert.True(wasCalled);
}

But Stryker reports:

[Survived] Binary expression mutation on line 8: 'a == null' ==> 'a != null'
[Survived] Boolean mutation on line 8: 'false' ==> 'true'
[Killed] Boolean mutation on line 8: 'true' ==> 'false'

Which can't be based on the Unit test correct?

richardwerkman commented 5 years ago

@cs-util I've tried to recreate the compiling "string" - "string" issue but couldn't achieve your output.

string test = "";
if (test.Contains("UTC"))
{
    test = test.Substring(0, 1) + "GMT";
}

Results only in 3 mutations for me:

--- Program.cs [0/3 (0.00 %)]
[Survived] String mutation on line 17: '""' ==> '"Stryker was here!"'
[Survived] String mutation on line 18: '"UTC"' ==> '""'
[Survived] String mutation on line 20: '"GMT"' ==> '""'

Am I missing something?

richardwerkman commented 5 years ago

Also, I tried running your extension method with your test and this resulted in:

--- ExampleExtension.cs [3/3 (100.00 %)]
[Killed] Binary expression mutation on line 11: 'a == null' ==> 'a != null'
[Killed] Boolean mutation on line 11: 'false' ==> 'true'
[Killed] Boolean mutation on line 11: 'true' ==> 'false'

What version of Stryker are you running? Version 0.7.0 worked fine for me.

Also your .Net Core runtime could be of importance.

dotnet --version
2.1.600-preview-009426

I hope this helps!

cs-util commented 5 years ago

I am using Stryker version 0.7.0 as well and .Net version 2.2.101

I think I found the problem, my target framework was set to "netstandard2.0" and I switched now to "netcoreapp2.1" as its set in the integration tests project.

In case that's easily possible, I think letting Striker fail if a non-supported target framework is set would be a very helpful feature since currently, it does behave kind of random for unsupported target frameworks and I am pretty sure I will not be the last person opening a ticket for this in the next months ;)

cs-util commented 5 years ago

With target framework netstandard2.0 the results looked like this:

Tests progress | ██████████ | 779 / 779 | 100 % |
Killed : 290
Survived: 489
Timeout : 0

Now with target framework netcoreapp2.1 they look like this:

Tests progress | ██████████ | 779 / 779 | 100 % |
Killed : 686
Survived: 93
Timeout : 0

But there are still mutations that if I do them manually cause the tests to fail but the report says they survived:

--------------- DelegateExtensions.cs [16/21 (76.19 %)]
[Survived] Binary expression mutation on line 8: 'a == null' ==> 'a != null'
[Killed] Boolean mutation on line 8: 'false' ==> 'true'
[Killed] Boolean mutation on line 8: 'true' ==> 'false'

And there are also still mutations that produce non-compiling code, for example [Survived] Binary expression mutation on line 26: '"" + toString(x)' ==> '"" - toString(x)' Mutating this method: image

richardwerkman commented 5 years ago

Thanks for looking into this further! I was not aware that the target projects "target framework" had this impact. I assumed Stryker would be backwards compatible since .net also is.

Your suggestion to kill Stryker when a not supported target framework is found would not have my preference. I think Stryker should support netcoreapp1.1 and up. Also netstandard2.0 should be supported. We could start by creating an integration test for each target framework we want to support. All should give the same output. If this is not the case we should fix this.

richardwerkman commented 5 years ago

Hi @cs-util I have updated the integrationtest to validate the output for both netcoreapp2.1 and netstandard2.0 but they give the same output. I'm not sure what is going on for you but Stryker seems pretty stable. You could send me your project to reproduce the issue or try to fix it yourself but I can't help anymore if I can't reproduce the issue...

Good luck!

cs-util commented 5 years ago

Hey, yes I wanted to set the repository to public in the next days anyway and will share the link with you afterwards, so that you can test if its some setting of the repository or my installed .net etc

Thanks!

cs-util commented 5 years ago

@richardwerkman Here is the repository for you to try: https://github.com/cs-util-com/cscore The relevant xUnit project for you to run is https://github.com/cs-util-com/cscore/tree/master/CsCore/xUnitTests

richardwerkman commented 5 years ago

@cs-util I am looking into your project right now and noticed a few things. I got an error saying Failed to restore the project to a buildable state. Please report the issue. Stryker can not proceed further. This means some mutants resulting in a compile error were not reverted.

I looked into the mutants that were not rollbacked and saw nothing strange about them. Then I thought of #18. When I first implemented the rollback system I didn't know if it would work on large projects. On your project it seemed that just 4 compile errors were not rollbacked, even though the rollback process rollbacked all compile errors it was given. Conclusion: the roslyn compiler didn't provide all compile errors at the first compilation... To handle this we might need to rollback multiple times. So I implemented that and now Stryker runs fine on your project.

Another thing I noticed was that I got a initial test failed message, but only sometimes. Could this be your issue #303? It might also explain why you get such different results for each Stryker run? I'm not sure of that part yet. What do you think?

richardwerkman commented 5 years ago

@cs-util Did you by any chance already figure this out yourself? I now get the same output as you did: [Survived] Binary expression mutation on line 26: '"" + toString(x)' ==> '"" - toString(x)' But this is because of a wrong implementation of the fix I did. I'll create a pull request hoping to fix your issue. If you could test this that would be great!

cs-util commented 5 years ago

Thanks @richardwerkman for looking into this, I will give v0.8.2 a try, these changes are already in there right?

and yes #303 I created when I noticed that my tests randomly fail sometimes. Some of them are depending on external conditions, e.g. the REST communication tests and some test for normal distributions of random extensions I added where there is always the probabilities that these fail, so its kind of my own fault that my tests are not robust against this. I did not find a good solution for these tests yet, maybe some of them I should just disable or for the REST tests at least just mock the actual communication

richardwerkman commented 5 years ago

Yes, the fix is released with 0.8.2! I hope it will solve the unexpected output issue. And good luck with your test stability :+1: