Block Mutator should report when it returns default value

[Liam-Rougoor]

Is your feature request related to a problem? Please describe.
When the BlockMutator removes a block that contains a return statement, it implicitly adds a statement that returns default. However, this is not shown in the output report. The code that is displayed in the report would actually result in a compile error in some cases.

The reason I think this is an issue, is that if I want to try out the mutated code myself, the code wouldn't work. It would help if the reporter could show how the code actually looks like, so I won't have to try and figure out what's missing myself.

Describe the solution you'd like
I would like the report to explicitly display when a 'return default' is added.

Additional context
This:

Should look something like this:

[Liam-Rougoor]

I'll work on it 👍

[dupdob]

I think I already explained this, but this is not how this works at all: block mutator never adds return default.

[Liam-Rougoor]

Thanks for your reply @dupdob. I'm pretty new to the stryker.net codebase, so if you remember the reference to your explanation, please do share. I'd love to learn more!

I've taken another look, and I think I do understand now. The return default is not added to the empty replacement block, but it is added to the end of the method by the EndingReturnEngine, right?

Doesn't this behavior still need to be shown in the report? The current report still has the issue that it says that the mutation would compile, even though it wouldn't (in some cases).

[dupdob]

I am afraid I can't find where this discussion happened, so I just opened discussion #2296 to explain this and discuss what may comes next.

[Liam-Rougoor]

After some consideration, we are going to add the return default within the BlockMutator. The BlockMutator can know when it causes a compile error due to a missing return statement, so it should also be able to add a return statement itself. This adds transparency to the users.

[dupdob]

again, the default return is not added by the block mutator

[Liam-Rougoor]

@dupdob I know, I want to change so that the default return is added by the block mutator, so we can make it transparent to the user.

[dupdob]

ok.Note that return default will still be needed for methods/functions where the control flow does not require an ending return premutation but where one is needed afterward. The nominal example being while(true) or some variant of it.

[rouke-broersma]

@dupdob

For sure! We had a discussion today and it turns out we get a lot of questions by users internally about the block mutator specifically because when they try out the mutation manually it does not compile. Since we should fairly easily be able to determine when this is the case with the block mutator, we decided that it was probably better to just show the user that this is happening in order to create a valid mutation. For all other cases where this is less obvious we still need the fallback of course.

Do you think that is a mistake?

[dupdob]

Yes and no 😄. As said in the discussion, having the default return is just a hack that reduce the number of time we need safe mode and I would love for a better fix. I understand your point of view; interestingly, there is little concern about this from external users, which is a bit disappointing I must say.
My only concern is making sure the current default return logic more or stays in place in order to keep the objective of reducing the amount of safe mode.

I have read several articles and post about mutation testing with this question in mind:
Is it acceptable to have complex mutations, such as one spanning several lines ?

What is a 'realistic mutant' is far from being clear cut. Most of Stryker mutators generates mutant that looks like typo/stupid mistake kind of errors (such as swapping arithmetic operators or comparison). But some goes toward stuff that looks like 'error of logic', such as the block mutator or the idea for a default parameter mutator.
I think it is acceptable to have mutators that have somewhat complex logic and impact the code in more than one places.

As such, I feel the best solution would be to be able to display all helpers within the report and offer an option to disable them altogether if the users does not accept this.

That being said, I will give you one example of a block mutation that does compile (currently) thanks to other helper (than return default)

private bool Retrieve(string key, out string value)
{
  if (ExistKey(key))
  {
    return Get(key, value);
  }
  else
  {
    value = string.Empty();
    return false;
  }
}

will have this mutant

private bool Retrieve(string key, out string value)
{
  if (ExistKey(key))
  {
    // block mutator
    // returns added for clarity
    return default(bool);
  }
  else
  {
    value = string.Empty();
    return false;
  }
}

which fails when introduced manually because value is not assigned any value. But it works because the code will look like this

private bool Retrieve(string key, out string value)
{
  {
    value = default(string);
  }
  if (ExistKey(key))
  {
    // block mutator
  }
  else
  {
    value = string.Empty();
    return false;
  }
  return default(bool);
}

[rouke-broersma]

Yes we will have to keep that in mind, thanks!