feat: updating junit from version 4.x to 5.x #56
Conversation
- updated version of junit - updated version of mockito - updated tests (initial changes has been made using openrewrite, but adjusted after to keep name of test functions unchanged - result codes tests has been split on a several groups due to difficulties to use @nested tests in the existing way the tests are written - marking commented out tests as @disabled - several misprints has been addressed as well Signed-Off-By: Mikhail Laptev <mikhail.laptev@gmail.com>
| } catch (RuntimeException e) { | ||
| // OK | ||
| } | ||
| @ParameterizedTest |
There was a problem hiding this comment.
I think it is something that I like the most for parametrized tests in JUnit5 together with the fact that you don't need a separate class for them, so both regular and parametrized can be part of just a single one
| @@ -519,7 +520,7 @@ public void arriveAtStep(QueueImpl queue, QueueOptions queueOptions, QueueTestSt | |||
| connection.setChannelStatus(ChannelStatus.CHANNEL_UP); | |||
|
|
|||
| // Negotiate | |||
| sendNegotiationRespose( | |||
| sendNegotiationResponse( | |||
There was a problem hiding this comment.
👍
Less typos, better code
| @@ -589,8 +590,8 @@ public void arriveAtStep(QueueImpl queue, QueueOptions queueOptions, QueueTestSt | |||
| Argument.expectNonNull(curRequest, "curRequest") | |||
| .configureStream() | |||
| .streamParameters(); | |||
| assertEquals(params.subscriptions().length, 1); | |||
| assertEquals(params.subscriptions()[0].consumers().length, 1); | |||
| assertEquals(1, params.subscriptions().length); | |||
There was a problem hiding this comment.
👍
Now is everywhere: first is expected, second is actual
| @@ -709,7 +710,7 @@ private static Uri createUri() { | |||
|
|
|||
| /** Basic test that creates BrokerSession with a test channel */ | |||
| @Test | |||
| public void breathingTest() { | |||
| void breathingTest() { | |||
There was a problem hiding this comment.
👍
No need to be public
| @@ -631,7 +632,8 @@ public void testDisconnectWhileConnectionInProgress() { | |||
| assertEquals(0, obj.linger()); | |||
| } | |||
|
|
|||
| // @Test | |||
| @Test | |||
| @Disabled | |||
| public void testExpressionValidator( | ||
| String inputExpression, Boolean expectedResult, String expectedErrorMessage) | ||
| throws java.io.IOException { | ||
| initExpressionValidatorTest(inputExpression, expectedResult, expectedErrorMessage); |
There was a problem hiding this comment.
Do we still need this?
There was a problem hiding this comment.
hm, we don't, good catch!
| // @Test | ||
| public void pushMessageTest() throws BMQException, IOException { | ||
| @Test | ||
| @Disabled |
There was a problem hiding this comment.
Put the comment above as an argument for @Disabled or at least add some summary?
There was a problem hiding this comment.
Moved the comment to the @Disabled 👍🏼
|
|
||
| import com.bloomberg.bmq.ResultCodes; | ||
|
|
||
| public class AckResultChecks { |
There was a problem hiding this comment.
Do such classes need to be in separate files?
There was a problem hiding this comment.
I've looked one more on the structure, it is not needed, at the time of applying changes it has been easier to keep them in such a structure, but, I've consolidated checks with particular test classes, the only one - generic, I kept separate since it is related to all of them.
Let me know what do you think about that!
There was a problem hiding this comment.
If it makes sense to have a separate class/file for some reason e.g. it's used by other classes then it should be fine.
If it's used by one class only, then it may be better to not use a separate file for that (unless it may be used by other classes in future)
| } | ||
|
|
||
| @Test | ||
| public void successTest() { |
There was a problem hiding this comment.
yeah, didn't notice while looking through the changes.
| } | ||
| } | ||
|
|
||
| public static class StorageFailureTest extends AckGenericTest { |
There was a problem hiding this comment.
hm, I agree, there is no particular reason to have it public here and in the other classes and functions.
Signed-Off-By: Mikhail Laptev <mikhail.laptev@gmail.com>
|
Comments have been addressed. |
| @@ -20,10 +20,9 @@ | |||
|
|
|||
| import com.bloomberg.bmq.ResultCodes; | |||
|
|
|||
| public class GenericResultChecks { | |||
| class GenericResultChecks { | |||
There was a problem hiding this comment.
So unlike others, we still need this as a separate class?
There was a problem hiding this comment.
Answered in original comment above
There was a problem hiding this comment.
yeah, it is used by all the tests in the com.bloomberg.bmq.resultcodes package.
maybe I need to look at the structure with a fresh mind one day...
Issue number of the reported bug or feature request: #55
Describe your changes
junitmockitoopenrewrite, but adjusted after to keep name of test functions unchanged@Nestedtests in the existing way the tests are written@DisabledTesting performed
I have used
mvn -U clean testwith all targeted platforms such asJDK8,JDK11andJDK17.Additional context
I didn't find how I can run integration tests locally as part of the dockerized setup, so, it would be great if you can added some instructions 🙏🏼