[GitHub] [samza] cameronlee314 commented on a change in pull request #1382: [Minor test fix] Enabled and fixed a TestKafkaCheckpointManager test

[GitBox <gi...@apache.org>]

cameronlee314 commented on a change in pull request #1382:
URL: https://github.com/apache/samza/pull/1382#discussion_r439657646



##########
File path: samza-kafka/src/test/scala/org/apache/samza/checkpoint/kafka/TestKafkaCheckpointManager.scala
##########
@@ -82,7 +83,6 @@ class TestKafkaCheckpointManager extends KafkaServerTestHarness {
     val spec = new KafkaStreamSpec("id", checkpointTopic, checkpointSystemName, 1, 1, props)
     val checkPointManager = Mockito.spy(new KafkaCheckpointManager(spec, new MockSystemFactory, false, config, new NoOpMetricsRegistry))

Review comment:
       It's not necessary to use the spy, but this test seems to be using some "real" method implementations for some underlying objects, so I guess it is a little awkward to mix in mocking too. I'm not a fan of how this unit test class was written overall (e.g. I think it uses an embedded Kafka/Zk just to run a unit test), but I also didn't have time to refactor everything, so I went with the easiest fix.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org