You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2020/06/12 16:58:03 UTC
[GitHub] [samza] cameronlee314 opened a new pull request #1382: [Minor test fix] Enabled and fixed a TestKafkaCheckpointManager test
cameronlee314 opened a new pull request #1382:
URL: https://github.com/apache/samza/pull/1382
Symptom: `TestKafkaCheckpointManager.testWriteCheckpointShouldRecreateSystemProducerOnFailure` was not enabled. It also did not pass when it was enabled. It doesn't seem like this was a disabled flaky test; just seems like the author forgot to enable it.
Cause:
1. Missing the `@Test` annotation.
2. Retry timeout was set too low, so the test never had the chance to recreate the system producer.
Tests: `./gradlew :samza-kafka:test --tests TestKafkaCheckpointManager`
API/Usage changes: N/A
----------------------------------------------------------------
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
[GitHub] [samza] bkonold commented on a change in pull request #1382: [Minor test fix] Enabled and fixed a TestKafkaCheckpointManager test
Posted by GitBox <gi...@apache.org>.
bkonold commented on a change in pull request #1382:
URL: https://github.com/apache/samza/pull/1382#discussion_r439650648
##########
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:
Do you know why we need to spy the class under test here rather than mock the underlying objects used by the method that is stubbed?
----------------------------------------------------------------
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
[GitHub] [samza] cameronlee314 merged pull request #1382: [Minor test fix] Enabled and fixed a TestKafkaCheckpointManager test
Posted by GitBox <gi...@apache.org>.
cameronlee314 merged pull request #1382:
URL: https://github.com/apache/samza/pull/1382
----------------------------------------------------------------
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
[GitHub] [samza] bkonold commented on a change in pull request #1382: [Minor test fix] Enabled and fixed a TestKafkaCheckpointManager test
Posted by GitBox <gi...@apache.org>.
bkonold commented on a change in pull request #1382:
URL: https://github.com/apache/samza/pull/1382#discussion_r439659738
##########
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:
Fair enough. Better than the test being disabled.
I'd be more worried if all invocations of `KafkaCheckpointManager.getSystemProducer` were stubbed using the spy, but it looks to be specifically targeting the second invocation (the first is during construction), so the test is at least not completely masking away the contract with `SystemFactory`.
You can ignore this comment.
----------------------------------------------------------------
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
[GitHub] [samza] cameronlee314 commented on a change in pull request #1382: [Minor test fix] Enabled and fixed a TestKafkaCheckpointManager test
Posted by 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