Github user arrodrigues commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179016314
--- 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 agree with your point. I am also not confortable with the place I put
this test, but just to explain myself:
I've chosen to improve this already existing test because its name is
"testErrorPolicies" and the fix was exactly on the errorPolicies behaviour.
Moreover, the already existing test, tests the injection of LOST after 100% of
the session timeout has elapsed, I've just copy-pasted some lines, and set to
inject lost after 30% of sessionTimeout.
In my opinion, if we move the lines I added from here to another test
class, we should also move the whole already existing testErrorPolicies method.
What do you think?
---
