zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From anmolnar <...@git.apache.org>
Subject [GitHub] zookeeper pull request #622: [ZOOKEEPER-3145] Fix potential watch missing is...
Date Mon, 17 Sep 2018 12:52:41 GMT
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.


---

Mime
View raw message