-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add Math Mutations #2233
base: master
Are you sure you want to change the base?
Add Math Mutations #2233
Conversation
docs/supported-mutators.md
Outdated
## Math Methods | ||
| Original | Mutated | | ||
| ----------------------- | ----------------------- | | ||
| `Acos()` | `Acosh()` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These all look like C# math functions, I don't know if for example Java has the same methods. Perhaps it's cleanest to do a similar thing to method expressions and make a C# category for math methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add the categories as soon as a second language support this right? Because for now C# is the only category
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but this now reads as though this should be the implementation for any language that wants to add math methods, while that's not the case. There should at least be some indication that this is C#-specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -25,6 +25,7 @@ All Stryker versions support a variety of different mutators. We've aligned on a | |||
| [String Literal](#string-literal) | ✅ | ✅ | ✅ | | |||
| [Unary Operator](#unary-operator) | ✅ | ✅ | ❌ | | |||
| [Update Operator](#update-operator) | ✅ | ✅ | n/a | | |||
| [Math Methods](#math-methods) | ❌ | ✅ | ❌ | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be a new mutator IMO. These are just Method Expression
mutators, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well yes and no. We specifically check if the method is called on the Math class. If not we won't mutate. That's different from the other method mutators. We mutate those whatever they are called upon.
We could add these to the method expression mutators section. But then we should add Math.
in front of all the mutations. And in Stryker.NET we already have them in a seperate section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is something that should be able to turned off separately from the other method mutations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is something that should be able to turned off separately from the other method mutations.
I can see that. That's why I think we should allow grouping a mutant operator outside of its category and individually name each mutant operator as per #1458 (I know the PR isn't sexy, but please take a look and provide feedback. It becomes more and more critical by the day).
However, that shouldn't be a reason to separate them here. It's still a method expression being mutated.
In the end, I think:
- Each mutant operator should have a unique mutation-testing-framework-agnostic name. For example:
AcosMethodCallNegation
- Each mutant operator fits into exactly one category. For example:
Method Expression
- Each mutant operator can fit into multiple groups. For example, based on level or class. For example
@level1
or@mathMethods
We specifically check if the method is called on the
Math
class.
That is smart, but a tiny detail that shouldn't be steering the category naming scheme IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I'd rather keep it like this. Since it has been merged already in Stryker.NET and would require a breaking change to revert. If in the future other strykers get math mutations as well we can realign.
docs/supported-mutators.md
Outdated
|
||
| Original | Mutated | | ||
| -------------------------- | -------------------------- | | ||
| `Acos()` | `Acosh()` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're checking if this method comes from the Math
class, shouldn't these be Math.Acos()
-> Math.Acosh()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
No description provided.