-
Notifications
You must be signed in to change notification settings - Fork 354
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
creation of mutable collection consider as can be avoided #1300
Comments
Some internal immutable collection is good for engineers express intend, such details might not be easily testable from public methods. Avoiding reporting of survivals would be awesome, may be by options or specific mutator or .... |
This feature is implemented in #1310. In the next release it will be possible to filter these mutations out by adding the filter string "+funmodifiablecollection" |
@hcoles , please share link to some doc or please share how to use ? Or where to put "+funmodifiablecollection" ? We use maven. Thanks a lot in advance. |
@hcoles , one more fix is required |
The filter is restricted to direct returns wrapping return values in this way is wide spread idiom where it might be reasonably argued that writing tests to confirm the behaviour isn't a productive use of time. A failure to wrap a return value can be detected by static an analysis rather than a test. Filtering the calls in other places makes less sense as it means pitest would not highlight redundant code. The example code in your link public List<Token> getHiddenBefore() {
List<Token> returnList = null;
if (hiddenBefore != null) {
returnList = Collections.unmodifiableList(hiddenBefore);
}
return returnList;
} Could be more clearly expressed as public List<Token> getHiddenBefore() {
if (hiddenBefore != null) {
return Collections.unmodifiableList(hiddenBefore);
}
return hiddenBefore;
} Or public List<Token> getHiddenBefore() {
if (hiddenBefore == null) {
return null;
}
return Collections.unmodifiableList(hiddenBefore);
} To emphasize that the method may return null. |
there is another evil "single return from method" , but we refactored it to java8 style of code to reconcile. ok, what about code like (link):
|
still mutation survival:
java:
|
public void setHiddenBefore(List<Token> hiddenBefore) {
this.hiddenBefore = Collections.unmodifiableList(hiddenBefore);
} Wrapping on write is a common idiom, and I agree it would make sense to filter this. Patterns such as creating an Optional in order to avoid writing an if statement, are not so common and it makes sense for pitest to continue to highlight code not justified by a test in these constructs. If you wish to avoid mutating calls to unmodifiableList etc completely I think this would be better dealt with by a general purpose "don't mutate this" parameter similar to avoidCallTo. |
Do you already have parameter by which we config pitest to skip mutation over avoidCallTo do no mutation inside method, but still mutate call of method. |
avoidCallsTo would work, but it works at the class level, so you have to filter out calls to everything in java.util.Collections. Filtering out calls to individual methods would need a new parameter. |
yes, you are right, we already use avoidCallsTo for classes. but did not want to exclude all methods from this class, as it has bunch of other methods like sort/...... that is better to question their necessity in main code. |
this code has been mutated as
for
Modifying the code with Pitest from
return Collections.unmodifiableSet(methodCalls)
toreturn new HashSet<>(methodCalls)
eliminates the guarantee of returning an unmodifiable set, potentially leading to unintended modifications and inconsistent behavior.we have faced this issue at checkstyle/checkstyle#13127
and checkstyle/checkstyle#13126
The text was updated successfully, but these errors were encountered: