jsancio commented on code in PR #18600:
URL: https://github.com/apache/kafka/pull/18600#discussion_r1987805617
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java:
##########
@@ -52,13 +53,17 @@
import java.util.concurrent.atomic.AtomicLong;
import java.util.stream.Stream;
+import static org.apache.kafka.raft.RaftClientTestContext.Builder;
+import static org.apache.kafka.raft.RaftClientTestContext.MockListener;
+import static org.apache.kafka.raft.RaftClientTestContext.RaftProtocol;
Review Comment:
Let's revert this change. It cause a lot of unnecessary line changes to this
file.
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -516,15 +521,15 @@ public void
testEndQuorumEpochRetriesWhileResigned(boolean withKip853Rpc) throws
}
@ParameterizedTest
- @ValueSource(booleans = { true, false })
- public void testResignWillCompleteFetchPurgatory(boolean withKip853Rpc)
throws Exception {
+ @EnumSource(value = RaftClientTestContext.RaftProtocol.class, names =
{"KIP_595_PROTOCOL", "KIP_853_PROTOCOL"})
Review Comment:
You imported RaftProtocol (`import static
....RaftClientTestContext.RaftProtocol`) so you should be able to reference
`RaftProtocol` directly like you do in the method parameter. E.g. `RaftProtocol
raftProtocol`.
I am also okay if you want to pull `RaftProtocol` into its own file. E.g.
`raft/src/test/java/org/apache/kafka/raft/RaftProtocol.java`
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -92,25 +97,25 @@ public void testNodeDirectoryId() {
}
@ParameterizedTest
- @ValueSource(booleans = { true, false })
- public void testInitializeSingleMemberQuorum(boolean withKip853Rpc) throws
IOException {
+ @EnumSource(value = RaftProtocol.class, names = {"KIP_595_PROTOCOL",
"KIP_853_PROTOCOL"})
Review Comment:
As @ahuang98 mentioned. Let's try to be consistent between
`KafkaRaftClientTest` and `KafkaRaftClientSnapshotTest`. Ideally we would test
all `RaftProtocol` but I understand if you want to consider that outside the
scope of this PR if it is too difficult to change and make all of these tests
pass.
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java:
##########
@@ -276,16 +281,16 @@ public void testListenerRenotified(boolean withKip853Rpc)
throws Exception {
}
@ParameterizedTest
- @ValueSource(booleans = { false, true })
- public void testLeaderImmediatelySendsSnapshotId(boolean withKip853Rpc)
throws Exception {
+ @EnumSource(value = RaftProtocol.class)
Review Comment:
I agree that we should try to be consistent. I prefer if we inject the
`RaftProtocol` directly to the test as it is the most flexible abstraction. And
to not inject a boolean that then gets converted to a `RaftProtocol`.
I am okay testing all possible values of `RaftProtocol` as long as it is not
too much work to support and the tests don't take a lot longer.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
