Improve Test Coverage to 81% (#258)

* pending tests to be tested on linuxOS only * add UT for MasterServiceEventMetricsTests * add UT for PerformanceAnalyzerSearchListenerTests * move linuxOS check to @BeforeClass * add UT for MasterServiceMetricsTests * add UT for NodeStatsSettingHandlerTests * add UT for WhoAmI package * remove unused PA reader tests * clean metricQueue before running every test * pending tests to be tested on linuxOS only * add UT for MasterServiceEventMetricsTests * add UT for PerformanceAnalyzerSearchListenerTests * move linuxOS check to @BeforeClass * add UT for MasterServiceMetricsTests * add UT for NodeStatsSettingHandlerTests * add UT for WhoAmI package * remove unused PA reader tests * clean metricQueue before running every test * add UT for PerformanceAnalyzerActionFilterTests * add UT for PerformanceAnalyzerActionListenerTests * add UT for PerformanceAnalyzerControllerTests * add UT for EventLogFileHandlerTests * exclude test for ClusterSettingsManager.java * code review changes Co-authored-by: Yu Sun <[email protected]>
opendistro-for-elasticsearch · Jan 8, 2021 · 8c04a7e · 8c04a7e
1 parent 1620c77
commit 8c04a7e
Show file tree

Hide file tree

Showing 25 changed files with 748 additions and 757 deletions.
diff --git a/build.gradle b/build.gradle
@@ -147,6 +147,7 @@ jacocoTestReport {
                     exclude: [
                             '**/FaultDetectionMetricsCollector.class',
                             '**/MasterThrottlingMetricsCollector.class',
+                            '**/ClusterSettingsManager.class',
                     ])
         })
     }
@@ -167,7 +168,7 @@ jacocoTestCoverageVerification {
     violationRules {
         rule {
             limit {
-                minimum = 0.6212
+                minimum = 0.8
             }
         }
     }

diff --git a/...on/opendistro/elasticsearch/performanceanalyzer/collectors/MasterServiceEventMetrics.java b/...on/opendistro/elasticsearch/performanceanalyzer/collectors/MasterServiceEventMetrics.java
@@ -23,6 +23,7 @@
 import java.util.Queue;
 import java.util.concurrent.ThreadPoolExecutor;
 
+import com.google.common.annotations.VisibleForTesting;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.elasticsearch.cluster.service.MasterService;
@@ -43,7 +44,6 @@ public class MasterServiceEventMetrics extends PerformanceAnalyzerMetricsCollect
             MasterServiceEventMetrics.class).samplingInterval;
     private static final Logger LOG = LogManager.getLogger(MasterServiceEventMetrics.class);
     private static final String MASTER_NODE_NOT_UP_METRIC = "MasterNodeNotUp";
-    private long lastTaskInsertionOrder;
     private static final int KEYS_PATH_LENGTH = 3;
     private StringBuilder value;
     private static final int TPEXECUTOR_ADD_PENDING_PARAM_COUNT = 3;
@@ -53,6 +53,9 @@ public class MasterServiceEventMetrics extends PerformanceAnalyzerMetricsCollect
     private long currentThreadId;
     private Object currentWorker;
 
+    @VisibleForTesting
+    long lastTaskInsertionOrder;
+
     public MasterServiceEventMetrics() {
         super(SAMPLING_TIME_INTERVAL, "MasterServiceEventMetrics");
         masterServiceCurrentQueue = null;
@@ -137,7 +140,8 @@ public void collectMetrics(long startTime) {
         }
     }
 
-    private void generateFinishMetrics(long startTime) {
+    @VisibleForTesting
+    void generateFinishMetrics(long startTime) {
         if (lastTaskInsertionOrder != -1) {
             value.append(PerformanceAnalyzerMetrics.getCurrentTimeMetric());
             PerformanceAnalyzerMetrics.addMetricEntry(value, MasterMetricValues.FINISH_TIME.toString(),

diff --git a/...ro/elasticsearch/performanceanalyzer/collectors/NodeStatsFixedShardsMetricsCollector.java b/...ro/elasticsearch/performanceanalyzer/collectors/NodeStatsFixedShardsMetricsCollector.java
@@ -237,42 +237,6 @@ public NodeStatsMetricsFixedShardsPerCollectionStatus(ShardStats shardStats) {
             this.shardSizeInBytes = calculate(ShardStatsValue.SHARD_SIZE_IN_BYTES);
         }
 
-        @SuppressWarnings("checkstyle:parameternumber")
-        public NodeStatsMetricsFixedShardsPerCollectionStatus(long indexingThrottleTime, long refreshCount, long refreshTime,
-                                                              long flushCount, long flushTime, long mergeCount,
-                                                              long mergeTime, long mergeCurrent, long indexBufferBytes,
-                                                              long segmentCount, long segmentsMemory, long termsMemory,
-                                                              long storedFieldsMemory, long termVectorsMemory,
-                                                              long normsMemory, long pointsMemory, long docValuesMemory,
-                                                              long indexWriterMemory, long versionMapMemory,
-                                                              long bitsetMemory, long shardSizeInBytes) {
-            super();
-            this.shardStats = null;
-
-            this.indexingThrottleTime = indexingThrottleTime;
-            this.refreshCount = refreshCount;
-            this.refreshTime = refreshTime;
-            this.flushCount = flushCount;
-            this.flushTime = flushTime;
-            this.mergeCount = mergeCount;
-            this.mergeTime = mergeTime;
-            this.mergeCurrent = mergeCurrent;
-            this.indexBufferBytes = indexBufferBytes;
-            this.segmentCount = segmentCount;
-            this.segmentsMemory = segmentsMemory;
-            this.termsMemory = termsMemory;
-            this.storedFieldsMemory = storedFieldsMemory;
-            this.termVectorsMemory = termVectorsMemory;
-            this.normsMemory = normsMemory;
-            this.pointsMemory = pointsMemory;
-            this.docValuesMemory = docValuesMemory;
-            this.indexWriterMemory = indexWriterMemory;
-            this.versionMapMemory = versionMapMemory;
-            this.bitsetMemory = bitsetMemory;
-            this.shardSizeInBytes = shardSizeInBytes;
-        }
-
-
         private long calculate(ShardStatsValue nodeMetric) {
             return valueCalculators.get(nodeMetric.toString()).calculateValue(shardStats);
         }

diff --git a/...on/opendistro/elasticsearch/performanceanalyzer/config/PerformanceAnalyzerController.java b/...on/opendistro/elasticsearch/performanceanalyzer/config/PerformanceAnalyzerController.java
@@ -1,22 +1,20 @@
 package com.amazon.opendistro.elasticsearch.performanceanalyzer.config;
 
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.ESResources;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.PerformanceAnalyzerPlugin;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.collectors.ScheduledMetricCollectorsExecutor;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.config.overrides.ConfigOverridesWrapper;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.PerformanceAnalyzerMetrics;
 import java.io.File;
 import java.nio.file.Files;
 import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.List;
 import java.util.Scanner;
-
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.config.overrides.ConfigOverridesWrapper;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.ESResources;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.PerformanceAnalyzerMetrics;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.PerformanceAnalyzerPlugin;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.collectors.ScheduledMetricCollectorsExecutor;
-
 public class PerformanceAnalyzerController {
     private static final String PERFORMANCE_ANALYZER_ENABLED_CONF = "performance_analyzer_enabled.conf";
     private static final String RCA_ENABLED_CONF = "rca_enabled.conf";

diff --git a/...mazon/opendistro/elasticsearch/performanceanalyzer/http_action/whoami/WhoAmIResponse.java b/...mazon/opendistro/elasticsearch/performanceanalyzer/http_action/whoami/WhoAmIResponse.java
@@ -25,8 +25,10 @@
 public class WhoAmIResponse extends ActionResponse implements ToXContent {
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-        builder.startObject("whoami");
-        builder.endObject();
+        builder
+            .startObject()
+            .field("whoami", "whoami")
+            .endObject();
         return builder;
     }
 

diff --git a/...ndistro/elasticsearch/performanceanalyzer/listener/PerformanceAnalyzerSearchListener.java b/...ndistro/elasticsearch/performanceanalyzer/listener/PerformanceAnalyzerSearchListener.java
@@ -43,7 +43,7 @@ public PerformanceAnalyzerSearchListener(final PerformanceAnalyzerController con
 
     @Override
     public String toString() {
-        return "PerformanceAnalyzerSearchListener";
+        return PerformanceAnalyzerSearchListener.class.getSimpleName();
     }
 
 

diff --git a/...distro/elasticsearch/performanceanalyzer/action/PerformanceAnalyzerActionFilterTests.java b/...distro/elasticsearch/performanceanalyzer/action/PerformanceAnalyzerActionFilterTests.java
@@ -15,6 +15,7 @@
 
 package com.amazon.opendistro.elasticsearch.performanceanalyzer.action;
 
+import static org.junit.Assert.assertEquals;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.verify;
@@ -40,6 +41,7 @@ public class PerformanceAnalyzerActionFilterTests {
   @Mock private PerformanceAnalyzerController controller;
   @Mock private SearchRequest searchRequest;
   @Mock private BulkRequest bulkRequest;
+  @Mock private ActionRequest request;
   @Mock private ActionListener<ActionResponse> listener;
   @Mock private ActionFilterChain<ActionRequest, ActionResponse> chain;
   @Mock private Task task;
@@ -63,9 +65,19 @@ public void testApplyWithBulkRequest() {
     testApply(bulkRequest);
   }
 
+  @Test
+  public void testApplyWithOtherRequest() {
+    testApply(request);
+  }
+
   private void testApply(ActionRequest request) {
     filter.apply(task, "_action", request, listener, chain);
     verify(chain).proceed(eq(task), eq("_action"), eq(request), any());
   }
+
+  @Test
+  public void testOrder() {
+    assertEquals(Integer.MIN_VALUE, filter.order());
+  }
 }
 
diff --git a/...stro/elasticsearch/performanceanalyzer/action/PerformanceAnalyzerActionListenerTests.java b/...stro/elasticsearch/performanceanalyzer/action/PerformanceAnalyzerActionListenerTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright <2019> Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * Copyright <2020> Amazon.com, Inc. or its affiliates. All Rights Reserved.
  *
  * Licensed under the Apache License, Version 2.0 (the "License").
  * You may not use this file except in compliance with the License.
@@ -15,49 +15,106 @@
 
 package com.amazon.opendistro.elasticsearch.performanceanalyzer.action;
 
-import org.junit.Ignore;
-import org.junit.Test;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.CustomMetricsLocationTestBase;
 import com.amazon.opendistro.elasticsearch.performanceanalyzer.config.PluginSettings;
 import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.PerformanceAnalyzerMetrics;
-import static org.junit.Assert.assertEquals;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.reader_writer_shared.Event;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.util.TestUtil;
+import java.util.List;
+import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.action.ActionListener;
+import org.elasticsearch.action.ActionResponse;
+import org.elasticsearch.action.bulk.BulkResponse;
+import org.elasticsearch.action.search.SearchResponse;
+import org.elasticsearch.rest.RestStatus;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+@SuppressWarnings("unchecked")
+public class PerformanceAnalyzerActionListenerTests {
+    private static final String listenerId= "12345";
+    private long startTimeInMills = 1153721339;
+    private PerformanceAnalyzerActionListener actionListener;
+    private ActionListener originalActionListener;
+
+    @Before
+    public void init() {
+        originalActionListener = Mockito.mock(ActionListener.class);
+        actionListener = new PerformanceAnalyzerActionListener();
+    }
+
+    @Test
+    public void testGetMetricsPath() {
+        String expectedPath = PluginSettings.instance().getMetricsLocation()
+            + PerformanceAnalyzerMetrics.getTimeInterval(startTimeInMills) + "/"
+            + PerformanceAnalyzerMetrics.sThreadsPath + "/"
+            + PerformanceAnalyzerMetrics.sHttpPath + "/"
+            + "bulk/bulkId/start";
+        String actualPath = actionListener.getMetricsPath(startTimeInMills, "bulk", "bulkId", "start");
+        assertEquals(expectedPath, actualPath);
+
+        try {
+            actionListener.getMetricsPath(startTimeInMills, "bulk");
+            fail("Negative scenario test: Should have been a RuntimeException");
+        } catch (RuntimeException ex) {
+            //- expecting exception...1 values passed; 3 expected
+        }
+    }
+
+    @Test
+    public void testOnResponseWithBulkResponse() {
+        BulkResponse bulkResponse = Mockito.mock(BulkResponse.class);
+        Mockito.when(bulkResponse.status()).thenReturn(RestStatus.OK);
+        actionListener.set(RequestType.bulk, listenerId , originalActionListener);
+        testOnResponse(bulkResponse);
+    }
+
+    @Test
+    public void testOnResponseWithSearchResponse() {
+        SearchResponse searchResponse = Mockito.mock(SearchResponse.class);
+        Mockito.when(searchResponse.status()).thenReturn(RestStatus.OK);
+        actionListener.set(RequestType.search, listenerId , originalActionListener);
+        testOnResponse(searchResponse);
+    }
 
-@Ignore
-public class PerformanceAnalyzerActionListenerTests extends CustomMetricsLocationTestBase {
+    @Test
+    public void testOnFailureWithElasticsearchException() {
+        ElasticsearchException exception = Mockito.mock(ElasticsearchException.class);
+        Mockito.when(exception.status()).thenReturn(RestStatus.INTERNAL_SERVER_ERROR);
+        actionListener.set(RequestType.search, listenerId , originalActionListener);
+        actionListener.onFailure(exception);
+        String[] metricsValues = readMetricsValue();
+        assertEquals("HTTPRespCode:500", metricsValues[2]);
+        assertTrue( metricsValues[3].contains("Exception:org.elasticsearch.ElasticsearchException"));
+    }
 
     @Test
-    public void testHttpMetrics() {
-        System.setProperty("performanceanalyzer.metrics.log.enabled", "False");
-        long startTimeInMills = 1553725339;
-        PerformanceAnalyzerActionListener performanceanalyzerActionListener = new PerformanceAnalyzerActionListener();
-        performanceanalyzerActionListener.saveMetricValues("XYZADFAS", startTimeInMills, "bulk", "bulkId", "start");
-        String fetchedValue = PerformanceAnalyzerMetrics.getMetric(
-                PluginSettings.instance().getMetricsLocation()
-                        + PerformanceAnalyzerMetrics.getTimeInterval(startTimeInMills)+"/threads/http/bulk/bulkId/start");
-        assertEquals("XYZADFAS", fetchedValue);
-
-        String startMetricsValue = performanceanalyzerActionListener.generateStartMetrics(123, "val2", 0).toString();
-        performanceanalyzerActionListener.saveMetricValues(startMetricsValue,  startTimeInMills, "search", "searchId1", "start");
-        fetchedValue = PerformanceAnalyzerMetrics.getMetric(
-                PluginSettings.instance().getMetricsLocation()
-                        + PerformanceAnalyzerMetrics.getTimeInterval(startTimeInMills)+"/threads/http/search/searchId1/start");
-        assertEquals(startMetricsValue, fetchedValue);
-
-        String finishMetricsValue = performanceanalyzerActionListener.generateFinishMetrics(456, 200, "val4").toString();
-        performanceanalyzerActionListener.saveMetricValues(finishMetricsValue, startTimeInMills, "search", "searchId1", "finish");
-        fetchedValue = PerformanceAnalyzerMetrics.getMetric(
-                PluginSettings.instance().getMetricsLocation()
-                        + PerformanceAnalyzerMetrics.getTimeInterval(startTimeInMills)+"/threads/http/search/searchId1/finish");
-        assertEquals(finishMetricsValue, fetchedValue);
-
-        performanceanalyzerActionListener.saveMetricValues(finishMetricsValue, startTimeInMills, "search", "searchId2", "finish");
-        fetchedValue = PerformanceAnalyzerMetrics.getMetric(
-                PluginSettings.instance().getMetricsLocation()
-                        + PerformanceAnalyzerMetrics.getTimeInterval(startTimeInMills)+"/threads/http/search/searchId2/finish");
-        assertEquals(finishMetricsValue, fetchedValue);
-
-        PerformanceAnalyzerMetrics.removeMetrics(PluginSettings.instance().getMetricsLocation()
-                + PerformanceAnalyzerMetrics.getTimeInterval(startTimeInMills));
+    public void testOnFailureWithException() {
+        NullPointerException exception = new NullPointerException();
+        actionListener.set(RequestType.search, listenerId , originalActionListener);
+        actionListener.onFailure(exception);
+        String[] metricsValues = readMetricsValue();
+        assertEquals("HTTPRespCode:-1", metricsValues[2]);
+        assertTrue( metricsValues[3].contains("Exception:java.lang.NullPointerException"));
+    }
+
+    private void testOnResponse(ActionResponse response) {
+        actionListener.onResponse(response);
+
+        String[] metricsValues = readMetricsValue();
+        assertEquals("HTTPRespCode:200", metricsValues[2]);
+        assertEquals("Exception:", metricsValues[3]);
+    }
+
+    private String[] readMetricsValue() {
+        List<Event> metrics = TestUtil.readEvents();
+        assert metrics.size() == 1;
+        String[] metricsValues = metrics.get(0).value.split("\n");
+        assert metricsValues.length == 4;
+        return metricsValues;
     }
 }
diff --git a/...distro/elasticsearch/performanceanalyzer/collectors/CacheConfigMetricsCollectorTests.java b/...distro/elasticsearch/performanceanalyzer/collectors/CacheConfigMetricsCollectorTests.java
@@ -46,6 +46,9 @@ public void init() {
 
     MetricsConfiguration.CONFIG_MAP.put(CacheConfigMetricsCollector.class, MetricsConfiguration.cdefault);
     collector = new CacheConfigMetricsCollector();
+
+    //clean metricQueue before running every test
+    TestUtil.readEvents();
   }
 
   @After

diff --git a/...opendistro/elasticsearch/performanceanalyzer/collectors/CircuitBreakerCollectorTests.java b/...opendistro/elasticsearch/performanceanalyzer/collectors/CircuitBreakerCollectorTests.java
@@ -47,6 +47,9 @@ public void init() {
 
         MetricsConfiguration.CONFIG_MAP.put(CircuitBreakerCollector.class, MetricsConfiguration.cdefault);
         collector = new CircuitBreakerCollector();
+
+        //clean metricQueue before running every test
+        TestUtil.readEvents();
     }
 
     @After