Improve TestNGToAssertJRules generic type constraints
#1645
Conversation
rickie
requested review from
Stephan202 and
rickie
and removed request for
Stephan202
| .toIterable() | ||
| .containsExactlyElementsOf( | ||
| copyOf(Iterators.unmodifiableIterator(new ArrayList<>().iterator()))); | ||
| assertEquals( |
There was a problem hiding this comment.
I'd say we can omit the negative matches; it's not a pattern I see often in the tests. Up to @Stephan202
There was a problem hiding this comment.
Indeed we generally don't do this, though we do for @Matches. Ideally we do move towards such more accurate testing, but that'd be quite an undertaking.
I'm fine with keeping the next tests, but do think we should then apply the changes consistently; will push a proposal.
Stephan202
force-pushed
the
share_generics_within_the_rule
branch
from
b59260a to
da8330d
Compare
There was a problem hiding this comment.
Rebased and added a commit. Thanks for this contribution @fmodesto!
Suggested commit message:
Improve `TestNGToAssertJRules` generic type constraints (#1645)
| .toIterable() | ||
| .containsExactlyElementsOf( | ||
| copyOf(Iterators.unmodifiableIterator(new ArrayList<>().iterator()))); | ||
| assertEquals( |
There was a problem hiding this comment.
Indeed we generally don't do this, though we do for @Matches. Ideally we do move towards such more accurate testing, but that'd be quite an undertaking.
I'm fine with keeping the next tests, but do think we should then apply the changes consistently; will push a proposal.
| } | ||
|
|
||
| static final class AssertEqualIteratorIterationOrder { | ||
| static final class AssertEqualIteratorIterationOrder<S, T extends S> { |
There was a problem hiding this comment.
While more correct, this means that sometimes assertion statements won't be migrated. I'll add a note to track this limitation, for future improvements.
| Iterators.unmodifiableIterator(new ArrayList<>().iterator())); | ||
| assertEquals( | ||
| Iterators.unmodifiableIterator(new ArrayList<String>().iterator()), | ||
| Iterators.unmodifiableIterator(new ArrayList<Number>().iterator())); |
There was a problem hiding this comment.
Looks like we can omit the unmodifiableIterator and synchronizedCollection wrappers. I might have introduced those in the past for consistency, but will drop them now.
| // XXX This rule fails for `java.nio.file.Path` as it is `Iterable`, but AssertJ's | ||
| // `assertThat(Path)` does not support `.containsExactlyElementsOf`. |
There was a problem hiding this comment.
This comment also applies to the rule below. Will copy it over.
|
Looks like one of the GitHub Actions builds hung; pushed an empty commit. |
There was a problem hiding this comment.
Thanks for the changes @fmodesto ! 🚀
Suggestions LGTM @Stephan202 🙏
|
Ah, I should have pressed the button to approve running of the workflows 🤦 |
|
Looks good. No mutations were possible for these changes. |
Stephan202
changed the title
TestNGToAssertJRules generic type constraints
3 participants
Summary
Refactor a few Refaster recipes by moving type variable declarations from the method level to the class level. This ensures consistency between before() and after() templates.
Motivation
OpenRewrite doesn’t support method-level generic type variables, which can result in incorrect or uncompilable replacements.