Issue #13809: Kill mutation in XpathUtil#13944
Conversation
nrmancuso
left a comment
There was a problem hiding this comment.
We need to understand how pitest is actually mutating this code before we make changes like this. The code proposed by this PR is objectively less clear and concise than what we have now. We should not allow pitest to decrease our code quality, especially if we don’t even understand what it is doing under the hood.
src/main/java/com/puppycrawl/tools/checkstyle/utils/XpathUtil.java
Outdated
Show resolved
Hide resolved
5c3770a to
9d83a99
Compare
romani
left a comment
There was a problem hiding this comment.
Ok to merge.
@Vyom-Yadav , please share your opinion on cast problem, looks like this becomes hot topic.
|
@Kevin222004 , please rebase to resolve conflict |
9d83a99 to
8bb3ca3
Compare
src/main/java/com/puppycrawl/tools/checkstyle/utils/UnmodifiableCollectionUtil.java
Outdated
Show resolved
Hide resolved
19ce261 to
8ca3e77
Compare
|
@Kevin222004 , please address Checker |
rdiachenko
left a comment
There was a problem hiding this comment.
lgtm
@Kevin222004 please resolve checker errors
8ca3e77 to
1daf995
Compare
Vyom-Yadav
left a comment
There was a problem hiding this comment.
LGTM! Please fix CI errors.
1daf995 to
bae8482
Compare
| public static <S, T> List<T> unmodifiableList(Collection<S> items, Class<T> elementType) { | ||
| return items.stream() | ||
| .map(elementType::cast) | ||
| .collect(Collectors.toUnmodifiableList()); | ||
| } |
There was a problem hiding this comment.
Why did we make this about collections, when the issue is with the calls to Class#cast? If we want to make some hack to bypass pitest, why not just have a util method to cast the stream as an intermediate operation?
Pseudocode:
<R, T> Stream<T> castedStreamOf(Stream<R> s, Class<T> c) {
return stream.map(c::cast)
}
Then we can collect or perform subsequent stream operations elsewhere more readily.
There was a problem hiding this comment.
It has unmodified collection, that pitest will complain about, not sure why it is not doing it now.
And current implementation is easier, and we don't want to make it highly reusable, at least for now.
There was a problem hiding this comment.
Why is it a public method in a utility class if we don't want it to be reusable?
There was a problem hiding this comment.
Pitest limitation to allow easy exclude from mutation by class name .
Whole code of this class is excluded from mutation at all. And class of exist until pitest improve. We did not report issue yet, but we collecting unfortunate cases in our issue
There was a problem hiding this comment.
The changes here don't add up for me. In order to remove one pitest suppression (that should just be permanent IMO), we have added:
- 2 checker suppressions (tech debt)
- avoidCallsTo suppression in pom.xml (tech debt)
- yet another specialized utility method that won't get used anywhere else (tech debt)
- Production code (the reason why we are really here) that is much less clear than before
At this point, we are shuffling tech debt around. I won't block this, but I cannot approve either.
|
@romani let’s just merge this and move on. We have already spent too much time and resources on this PR. |
Issue #13809: Kill mutation in XpathUtil
checkstyle/config/pitest-suppressions/pitest-utils-suppressions.xml
Lines 354 to 361 in 0322ba1