-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
KAFKA-7342: Migrate tests in o.a.k.streams to JUnit 5 (except KafkaStreamsTest) #15942
Conversation
…Test) Migrates tests in org.apache.kafka.streams (excluding sub-packages) to JUnit 5. KafkaStreamsTest is not migrated since it needs some more work due to unnecessary stubbings exceptions.
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.
Looks great to me!
@@ -330,8 +327,6 @@ public void shouldNotAllowToAddStateStoreToSink() { | |||
|
|||
private void mockStoreBuilder() { | |||
when(storeBuilder.name()).thenReturn("store"); | |||
when(storeBuilder.logConfig()).thenReturn(Collections.emptyMap()); |
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.
I assume Mockito said these are unnecessary?
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.
Yeah, it is somehow strange. We tried to define Mockito with strict stubs but now with the migration to JUnit 5 it turns out that it was that strict after all. I haven't yet understood why we get unnecessary stubbing exceptions. 🤔
In this specific case, it does not make any difference from a stubbing point of view since the return values are the default return values. So instead of considering lenient
, I just removed the stubs.
There are some test failures of the following kind
I do not yet know whether these have been introduced by the MockitoExtensions in this PR or another PR which has already been merged |
This comes from there:
I do not think it is related to this PR, but I will have a closer look. |
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.
LGTM
…Test) (apache#15942) Reviewers: Christo Lolov <lolovc@amazon.com>, Chia-Ping Tsai <chia7712@gmail.com>
…Test) (apache#15942) Reviewers: Christo Lolov <lolovc@amazon.com>, Chia-Ping Tsai <chia7712@gmail.com>
…Test) (apache#15942) Reviewers: Christo Lolov <lolovc@amazon.com>, Chia-Ping Tsai <chia7712@gmail.com>
Migrates tests in org.apache.kafka.streams (excluding sub-packages) to JUnit 5.
KafkaStreamsTest is not migrated since it needs some more work due to unnecessary stubbings exceptions.
Committer Checklist (excluded from commit message)