Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179022253
--- Diff:
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java
---
@@ -222,6 +223,35 @@ public void notLeader()
next.add(states.poll(timing.forSessionSleep().milliseconds(),
TimeUnit.MILLISECONDS));
next.add(states.poll(timing.forSessionSleep().milliseconds(),
TimeUnit.MILLISECONDS));
Assert.assertTrue(next.equals(Arrays.asList(ConnectionState.LOST.name(),
"false")) || next.equals(Arrays.asList("false", ConnectionState.LOST.name())),
next.toString());
+
+ latch.close();
+ client.close();
+
+ timing.sleepABit();
+ states.clear();
+ server = new TestingServer();
+ client = CuratorFrameworkFactory.builder()
+ .connectString(server.getConnectString())
+ .connectionTimeoutMs(1000)
+ .sessionTimeoutMs(timing.session())
+ .retryPolicy(new RetryOneTime(1))
+ .connectionStateErrorPolicy(new
SessionConnectionStateErrorPolicy())
+ .connectionHandlingPolicy(new
StandardConnectionHandlingPolicy(30))
+ .build();
+
client.getConnectionStateListenable().addListener(stateListener);
+ client.start();
+ latch = new LeaderLatch(client, "/test");
+ latch.addListener(listener);
+ latch.start();
+
Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(),
TimeUnit.MILLISECONDS), ConnectionState.CONNECTED.name());
+
Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(),
TimeUnit.MILLISECONDS), "true");
+ server.close();
+
Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(),
TimeUnit.MILLISECONDS), ConnectionState.SUSPENDED.name());
+ next = Lists.newArrayList();
+
+ next.add(states.poll(timing.session() / 3,
TimeUnit.MILLISECONDS));
+ next.add(states.poll(timing.forSleepingABit().milliseconds(),
TimeUnit.MILLISECONDS));
+
Assert.assertTrue(next.equals(Arrays.asList(ConnectionState.LOST.name(),
"false")) || next.equals(Arrays.asList("false", ConnectionState.LOST.name())),
next.toString());
--- End diff --
I think that the existing test case is trying to test connection / session
loss cases in the context of the LeaderLatch recipe (thus the checks for "true"
and "false"), whereas for this new test case, we don't really care about the
fact that a LeaderLatch is running, rather that the ConnectionStateManager is
doing the right thing. So, I'd prefer to see a standalone test in a
TestConnectionStateManager class.
---
