Github user eolivelli commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/601#discussion_r216658766
  
    --- Diff: 
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
    @@ -1486,6 +1490,377 @@ public TestQPMain getTestQPMain() {
             }
         }
     
    +    /**
    +     * Verify boot works configuring a MetricsProvider
    +     */
    +    @Test
    +    public void testMetricsProviderLifecycle() throws Exception {
    --- End diff --
    
    The whole file need a good refactor as already commented with @lvfangmin 
    I did not change the overall style of the class in order not to pollute the 
patch.
    
    I will be happy to follow up with a refactor of this class.
    
    As told with @lvfangmin it is important to test the lifecycle of 
QuorumPeerMain which is actually used in production, instead of 
ZooKeeperServerMain which is never used in production sites.
    
    Alternatively I can move these new tests to a new class.
    It is not simple to mock QuorumPeerMain without creating an useless test.
    I could use PowerMock to remove all real code (TxLog, Database....) and 
speed up
    
    As soon as we will have Maven+surefire it will be easier to run single 
methods and not be forced to run the whole file (or but a lot of @Ignore) 



---

Reply via email to