Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/622#discussion_r218055822
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -1861,6 +1862,76 @@ public void testFaultyMetricsProviderOnConfigure()
throws Exception {
Assert.assertTrue("complains about metrics provider
MetricsProviderLifeCycleException", found);
}
+ /**
+ * Test leader/leader compatibility with/without CloseSessionTxn, so
that
+ * we can gradually rollout this code and rollback if there is problem.
+ */
+ @Test
+ public void testCloseSessionTxnCompatile() throws Exception {
--- End diff --
Testing multiple things in a single unit test is not a good practice
either. Also you could add more context to the method names like
`testCloseSessionTxnCompatile_LeaderDisabled_FollowerEnabled()`.
Why not refactoring out to a separate test file then? This file is already
quite overloaded: ~2000 lines of code.
From my experiences waiting for a refactoring ticket to fix these kind of
issues doesn't lead to the resolution. Rectoring smaller chunks of code in PRs
like this is essentially making progress. Following the existing bad practice
is just increasing tech debt.
---
