Conversation
dupdob
left a comment
dupdob
left a comment
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
Why having two separate mutators ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
why having two separate mutators?
| private static bool IsNumberLiteral(LiteralExpressionSyntax node) | ||
| { | ||
| var kind = node.Kind(); | ||
| return kind == SyntaxKind.NumericLiteralExpression && node.Parent is EqualsValueClauseSyntax; |
There was a problem hiding this comment.
why limiting this mutation to assignment ? it seems that it could be uses within any expression. It should at least also support method arguments.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 = """ |
There was a problem hiding this comment.
Any specific reason fro switching to raw strings?
I do not feel this has significant benefits here
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Thanks for your feedback, I'll get back to it this week |
|
|
||
| public class IntegerNegationMutatorTests : TestBase | ||
| { | ||
| [Fact] |
There was a problem hiding this comment.
We have recently migrated all unit tests from xunit to mstest, sorry! Could you migrate your unit tests to mstest?
|
I have been exceptionally busy due to the holidays, but I did read your further feedback and I will incorporate it |
3 participants
This pull request will add the following mutators:
Integer nullification mutator
Changes numeric literal expressions of type integer into 0. 0 is not mutated.
Examples:
var x = 10→var x = 0var x = -10→var x = 0Integer negation mutator
Changes numeric literal expressions of type integer into the negated form. 0 cannot be negated.
Examples:
var x = 10→var x = -10var x = -10→var x = 10closes #2849