[SPARK-5735] Replace uses of EasyMock with Mockito #4578
Conversation
|
Test build #27390 has started for PR 4578 at commit
|
|
I was meaning to do this at some point. Thanks Josh. LGTM! |
| recordProcessor.shutdown(checkpointerMock, ShutdownReason.ZOMBIE) | ||
| recordProcessor.shutdown(checkpointerMock, null) | ||
|
|
||
| verify(checkpointerMock, never()).checkpoint() |
There was a problem hiding this comment.
This assertion might not actually be necessary: I guess that EasyMock throws AssertionError if non-stubbed methods are called on a mock.
|
It would be helpful if reviewers could help me to confirm that I haven't changed the semantics of these tests. One major difference between EasyMock and Mockito seems to be in how they handle calls to non-stubbed methods. I think that EasyMock will throw exceptions if non-mocked methods are called, whereas Mockito will return a default value based on the method's return type (source). It looks like we could use Mockito's verifyNoMoreInteractions() to assert that only explicitly expected method calls took place. Given that our original tests were written with EasyMock-style semantics, I'm going to add |
| val actualVal = KinesisRecordProcessor.retryRandom(receiverMock.isStopped(), 2, 100) | ||
| assert(actualVal == expectedIsStopped) | ||
|
|
||
| verify(receiverMock, times(1)).isStopped() |
There was a problem hiding this comment.
nit: times(1) is redundant.
There was a problem hiding this comment.
I realized this while writing the tests. Originally I had just plain verify(receiverMock) but I noticed that all of our other Mockito tests used an explicit times(1) and decided to match that style.
|
LGTM, although I didn't pay too much attention to the test semantics. |
|
Test build #27392 has started for PR 4578 at commit
|
|
Test build #27390 has finished for PR 4578 at commit
|
|
Test PASSed. |
|
Test build #27392 has finished for PR 4578 at commit
|
|
Test PASSed. |
| import org.scalatest.mock.MockitoSugar | ||
| import org.mockito.Mockito._ | ||
| import org.mockito.Matchers._ | ||
| import org.mockito.{Matchers, ArgumentCaptor} |
There was a problem hiding this comment.
can you reorganize this?
|
Ok LGTM, this is a fairly straightforward and hopefully it fixes the objenesis flaky test problem. Merging into master 1.3 |
This patch replaces all uses of EasyMock with Mockito. There are two motivations for this: 1. We should use a single mocking framework in our tests in order to keep things consistent. 2. EasyMock may be responsible for non-deterministic unit test failures due to its Objensis dependency (see SPARK-5626 for more details). Most of these changes are fairly mechanical translations of Mockito code to EasyMock, although I made a small change that strengthens the assertions in one test in KinesisReceiverSuite. Author: Josh Rosen <[email protected]> Closes #4578 from JoshRosen/SPARK-5735-remove-easymock and squashes the following commits: 0ab192b [Josh Rosen] Import sorting plus two minor changes to more closely match old semantics. 977565b [Josh Rosen] Remove EasyMock from build. fae1d8f [Josh Rosen] Remove EasyMock usage in KinesisReceiverSuite. 7cca486 [Josh Rosen] Remove EasyMock usage in MesosSchedulerBackendSuite fc5e94d [Josh Rosen] Remove EasyMock in CacheManagerSuite (cherry picked from commit 077eec2) Signed-off-by: Andrew Or <[email protected]>
|
Not that it matters, but mockito also depends on objenesis. :) |
This patch replaces all uses of EasyMock with Mockito. There are two motivations for this:
Most of these changes are fairly mechanical translations of Mockito code to EasyMock, although I made a small change that strengthens the assertions in one test in KinesisReceiverSuite.