Migrate spring-integration-core tests from Junit 4 to Junit Jupiter
#10009
Conversation
Signed-off-by: Jiandong Ma <[email protected]>
There was a problem hiding this comment.
This is huge, @mjd507 !
Thank you very much!
Honestly, all my review is just nit-pick and we simply can accept your changes as is.
Those are just recommendations for the future.
Let me know if you are going to address them or let's move on for other more important tasks!
I wonder if you automated such a migration somehow or what 😄
| @@ -506,7 +502,8 @@ public String method2(List<String> input) { | |||
| } | |||
|
|
|||
| MultipleAnnotationTestBean bean = new MultipleAnnotationTestBean(); | |||
| new MethodInvokingMessageGroupProcessor(bean); | |||
| assertThatThrownBy(() -> new MethodInvokingMessageGroupProcessor(bean)) | |||
| .isInstanceOf(IllegalArgumentException.class); | |||
There was a problem hiding this comment.
We prefer an API like this:
assertThatIllegalArgumentException().isThrownBy();
instead.
| @@ -567,7 +565,8 @@ String lowerCase(String s) { | |||
| } | |||
|
|
|||
| NoPublicMethodTestBean bean = new NoPublicMethodTestBean(); | |||
| new MethodInvokingMessageGroupProcessor(bean); | |||
| assertThatThrownBy(() -> new MethodInvokingMessageGroupProcessor(bean)) | |||
There was a problem hiding this comment.
Similar here:
assertThatIllegalStateException()
| public void testEmptyChannelArray() { | ||
| QueueChannel[] channels = new QueueChannel[0]; | ||
| new ChannelPurger(channels); | ||
| assertThatThrownBy(() -> new ChannelPurger(channels)) | ||
| .isInstanceOf(IllegalArgumentException.class); |
There was a problem hiding this comment.
Don't miss, please, to address this class changes in favor of assertThatIllegalArgumentException().
| public void unsupportedTypeAndNoConversionService() { | ||
| MessageChannel channel = createChannel(Integer.class); | ||
| channel.send(new GenericMessage<String>("123")); | ||
| assertThatThrownBy(() -> channel.send(new GenericMessage<String>("123"))) |
There was a problem hiding this comment.
And here we prefer: assertThatExceptionOfType(MessageDeliveryException.class).isThrownBy().
| public void testSingleSelectorRejects() { | ||
| final AtomicInteger counter = new AtomicInteger(); | ||
| MessageSelector selector = new TestMessageSelector(false, counter); | ||
| MessageSelectingInterceptor interceptor = new MessageSelectingInterceptor(selector); | ||
| QueueChannel channel = new QueueChannel(); | ||
| channel.addInterceptor(interceptor); | ||
| channel.send(new GenericMessage<>("test1")); | ||
| assertThatThrownBy(() -> channel.send(new GenericMessage<>("test1"))) |
| assertThat(outputChannel.receive(0)).isNull(); | ||
| } | ||
|
|
||
| }).isInstanceOf(MessageRejectedException.class); |
There was a problem hiding this comment.
This is a bit awkward situation.
I guess it is hard to get rid of finally block, so let's see if extracting exception into a variable from that catch block, and then assert it in the end would make the test easier to digest!
| assertThat(message).isEqualTo(e.getFailedMessage()); | ||
| throw e; | ||
| } | ||
| }).isInstanceOf(MessagingException.class); |
There was a problem hiding this comment.
There is an AssertJ API to extract object properties smooth way without this composition which maybe lost.
Something like:
assertThatExceptionOfType(MessagingException.class)
.isThrownBy(() -> handler.handleMessage(message))
.extracting(MessagingException::getFailedMessage)
.isEqualTo(message);
| public void testDefaultConfigNoLocationPattern() { | ||
| new ClassPathXmlApplicationContext("ResourcePatternResolver-config-fail.xml", getClass()).close(); | ||
| assertThatThrownBy(() -> new ClassPathXmlApplicationContext("ResourcePatternResolver-config-fail.xml", getClass()).close()) |
There was a problem hiding this comment.
I think with this AssertJ API we don't need that close() any more.
Previously it was a code quality question to have Closable resource closed.
The fact that it ended up with an exception was not visible for that tool.
| assertThat(result1a).isNotNull(); | ||
| assertThat(result1a.getPayload()).isEqualTo("blocker"); | ||
| assertThat(channelB.receive(0)).isNull(); | ||
| assertThat(channelC.receive(0)).isNull(); |
There was a problem hiding this comment.
Doesn't look like this logic has to be done inside catch block.
We simply can move all of these assertions after assertThatExceptionOfTyle().
This way the code in this test would be much easier to read.
| router.setChannels(null); | ||
| assertThatThrownBy(() -> { | ||
| router.setChannels(null); | ||
| }).isInstanceOf(IllegalArgumentException.class); |
There was a problem hiding this comment.
Doesn't look like we need body for the lambda. I mean {} and ; in the end of sentence are redundant.
|
hi @artembilan I read the comments again, and found the they're really new and useful for me. So leave this PR open, I will address the pr comments this evening, probably your tomorrow morning time. Not fully automated, I utilize the open-rewrite tool(see config) but only applies its simplest recipe, which is change the @test to Jupiter package, then I change all changed files manually, really huge😉 |
Signed-off-by: Jiandong Ma <[email protected]>
...gration-core/src/test/java/org/springframework/integration/channel/DatatypeChannelTests.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Artem Bilan <[email protected]>
No description provided.