[GitHub] [kafka] mdedetrich commented on pull request #12524: KAFKA-14133: Replace EasyMock with Mockito in streams test
mdedetrich commented on PR #12524: URL: https://github.com/apache/kafka/pull/12524#issuecomment-1414149629 Sorry I have just been busy with other things, will look into it next week -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] mdedetrich commented on pull request #12524: KAFKA-14133: Replace EasyMock with Mockito in streams test
mdedetrich commented on PR #12524: URL: https://github.com/apache/kafka/pull/12524#issuecomment-1263294566 @cadonna > https://github.com/apache/kafka/pull/12524/files#r984332259 This one is resolved now, I think I somehow missed that, apologies > https://github.com/apache/kafka/pull/12524/files#r984332539 So this one is not clear to me, commented on it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] mdedetrich commented on pull request #12524: KAFKA-14133: Replace EasyMock with Mockito in streams test
mdedetrich commented on PR #12524: URL: https://github.com/apache/kafka/pull/12524#issuecomment-1263241592 @cadonna Apologies, I must have missed the replies to the comments, will look at them today. Note that https://github.com/apache/kafka/pull/12524#discussion_r975464527 is still waiting for a response from you. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] mdedetrich commented on pull request #12524: KAFKA-14133: Replace EasyMock with Mockito in streams test
mdedetrich commented on PR #12524: URL: https://github.com/apache/kafka/pull/12524#issuecomment-1252484624 @cadonna I have addressed the comments which I could. I just noticed that I have gone back and forth between removing and adding the same stubs/verifications in various without realizing due to me force pushing so I pushed each of the changes as an individual commit to better track changes. If you want me to squash the PR then let me know. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] mdedetrich commented on pull request #12524: KAFKA-14133: Replace EasyMock with Mockito in streams test
mdedetrich commented on PR #12524:
URL: https://github.com/apache/kafka/pull/12524#issuecomment-1252139067
@cadonna Okay so I have just pushed and rebased the PR with these changes
* As you have rightly pointed out there were a lot of things being
stubbed/tested which made no sense due to peculiarities of how `EasyMock` works
(which I am still trying to get my head around). I believe I have removed most
of the unnecessary `reset`s/stubs. There are still some `verify`'s which can be
argued are pointless (mainly in the setup of the mock), if you still want to
remove these then let me know
* `StreamTaskTest` previously with `EasyMock` used stub exceptions to verify
a method was not meant to be called, i.e.
```java
EasyMock.expectLastCall().andThrow(new AssertionError("Should not have
tried to checkpoint"));
```
This has now been replaced with using `never`, i.e. the above example
translates to `verify(stateManager, never()).checkpoint();` which
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] mdedetrich commented on pull request #12524: KAFKA-14133: Replace EasyMock with Mockito in streams test
mdedetrich commented on PR #12524: URL: https://github.com/apache/kafka/pull/12524#issuecomment-1243928456 Thanks for the comments, I am still get my head around the tests and what precisely is being tested. I forgot to mention earlier that `StreamTaskTest` needs more work (doing this now, will push changes). There are other fixes needed due to the `MockitoJUnitRunner.StrictStubs`, i.e. some tests throw exceptions and expect that exception path to never be hit and this needs to be replaced with `verify(times(0))` or something similar. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] mdedetrich commented on pull request #12524: KAFKA-14133: Replace EasyMock with Mockito in streams test
mdedetrich commented on PR #12524: URL: https://github.com/apache/kafka/pull/12524#issuecomment-1243525214 @cadonna I have just rebased the PR with the latest streams changes that got merged into. trunk. I also did some more improvements for `StreamThreadTest` with the usage of `MockitoJUnitRunner.StrictStubs`, i.e. before where we had ```java verify(taskManager) ``` we now have ```java verify(taskManager).handleCorruption(corruptedTasks); ``` Also `MockitoJUnitRunner.StrictStubs` revealed pointless stubs have been removed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] mdedetrich commented on pull request #12524: KAFKA-14133: Replace EasyMock with Mockito in streams test
mdedetrich commented on PR #12524: URL: https://github.com/apache/kafka/pull/12524#issuecomment-1219138597 PR has been rebased and pushed which fixes the problem described earlier, i.e. usage of `when(storeMetadata.offset()).thenReturn(null);` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] mdedetrich commented on pull request #12524: KAFKA-14133: Replace EasyMock with Mockito in streams test
mdedetrich commented on PR #12524: URL: https://github.com/apache/kafka/pull/12524#issuecomment-1218227510 @C0urante Thanks for finding the root cause, will definitely keep this as a mental note when doing other migrations. I'll update the PR later tonight -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
