From c79308d71c71ae35e137d4406f6cebd1aa87b9ea Mon Sep 17 00:00:00 2001 From: Chenyang Ji Date: Wed, 18 Sep 2024 14:13:58 -0700 Subject: [PATCH] Improve unit test coverage for Query Insights Signed-off-by: Chenyang Ji --- .../insights/core/exporter/DebugExporter.java | 19 +++++++-- .../core/exporter/DebugExporterTests.java | 35 ++++++++++++---- .../exporter/LocalIndexExporterTests.java | 11 +++++ .../core/service/TopQueriesServiceTests.java | 41 +++++++++++++++++++ 4 files changed, 93 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/opensearch/plugin/insights/core/exporter/DebugExporter.java b/src/main/java/org/opensearch/plugin/insights/core/exporter/DebugExporter.java index d63f19c..91cc821 100644 --- a/src/main/java/org/opensearch/plugin/insights/core/exporter/DebugExporter.java +++ b/src/main/java/org/opensearch/plugin/insights/core/exporter/DebugExporter.java @@ -20,12 +20,23 @@ public final class DebugExporter implements QueryInsightsExporter { /** * Logger of the debug exporter */ - private final Logger logger = LogManager.getLogger(); + private final Logger logger; /** - * Constructor of DebugExporter + * Private constructor for singleton pattern. */ - private DebugExporter() {} + private DebugExporter() { + this.logger = LogManager.getLogger(); + } + + /** + * Package-private constructor for injecting a custom logger, used for testing + * + * @param logger + */ + DebugExporter(Logger logger) { + this.logger = logger; + } private static class InstanceHolder { private static final DebugExporter INSTANCE = new DebugExporter(); @@ -34,7 +45,7 @@ private static class InstanceHolder { /** Get the singleton instance of DebugExporter * - @return DebugExporter instance + * @return DebugExporter instance */ public static DebugExporter getInstance() { return InstanceHolder.INSTANCE; diff --git a/src/test/java/org/opensearch/plugin/insights/core/exporter/DebugExporterTests.java b/src/test/java/org/opensearch/plugin/insights/core/exporter/DebugExporterTests.java index 45d91f1..1ace3fe 100644 --- a/src/test/java/org/opensearch/plugin/insights/core/exporter/DebugExporterTests.java +++ b/src/test/java/org/opensearch/plugin/insights/core/exporter/DebugExporterTests.java @@ -9,28 +9,45 @@ package org.opensearch.plugin.insights.core.exporter; import java.util.List; +import java.util.Locale; + +import org.apache.logging.log4j.core.Logger; import org.junit.Before; import org.opensearch.plugin.insights.QueryInsightsTestUtils; import org.opensearch.plugin.insights.rules.model.SearchQueryRecord; import org.opensearch.test.OpenSearchTestCase; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + /** - * Granular tests for the {@link DebugExporterTests} class. + * Unit tests for the {@link DebugExporterTests} class. */ public class DebugExporterTests extends OpenSearchTestCase { private DebugExporter debugExporter; + private Logger mockLogger = mock(Logger.class); @Before public void setup() { - debugExporter = DebugExporter.getInstance(); + debugExporter = new DebugExporter(mockLogger); + } + + public void testGetInstance() { + DebugExporter instance1 = DebugExporter.getInstance(); + DebugExporter instance2 = DebugExporter.getInstance(); + assertEquals(instance1, instance2); + } + + public void testExport() {; + List records = QueryInsightsTestUtils.generateQueryInsightRecords(1); + debugExporter.export(records); + // Verify that the logger received the expected debug message + verify(mockLogger).debug(String.format(Locale.ROOT, "QUERY_INSIGHTS_RECORDS: %s", records)); } - public void testExport() { - List records = QueryInsightsTestUtils.generateQueryInsightRecords(2); - try { - debugExporter.export(records); - } catch (Exception e) { - fail("No exception should be thrown when exporting query insights data"); - } + public void testClose() { + debugExporter.close(); + // Verify that the logger received the expected debug message + verify(mockLogger).debug("Closing the DebugExporter.."); } } diff --git a/src/test/java/org/opensearch/plugin/insights/core/exporter/LocalIndexExporterTests.java b/src/test/java/org/opensearch/plugin/insights/core/exporter/LocalIndexExporterTests.java index ebd9b03..27889fe 100644 --- a/src/test/java/org/opensearch/plugin/insights/core/exporter/LocalIndexExporterTests.java +++ b/src/test/java/org/opensearch/plugin/insights/core/exporter/LocalIndexExporterTests.java @@ -12,8 +12,11 @@ import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.util.Arrays; import java.util.List; import org.joda.time.format.DateTimeFormat; import org.joda.time.format.DateTimeFormatter; @@ -82,6 +85,14 @@ public void testExportRecordsWithError() { } } + public void testExportWithNoRecords() { + // Call the export method with no records + localIndexExporter.export(null); + localIndexExporter.export(List.of()); + // Verify no interactions with the client + verify(client, times(0)).prepareBulk(); + } + public void testClose() { try { localIndexExporter.close(); diff --git a/src/test/java/org/opensearch/plugin/insights/core/service/TopQueriesServiceTests.java b/src/test/java/org/opensearch/plugin/insights/core/service/TopQueriesServiceTests.java index 58854f9..648aee2 100644 --- a/src/test/java/org/opensearch/plugin/insights/core/service/TopQueriesServiceTests.java +++ b/src/test/java/org/opensearch/plugin/insights/core/service/TopQueriesServiceTests.java @@ -8,8 +8,11 @@ package org.opensearch.plugin.insights.core.service; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import java.util.Collections; import java.util.List; import java.util.concurrent.TimeUnit; import org.junit.Before; @@ -19,6 +22,7 @@ import org.opensearch.plugin.insights.core.exporter.QueryInsightsExporterFactory; import org.opensearch.plugin.insights.core.reader.QueryInsightsReaderFactory; import org.opensearch.plugin.insights.rules.model.GroupingType; +import org.opensearch.plugin.insights.rules.model.Measurement; import org.opensearch.plugin.insights.rules.model.MetricType; import org.opensearch.plugin.insights.rules.model.SearchQueryRecord; import org.opensearch.plugin.insights.settings.QueryInsightsSettings; @@ -42,6 +46,43 @@ public void setup() { topQueriesService.setEnabled(true); } + public void testSetAndGetTopNSize() { + int newSize = 5; + topQueriesService.setTopNSize(newSize); + assertEquals(newSize, topQueriesService.getTopNSize()); + } + + public void testConsumeRecords() { + // Prepare some mock SearchQueryRecords + SearchQueryRecord record1 = mock(SearchQueryRecord.class); + when(record1.getTimestamp()).thenReturn(System.currentTimeMillis()); + when(record1.getMeasurements()).thenReturn(Collections.singletonMap(MetricType.LATENCY, new Measurement(1000L))); + when(record1.getMeasurement(any())).thenReturn(1000L); + SearchQueryRecord record2 = mock(SearchQueryRecord.class); + when(record2.getTimestamp()).thenReturn(System.currentTimeMillis()); + when(record2.getMeasurements()).thenReturn(Collections.singletonMap(MetricType.LATENCY, new Measurement(2000L))); + when(record2.getMeasurement(any())).thenReturn(2000L); + // Consume records + topQueriesService.consumeRecords(List.of(record1, record2)); + // Verify that the topQueriesStore contains the records + List snapshot = topQueriesService.getTopQueriesCurrentSnapshot(); + assertEquals(2, snapshot.size()); + } + + public void testGetTopQueriesRecords() { + topQueriesService.setEnabled(true); + // Prepare some mock SearchQueryRecords and add to service + SearchQueryRecord record1 = mock(SearchQueryRecord.class); + when(record1.getTimestamp()).thenReturn(System.currentTimeMillis()); + when(record1.getMeasurements()).thenReturn(Collections.singletonMap(MetricType.LATENCY, new Measurement(1000L))); + topQueriesService.consumeRecords(List.of(record1)); + // Fetch records + List result = topQueriesService.getTopQueriesRecords(false, null, null); + // Verify the result + assertNotNull(result); + assertEquals(1, result.size()); + } + public void testIngestQueryDataWithLargeWindow() { final List records = QueryInsightsTestUtils.generateQueryInsightRecords(10); topQueriesService.consumeRecords(records);