Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r197403323
--- Diff:
src/java/test/org/apache/zookeeper/test/HierarchicalQuorumTest.java ---
@@ -226,38 +227,108 @@ void startServers(boolean withObservers) throws
Exception {
CONNECTION_TIMEOUT));
LOG.info(hp + " is accepting client connections");
}
+ LOG.info("initial status " + s1.getCurrentVote() + "," +
s2.getCurrentVote() + "," + s3.getCurrentVote());
--- End diff --
I think this is not right approach for testing this. The patch is about
adding a new field to a JMX bean to expose a new attribute of ZooKeeper. This
test is part of `ClientHammerTest`: completely different things. For example,
if the new functionality got broken and JMX value is not exposed correctly,
we'll see that `ClientHammerTest` is failing which is terribly misleading.
Please revert changes to this file and add more unit tests to
`RemotePeerBeenTest`, because that is essentially what you've changed in this
patch and you want to validate. I'd do something like this in a new test:
```java
...
QuorumPeer peerMock = mock(QuorumPeer.class);
RemotePeerBean remotePeerBean = new RemotePeerBean(peerMock, new
QuorumServer(5, ...));
when(peerMock.isLeader(peerId)).return(true);
assertThat("Remote peer bean should expose isLeader() property of remote
peer", remotePeerBean.isLeader(5), isTrue());
...
```
Haven't tested the code, sorry if it's not working out of the box, but
hopefully give you an idea.
---
