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
Some minor code improvements #965
Conversation
d26acc4
to
77162b1
Compare
@@ -60,7 +60,8 @@ ext { | |||
junitPlatformEngine : [group: 'org.junit.platform', name: 'junit-platform-engine', version: '1.8.2'], | |||
hamcrest : [group: 'org.hamcrest', name: 'hamcrest-core', version: '1.3'], | |||
junit_dataprovider : [group: 'com.tngtech.java', name: 'junit-dataprovider', version: '1.11.0'], | |||
mockito : [group: 'org.mockito', name: 'mockito-core', version: '4.4.0'], | |||
mockito : [group: 'org.mockito', name: 'mockito-core', version: '4.6.1'], | |||
mockito_junit5 : [group: 'org.mockito', name: 'mockito-junit-jupiter', version: '4.6.1'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: A version ≥4.7.0
broke the build with
Execution failed for task ':archunit:compileJdk9testJava'.
java.time.DateTimeException: Invalid value for MonthOfYear (valid values 1 - 12): 0
(which sounds like a JDK bug)
@@ -116,6 +118,7 @@ | |||
import static org.mockito.Mockito.when; | |||
|
|||
@ExtendWith(MockitoExtension.class) | |||
@MockitoSettings(strictness = Strictness.WARN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we only have one single usage of MockitoExtension
, I wonder whether we shouldn't just call MockitoAnnotations.initMocks(testInstance);
directly (and drop the mockito-junit-jupiter
dependency again).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah, sounds fair 🤔 Seems overkill to have an extra dependency for that and also not really saving anything in this case 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, initMocks(..)
is meanwhile deprecated, you should now call openMocks(..)
and hold the returned AutoCloseable
to close it in the tear down method 🤪
Once we migrate the other code from JUnit 4 to JUnit 5 we'll also need this in more places, so let's just keep the dependency for now...
Signed-off-by: Manfred Hanke <Manfred.Hanke@tngtech.com>
Signed-off-by: Manfred Hanke <Manfred.Hanke@tngtech.com>
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Signed-off-by: Manfred Hanke <Manfred.Hanke@tngtech.com>
Signed-off-by: Manfred Hanke <Manfred.Hanke@tngtech.com>
Note that mockito >= 4.7.0 breaks the build with JDK 8 and 9 due to https://bugs.openjdk.org/browse/JDK-8184940. Signed-off-by: Manfred Hanke <Manfred.Hanke@tngtech.com>
The Guava methods are meanwhile deprecated and the standard Java API is almost as convenient anyway. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
The former assertion has been deprecated because its name is confusing, users were expecting it to behave like `.isSubsetOf`. Signed-off-by: Manfred Hanke <Manfred.Hanke@tngtech.com>
77162b1
to
2ef09a8
Compare
Thanks a lot 🤩 |
`ExpectedException` is meanwhile deprecated, so we should not use it anymore. This also paves part of the way of upgrading to JUnit 5 in the future. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
2ef09a8
to
86255a7
Compare
Contributes some random code improvements for minor "issues" detected by the IDE:
com.google.common.io.Files
APIassertThat(x).containsOnlyElementsOf
with.hasSameElementsAs
ExpectedException