This is an automated email from the ASF dual-hosted git repository. pdallig pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push: new 7a97ffe11d [ZEPPELIN-6196] Improve and Add Unit Tests for Elasticsearch Client and Wrappers 7a97ffe11d is described below commit 7a97ffe11da01faebf04a178bffdfa3c0ca0abe9 Author: Gyeongtae Park <67095975+parkgyeong...@users.noreply.github.com> AuthorDate: Fri Jun 27 05:19:23 2025 +0900 [ZEPPELIN-6196] Improve and Add Unit Tests for Elasticsearch Client and Wrappers ### What is this PR for? This PR improves the test coverage for the Elasticsearch interpreter module by: - Refactoring and enhancing the existing unit tests in `ElasticsearchClientTypeTest` and `ElasticsearchClientTypeBuilderTest` - Adding new unit tests for `AggWrapper` and `HitWrapper` These changes help ensure better reliability and maintainability of Elasticsearch-related components in Apache Zeppelin. ### What type of PR is it? Improvement ### Todos * [x] - Refactor tests for ElasticsearchClientType and Builder * [x] - Add new tests for AggWrapper * [x] - Add new tests for HitWrapper ### What is the Jira issue? * Jira: https://issues.apache.org/jira/browse/ZEPPELIN-6196 ### How should this be tested? The following test classes were added or modified: - `ElasticsearchClientTypeTest` - `ElasticsearchClientTypeBuilderTest` - `AggWrapperTest` - `HitWrapperTest` You can verify these tests by running: `./mvnw -pl elasticsearch test` ### Screenshots (if appropriate) N/A ### Questions: * Does the license files need to update? No. * Is there breaking changes for older versions? No. * Does this needs documentation? No. Closes #4942 from ParkGyeongTae/improve-elasticsearch-client-tests. Signed-off-by: Philipp Dallig <philipp.dal...@gmail.com> --- .../elasticsearch/action/AggWrapperTest.java | 70 +++++++++++++++++ .../elasticsearch/action/HitWrapperTest.java | 89 ++++++++++++++++++++++ .../client/ElasticsearchClientTypeBuilderTest.java | 75 +++++------------- .../client/ElasticsearchClientTypeTest.java | 44 +++++++---- 4 files changed, 206 insertions(+), 72 deletions(-) diff --git a/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/action/AggWrapperTest.java b/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/action/AggWrapperTest.java new file mode 100644 index 0000000000..4ea2356f1d --- /dev/null +++ b/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/action/AggWrapperTest.java @@ -0,0 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zeppelin.elasticsearch.action; + +import org.apache.zeppelin.elasticsearch.action.AggWrapper.AggregationType; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; + +class AggWrapperTest { + + @Test + @DisplayName("should store type and result correctly when type is SIMPLE") + void shouldStoreSimpleAggregationCorrectly() { + // Given + AggregationType type = AggregationType.SIMPLE; + String result = "{\"value\":42}"; + + // When + AggWrapper wrapper = new AggWrapper(type, result); + + // Then + assertEquals(type, wrapper.getType()); + assertEquals(result, wrapper.getResult()); + } + + @Test + @DisplayName("should store type and result correctly when type is MULTI_BUCKETS") + void shouldStoreMultiBucketsAggregationCorrectly() { + // Given + String result = "[{\"key\":\"a\"},{\"key\":\"b\"}]"; + + // When + AggWrapper wrapper = new AggWrapper(AggregationType.MULTI_BUCKETS, result); + + // Then + assertEquals(AggregationType.MULTI_BUCKETS, wrapper.getType()); + assertEquals(result, wrapper.getResult()); + } + + @Test + @DisplayName("AggregationType should contain SIMPLE and MULTI_BUCKETS in order") + void shouldContainExpectedAggregationTypes() { + // Given + AggregationType[] expected = { + AggregationType.SIMPLE, + AggregationType.MULTI_BUCKETS + }; + + // Then + assertArrayEquals(expected, AggregationType.values()); + } +} diff --git a/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/action/HitWrapperTest.java b/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/action/HitWrapperTest.java new file mode 100644 index 0000000000..e90194d5f6 --- /dev/null +++ b/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/action/HitWrapperTest.java @@ -0,0 +1,89 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zeppelin.elasticsearch.action; + +import com.google.gson.JsonObject; +import com.google.gson.JsonSyntaxException; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +class HitWrapperTest { + + private final String validJson = "{\"foo\":\"bar\",\"num\":42}"; + + @Test + @DisplayName("should store index, type, id, and source correctly") + void shouldStoreFieldsCorrectly() { + // Given + String index = "my-index"; + String type = "_doc"; + String id = "123"; + + // When + HitWrapper hit = new HitWrapper(index, type, id, validJson); + + // Then + assertEquals(index, hit.getIndex()); + assertEquals(type, hit.getType()); + assertEquals(id, hit.getId()); + assertEquals(validJson, hit.getSourceAsString()); + } + + @Test + @DisplayName("should parse valid JSON source into JsonObject") + void shouldParseSourceAsJsonObject() { + // Given + HitWrapper hit = new HitWrapper(validJson); + + // When + JsonObject json = hit.getSourceAsJsonObject(); + + // Then + assertEquals("bar", json.get("foo").getAsString()); + assertEquals(42, json.get("num").getAsInt()); + } + + @Test + @DisplayName("should allow construction with source only (null index/type/id)") + void shouldSupportConstructorWithSourceOnly() { + // Given + HitWrapper hit = new HitWrapper(validJson); + + // Then + assertNull(hit.getIndex()); + assertNull(hit.getType()); + assertNull(hit.getId()); + assertEquals(validJson, hit.getSourceAsString()); + } + + @Test + @DisplayName("should throw JsonSyntaxException for invalid JSON") + void shouldThrowForInvalidJsonSource() { + // Given + String invalidJson = "{not_json}"; + HitWrapper hit = new HitWrapper(invalidJson); + + // Then + assertThrows(JsonSyntaxException.class, hit::getSourceAsJsonObject, + "Invalid JSON string should throw JsonSyntaxException"); + } +} diff --git a/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/client/ElasticsearchClientTypeBuilderTest.java b/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/client/ElasticsearchClientTypeBuilderTest.java index 39b019cd11..03fca638f1 100644 --- a/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/client/ElasticsearchClientTypeBuilderTest.java +++ b/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/client/ElasticsearchClientTypeBuilderTest.java @@ -17,69 +17,30 @@ package org.apache.zeppelin.elasticsearch.client; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; -import static org.apache.zeppelin.elasticsearch.client.ElasticsearchClientType.HTTP; -import static org.apache.zeppelin.elasticsearch.client.ElasticsearchClientType.HTTPS; -import static org.apache.zeppelin.elasticsearch.client.ElasticsearchClientType.TRANSPORT; -import static org.apache.zeppelin.elasticsearch.client.ElasticsearchClientType.UNKNOWN; import static org.junit.jupiter.api.Assertions.assertEquals; -import org.junit.jupiter.api.Test; - class ElasticsearchClientTypeBuilderTest { - @Test - void it_should_return_transport_as_default_value_when_property_is_empty() { - //GIVEN - String empty = ""; - //WHEN - ElasticsearchClientType clientType = - ElasticsearchClientTypeBuilder.withPropertyValue(empty).build(); - //THEN - assertEquals(TRANSPORT, clientType); - } - - @Test - void it_should_return_transport_as_default_value_when_property_is_null() { - //GIVEN - String nullValue = null; - //WHEN - ElasticsearchClientType clientType = - ElasticsearchClientTypeBuilder.withPropertyValue(nullValue).build(); - //THEN - assertEquals(TRANSPORT, clientType); - } - - @Test - void it_should_return_client_type_when_property_value_exists() { - //GIVEN - String clientType = "https"; - //WHEN - ElasticsearchClientType esClientType = - ElasticsearchClientTypeBuilder.withPropertyValue(clientType).build(); - //THEN - assertEquals(HTTPS, esClientType); - } - - @Test - void it_should_return_client_type_and_ignore_case_when_property_value_exists() { - //GIVEN - String clientType = "hTtP"; - //WHEN - ElasticsearchClientType esClientType = - ElasticsearchClientTypeBuilder.withPropertyValue(clientType).build(); - //THEN - assertEquals(HTTP, esClientType); + private ElasticsearchClientType buildFrom(String propertyValue) { + return ElasticsearchClientTypeBuilder.withPropertyValue(propertyValue).build(); } - @Test - void it_should_return_unknown_when_property_value_does_not_exist() { - //GIVEN - String unknownValue = "an_unknown_value"; - //WHEN - ElasticsearchClientType esClientType = - ElasticsearchClientTypeBuilder.withPropertyValue(unknownValue).build(); - //THEN - assertEquals(UNKNOWN, esClientType); + @ParameterizedTest(name = "property = \"{0}\" → expected = {1}") + @CsvSource({ + "'', TRANSPORT", + "null, TRANSPORT", + "https, HTTPS", + "hTtP, HTTP", + "an_unknown_value, UNKNOWN" + }) + @DisplayName("should resolve correct ElasticsearchClientType from property string") + void shouldResolveClientTypeFromProperty(String property, ElasticsearchClientType expected) { + String resolved = "null".equals(property) ? null : property; + assertEquals(expected, buildFrom(resolved), + String.format("Expected %s for input '%s'", expected, property)); } } diff --git a/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/client/ElasticsearchClientTypeTest.java b/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/client/ElasticsearchClientTypeTest.java index 9ff921627b..7260d22f30 100644 --- a/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/client/ElasticsearchClientTypeTest.java +++ b/elasticsearch/src/test/java/org/apache/zeppelin/elasticsearch/client/ElasticsearchClientTypeTest.java @@ -17,27 +17,41 @@ package org.apache.zeppelin.elasticsearch.client; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; +class ElasticsearchClientTypeTest { -import org.junit.jupiter.api.Test; + @ParameterizedTest + @EnumSource(value = ElasticsearchClientType.class, names = {"HTTP", "HTTPS"}) + @DisplayName("should be marked as HTTP-based when client type is HTTP or HTTPS") + void shouldBeHttpWhenTypeIsHttpOrHttps(ElasticsearchClientType type) { + assertTrue(type.isHttp(), type + " should be marked as HTTP-based"); + } -class ElasticsearchClientTypeTest { + @ParameterizedTest + @EnumSource(value = ElasticsearchClientType.class, names = {"TRANSPORT", "UNKNOWN"}) + @DisplayName("should NOT be marked as HTTP-based when client type is TRANSPORT or UNKNOWN") + void shouldNotBeHttpWhenTypeIsTransportOrUnknown(ElasticsearchClientType type) { + assertFalse(type.isHttp(), type + " should NOT be marked as HTTP-based"); + } @Test - void it_should_return_http_when_reducing_on_http_types() { - //GIVEN - List<ElasticsearchClientType> httpTypes = - new ArrayList<>(Arrays.asList(ElasticsearchClientType.HTTP, ElasticsearchClientType.HTTPS)); - //WHEN - Boolean httpTypesReduced = httpTypes.stream() - .map(ElasticsearchClientType::isHttp) - .reduce(true, (ident, elasticsearchClientType) -> ident && elasticsearchClientType); - //THEN - assertTrue(httpTypesReduced); + @DisplayName("should contain all expected enum values in order") + void shouldContainAllEnumValuesInOrder() { + ElasticsearchClientType[] expected = { + ElasticsearchClientType.HTTP, + ElasticsearchClientType.HTTPS, + ElasticsearchClientType.TRANSPORT, + ElasticsearchClientType.UNKNOWN + }; + assertArrayEquals(expected, ElasticsearchClientType.values(), "Enum values should match " + + "declared order"); } }