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

Integer mutators #2968

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Integer mutators #2968

wants to merge 3 commits into from

Conversation

YVbakker
Copy link

@YVbakker YVbakker commented Jun 19, 2024

This pull request will add the following mutators:

  • Integer nullification mutator
  • Integer negation mutator

Integer nullification mutator

Changes numeric literal expressions of type integer into 0. 0 is not mutated.

Examples:

  • var x = 10var x = 0
  • var x = -10var x = 0

Integer negation mutator

Changes numeric literal expressions of type integer into the negated form. 0 cannot be negated.

Examples:

  • var x = 10var x = -10
  • var x = -10var x = 10

closes #2849

Copy link
Member

@dupdob dupdob left a comment

Choose a reason for hiding this comment

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

thanks for this PR. I let you answer inline code questions.
You need to adjust integration test results, since adding a mutator changes them

private static bool IsNumberLiteral(LiteralExpressionSyntax node)
{
var kind = node.Kind();
return kind == SyntaxKind.NumericLiteralExpression && node.Parent is EqualsValueClauseSyntax;
Copy link
Member

Choose a reason for hiding this comment

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

why limiting this mutation to assignment ? it seems that it could be uses within any expression (excluding indexed access [...] ). It should at least also support method arguments.


namespace Stryker.Core.Mutators;

public class IntegerNullificationMutator : MutatorBase<LiteralExpressionSyntax>
Copy link
Member

Choose a reason for hiding this comment

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

Why having two separate mutators ?

Copy link
Author

Choose a reason for hiding this comment

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

I figured they can possibly be conflicting, and so I thought it'd be a good idea to add them separately.
When mutating var x = 10 for instance, would this be var x = 0? var x = -10?
Maybe I'm all wrong about this and it doesn't work this way at all: if this is not an issue it could be a single mutator.

Copy link
Member

Choose a reason for hiding this comment

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

Stryker's mutation orchestration takes care of this. A mutator needs only to generate a mutated version of some syntax construct.
Using separate mutators allows the users to fine grain the mutation they want, assuming each mutator uses a different Type (not the case here). Having multiple mutators sharing the same type may be interesting for code clarity when some mutators are complex (not the case here)


namespace Stryker.Core.Mutators;

public class IntegerNegationMutator : MutatorBase<LiteralExpressionSyntax>
Copy link
Member

Choose a reason for hiding this comment

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

why having two separate mutators?

private static bool IsNumberLiteral(LiteralExpressionSyntax node)
{
var kind = node.Kind();
return kind == SyntaxKind.NumericLiteralExpression && node.Parent is EqualsValueClauseSyntax;
Copy link
Member

Choose a reason for hiding this comment

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

why limiting this mutation to assignment ? it seems that it could be uses within any expression. It should at least also support method arguments.

Copy link
Author

Choose a reason for hiding this comment

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

great idea! initially, I didn't have a constraint on the node parent at all so all occurrences would be mutated. I then constrained it to at least work for the example in the original issue, and I kinda left it open for suggestions if other places should receive this mutation as well

Copy link
Member

Choose a reason for hiding this comment

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

limitation should only be placed if one detect an actual issue. But most (if not all) risky constructs, such as constants or attributes, are already identified and dealt with.

}
}";
ShouldMutateSourceToExpected(source, expected);
const string Source = """
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason fro switching to raw strings?
I do not feel this has significant benefits here

Copy link
Author

Choose a reason for hiding this comment

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

I see what you mean, however, I think this improves readability. Also, Microsoft recommends to use them when dealing with pre-formatted text. I think it's more suitable for source code snippets as those used in this test file.

Copy link
Member

Choose a reason for hiding this comment

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

I u de it comes from a good place. But this kind of change drags a huge, invisible cost: PRs

Any PR referencing the previous version will be painful to merge.
I am not sure the benefits outweigh this drawback.
Been there, done that.
We will need to check with the core team what they feel about this.

@YVbakker
Copy link
Author

thanks for this PR. I let you answer inline code questions. You need to adjust integration test results, since adding a mutator changes them

Thanks for your feedback, I'll get back to it this week


public class IntegerNegationMutatorTests : TestBase
{
[Fact]
Copy link
Member

Choose a reason for hiding this comment

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

We have recently migrated all unit tests from xunit to mstest, sorry! Could you migrate your unit tests to mstest?

@YVbakker
Copy link
Author

YVbakker commented Aug 8, 2024

I have been exceptionally busy due to the holidays, but I did read your further feedback and I will incorporate it

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.

Mutator for numbers
3 participants