Issue #13672: Kill mutation for SuppressionXpathSingleFilter#13794
Conversation
e1e0631 to
859a11b
Compare
859a11b to
a19a82c
Compare
|
@Kevin222004 , I think you can make this PR as ready for review as ask mentors to help. |
2e8a8af to
7bb481f
Compare
7bb481f to
90f771d
Compare
|
Windows failure: Stack |
|
You didn't sanitize the path before passing it on. Files is a regexp field. |
90f771d to
aac06da
Compare
aac06da to
3b05387
Compare
|
fixed. |
|
|
||
| // reset files to default value of filter | ||
| filter.setFiles(null); | ||
| filter.finishLocalSetup(); |
There was a problem hiding this comment.
I am pretty sure this goes against the normal lifecycle. We finish the setup 3 separate times. What is the mutation we are trying to kill?
There was a problem hiding this comment.
Mutation is in diff as removal, in suppression files.
There was a problem hiding this comment.
I assume this is it.
https://pitest.org/quickstart/mutators/#EXPERIMENTAL_MEMBER_VARIABLE
Please Note: This mutator is likely to create equivalent mutations if a member variable is explicitly initialized with the Java default value for the specific type of the member variable as in
This seems like another example we should report to pitest to change.
Pitest should have the below change behavior for Objects:
variable = assignment;
into
if (assignment == null) new RuntimeException();
variable = null;
Similar to what it does for other mutators like RETURN_VALS.
https://pitest.org/quickstart/mutators/#return-values-mutator-return_vals
There was a problem hiding this comment.
I interpreted pitest as:
public void setFiles(String files) {
if (files == null) {
}
else {
this.files = Pattern.compile(files);
}
}
and covered ^^^ mutation.
pitest was right, removal of such line was not covered.
There was a problem hiding this comment.
Why can't we change this code to:
if (files != null) {
this.files = Pattern.compile(files);
}
?
There was a problem hiding this comment.
We try the best to allow user to be able to mutate modules to default state of properties.
In other words, user created module, changed values of properties, changed his mind and want to change property value back to default.
it is not possible in XML in most cases where null is default value. But xml config is not whole world for us, I hope one day there will be more formats we support. If our java beans modules continue to allow setting default value, we will always be good.
So this is extreme edge case for us, I doubt much users care about this, but as a library we should not have opinion on our modules usage, and just allow to set default value back by setter.
There was a problem hiding this comment.
Do users really consider check module properties mutable? I understand that due to the way we instantiate/configure checks that it is not reasonable to assign all properties via constructor as final fields, but usage of check modules as mutable objects seems like a bad idea to me.
There was a problem hiding this comment.
Modules were mutable for 20 years.
We do not know how our users use us.
But ideally module should be initialized by explicit default values and work same as without being explicit.
3b05387 to
c55658c
Compare
|
Keep him mind that survival is here because we use null for default value of property. All modules which doing this are prone to it. By default value is null and there is special behavior for such null. But user can not set null by xml config, if user want something to be unset, he should not have property tag in config. But this creates inconsistency. All other fields can be defined in xml with explicit value that is equal to default and it works as default. But nullable properties are different. That is why I demanding in new design not null property, and be like empty string or anything that can be set in xml as value. Ideally we should refactor module and do not allow java nulls , but this is breaking compatibility and a way more complicated, out of scope of this project. If we have time, we need to update all modules to have ability to set default value explicitly iby supported configs. This problem very visible in plugins who do some UI for user to create config, dealing with such nulls is a pain and special hacks to handle differently than other properties. |
I agree on refactoring to remove nulls. I disagree that this is complicated. Why can't we just remove the Instead of doing all at once, just do one by one when we run into it with pitest.
What plugins do this hack with modules and nulls? The deepest eclipse-cs goes that I can recall is creating the checker themselves, not the nested modules. |
|
I have no time for this update now, I still have macros project as not finished. All is doable just takes a bit of planning, investigation, scope definition, ..... . We can have this battle later on, we need to finish projects and clean up our PRs list, and start new project without backlog, too much is assigned to be to unblock. |
|
Complications of to think to make breaking changes or not. Values in xdoc are values that user can set to java beans, so our API is java beans. Xml config is just convenience format, the only firmat, and unfortunately it does not have null. We already have somewhere idea to support json like config, and it will have null definition :) so we will be able to cover such mutation by json config implementation. I am in position do not break, do not be involved in new project, and simple put coverage for pitest. Imagine we need this test for jacoco, it is same. I can put extra comment on this test that it is required until we support config with null as first class element (json). Each module should get explicitly provided default values, and it should work same as no values provided.
I do not have exact links, Imagine you render some form for user, there are some fields, you already put default values in them to show user what is default, it is convenient for module creation you just take data entry fields values and set in bean. |
Issue #13672: Kill mutation for SuppressionXpathSingleFilter