fix:Improve tests separation based on code review comments

eclipse-edc · Apr 15, 2024 · 098cea1 · 098cea1
1 parent 4af488e
commit 098cea1
Show file tree

Hide file tree

Showing 10 changed files with 459 additions and 286 deletions.
diff --git a/...rg/eclipse/edc/connector/controlplane/services/asset/AssetServiceImplIntegrationTest.java b/...rg/eclipse/edc/connector/controlplane/services/asset/AssetServiceImplIntegrationTest.java
@@ -0,0 +1,78 @@
+/*
+ *  Copyright (c) 2024 Encho Belezirev (Digital Lights)
+ *
+ *  This program and the accompanying materials are made available under the
+ *  terms of the Apache License, Version 2.0 which is available at
+ *  https://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  SPDX-License-Identifier: Apache-2.0
+ *
+ *  Contributors:
+ *       Encho Belezirev (Digital Lights) - refactor(test): improve QueryValidator testing strategy
+ *
+ */
+
+package org.eclipse.edc.connector.controlplane.services.asset;
+
+import org.eclipse.edc.connector.controlplane.asset.spi.domain.Asset;
+import org.eclipse.edc.connector.controlplane.asset.spi.index.AssetIndex;
+import org.eclipse.edc.connector.controlplane.asset.spi.observe.AssetObservable;
+import org.eclipse.edc.connector.controlplane.contract.spi.negotiation.store.ContractNegotiationStore;
+import org.eclipse.edc.connector.controlplane.services.spi.asset.AssetService;
+import org.eclipse.edc.spi.query.Criterion;
+import org.eclipse.edc.spi.query.QuerySpec;
+import org.eclipse.edc.transaction.spi.NoopTransactionContext;
+import org.eclipse.edc.transaction.spi.TransactionContext;
+import org.eclipse.edc.validator.spi.DataAddressValidatorRegistry;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.ArgumentsProvider;
+import org.junit.jupiter.params.provider.ArgumentsSource;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.util.stream.Stream;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.eclipse.edc.spi.query.Criterion.criterion;
+import static org.junit.jupiter.params.provider.Arguments.arguments;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+class AssetServiceImplIntegrationTest {
+    private final AssetIndex index = mock();
+    private final ContractNegotiationStore contractNegotiationStore = mock();
+    private final TransactionContext dummyTransactionContext = new NoopTransactionContext();
+    private final AssetObservable observable = mock();
+    private final DataAddressValidatorRegistry dataAddressValidator = mock();
+    private final AssetService service = new AssetServiceImpl(index, contractNegotiationStore, dummyTransactionContext, observable, dataAddressValidator);
+
+    @ParameterizedTest
+    @ValueSource(strings = { Asset.PROPERTY_ID, Asset.PROPERTY_NAME, Asset.PROPERTY_DESCRIPTION, Asset.PROPERTY_VERSION, Asset.PROPERTY_CONTENT_TYPE })
+    void search_usingQueryValidator_withValidFilter_queryAssetsSuccessfully(String filter) {
+        var query = QuerySpec.Builder.newInstance().filter(criterion(filter, "=", "somevalue")).build();
+
+        service.search(query);
+
+        verify(index).queryAssets(query);
+    }
+
+    @ParameterizedTest
+    @ArgumentsSource(InvalidFilters.class)
+    void search_usingQueryValidator_withInvalidFilter_resultsInFailure(Criterion filter) {
+        var query = QuerySpec.Builder.newInstance().filter(filter).build();
+
+        var result = service.search(query);
+
+        assertThat(result.failed()).isTrue();
+    }
+
+    private static class InvalidFilters implements ArgumentsProvider {
+        @Override
+        public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
+            return Stream.of(arguments(criterion("  asset_prop_id", "in", "(foo, bar)")), // invalid leading whitespace
+                    arguments(criterion(".customProp", "=", "whatever"))  // invalid leading dot
+            );
+        }
+    }
+}
diff --git a/...test/java/org/eclipse/edc/connector/controlplane/services/asset/AssetServiceImplTest.java b/...test/java/org/eclipse/edc/connector/controlplane/services/asset/AssetServiceImplTest.java
@@ -24,7 +24,6 @@
 import org.eclipse.edc.connector.controlplane.services.query.QueryValidator;
 import org.eclipse.edc.connector.controlplane.services.spi.asset.AssetService;
 import org.eclipse.edc.policy.model.Policy;
-import org.eclipse.edc.spi.query.Criterion;
 import org.eclipse.edc.spi.query.QuerySpec;
 import org.eclipse.edc.spi.result.Failure;
 import org.eclipse.edc.spi.result.Result;
@@ -38,14 +37,7 @@
 import org.eclipse.edc.validator.spi.ValidationResult;
 import org.jetbrains.annotations.NotNull;
 import org.junit.jupiter.api.DisplayName;
-import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.ExtensionContext;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.Arguments;
-import org.junit.jupiter.params.provider.ArgumentsProvider;
-import org.junit.jupiter.params.provider.ArgumentsSource;
-import org.junit.jupiter.params.provider.ValueSource;
 
 import java.util.UUID;
 import java.util.function.Predicate;
@@ -58,7 +50,6 @@
 import static org.eclipse.edc.spi.result.ServiceFailure.Reason.CONFLICT;
 import static org.eclipse.edc.spi.result.ServiceFailure.Reason.NOT_FOUND;
 import static org.eclipse.edc.validator.spi.Violation.violation;
-import static org.junit.jupiter.params.provider.Arguments.arguments;
 import static org.mockito.AdditionalMatchers.and;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.argThat;
@@ -271,41 +262,6 @@ void updateAsset_shouldFail_whenPropertiesAreDuplicated() {
         verifyNoInteractions(index);
     }
 
-    @Nested
-    class QueryValidatorIntegrationTest {
-        private final AssetService service = new AssetServiceImpl(index, contractNegotiationStore, dummyTransactionContext, observable, dataAddressValidator);
-
-        @ParameterizedTest
-        @ValueSource(strings = {Asset.PROPERTY_ID, Asset.PROPERTY_NAME, Asset.PROPERTY_DESCRIPTION, Asset.PROPERTY_VERSION, Asset.PROPERTY_CONTENT_TYPE})
-        void search_validFilter(String filter) {
-            var query = QuerySpec.Builder.newInstance().filter(criterion(filter, "=", "somevalue")).build();
-
-            service.search(query);
-
-            verify(index).queryAssets(query);
-        }
-
-        @ParameterizedTest
-        @ArgumentsSource(InvalidFilters.class)
-        void search_invalidFilter(Criterion filter) {
-            var query = QuerySpec.Builder.newInstance().filter(filter).build();
-
-            var result = service.search(query);
-
-            assertThat(result).isFailed().extracting(Failure::getMessages).asList().hasSize(1);
-        }
-
-        private static class InvalidFilters implements ArgumentsProvider {
-            @Override
-            public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
-                return Stream.of(arguments(criterion("  asset_prop_id", "in", "(foo, bar)")), // invalid leading whitespace
-                        arguments(criterion(".customProp", "=", "whatever"))  // invalid leading dot
-                );
-            }
-        }
-
-    }
-
     @NotNull
     private Predicate<Asset> hasId(String assetId) {
         return it -> assetId.equals(it.getId());

diff --git a/...ontrolplane/services/contractdefinition/ContractDefinitionServiceImplIntegrationTest.java b/...ontrolplane/services/contractdefinition/ContractDefinitionServiceImplIntegrationTest.java
@@ -0,0 +1,91 @@
+/*
+ *  Copyright (c) 2024 Encho Belezirev (Digital Lights)
+ *
+ *  This program and the accompanying materials are made available under the
+ *  terms of the Apache License, Version 2.0 which is available at
+ *  https://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  SPDX-License-Identifier: Apache-2.0
+ *
+ *  Contributors:
+ *       Encho Belezirev (Digital Lights) - refactor(test): improve QueryValidator testing strategy
+ *
+ */
+
+package org.eclipse.edc.connector.controlplane.services.contractdefinition;
+
+import org.eclipse.edc.connector.controlplane.contract.spi.definition.observe.ContractDefinitionObservable;
+import org.eclipse.edc.connector.controlplane.contract.spi.definition.observe.ContractDefinitionObservableImpl;
+import org.eclipse.edc.connector.controlplane.contract.spi.offer.store.ContractDefinitionStore;
+import org.eclipse.edc.connector.controlplane.services.spi.contractdefinition.ContractDefinitionService;
+import org.eclipse.edc.spi.query.Criterion;
+import org.eclipse.edc.spi.query.QuerySpec;
+import org.eclipse.edc.transaction.spi.NoopTransactionContext;
+import org.eclipse.edc.transaction.spi.TransactionContext;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.ArgumentsProvider;
+import org.junit.jupiter.params.provider.ArgumentsSource;
+
+import java.util.stream.Stream;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.eclipse.edc.spi.query.Criterion.criterion;
+import static org.junit.jupiter.params.provider.Arguments.arguments;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+class ContractDefinitionServiceImplIntegrationTest {
+    private final ContractDefinitionStore store = mock();
+    private final TransactionContext transactionContext = new NoopTransactionContext();
+    private final ContractDefinitionObservable observable = new ContractDefinitionObservableImpl();
+    private final ContractDefinitionService service = new ContractDefinitionServiceImpl(store, transactionContext, observable);
+
+    @ParameterizedTest
+    @ArgumentsSource(InvalidFilters.class)
+    void search_usingQueryValidator_withInvalidFilter_resultsInFailure(Criterion invalidFilter) {
+        var query = QuerySpec.Builder.newInstance()
+                .filter(invalidFilter)
+                .build();
+
+        var result = service.search(query);
+
+        assertThat(result.failed()).isTrue();
+    }
+
+    @ParameterizedTest
+    @ArgumentsSource(ValidFilters.class)
+    void search_usingQueryValidator_withValidFilter_findsAllSuccessfully(Criterion validFilter) {
+        var query = QuerySpec.Builder.newInstance()
+                .filter(validFilter)
+                .build();
+        when(store.findAll(query)).thenReturn(Stream.empty());
+
+        service.search(query);
+
+        verify(store).findAll(query);
+    }
+
+    private static class InvalidFilters implements ArgumentsProvider {
+        @Override
+        public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
+            return Stream.of(
+                    arguments(criterion("assetsSelector.leftHand", "=", "foo")), // invalid path
+                    arguments(criterion("accessPolicyId'LIKE/**/?/**/LIMIT/**/?/**/OFFSET/**/?;DROP/**/TABLE/**/test/**/--%20", "=", "%20ABC--")) //some SQL injection
+            );
+        }
+    }
+
+    private static class ValidFilters implements ArgumentsProvider {
+        @Override
+        public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
+            return Stream.of(
+                    arguments(criterion("assetsSelector.operandLeft", "=", "foo")),
+                    arguments(criterion("assetsSelector.operator", "=", "LIKE")),
+                    arguments(criterion("assetsSelector.operandRight", "=", "bar"))
+            );
+        }
+    }
+}
diff --git a/...connector/controlplane/services/contractdefinition/ContractDefinitionServiceImplTest.java b/...connector/controlplane/services/contractdefinition/ContractDefinitionServiceImplTest.java
@@ -29,23 +29,15 @@
 import org.eclipse.edc.transaction.spi.TransactionContext;
 import org.jetbrains.annotations.NotNull;
 import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.ExtensionContext;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.Arguments;
-import org.junit.jupiter.params.provider.ArgumentsProvider;
-import org.junit.jupiter.params.provider.ArgumentsSource;
 
 import java.util.UUID;
 import java.util.function.Predicate;
 import java.util.stream.Stream;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.eclipse.edc.spi.query.Criterion.criterion;
 import static org.eclipse.edc.spi.result.ServiceFailure.Reason.CONFLICT;
 import static org.eclipse.edc.spi.result.ServiceFailure.Reason.NOT_FOUND;
-import static org.junit.jupiter.params.provider.Arguments.arguments;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.argThat;
@@ -210,56 +202,6 @@ void update_shouldReturnNotFound_whenTheStoreFails() {
         verify(listener, never()).updated(any());
     }
 
-    @Nested
-    class QueryValidatorIntegrationTest {
-        private final ContractDefinitionService service = new ContractDefinitionServiceImpl(store, transactionContext, observable);
-
-        @ParameterizedTest
-        @ArgumentsSource(InvalidFilters.class)
-        void search_invalidFilter(Criterion invalidFilter) {
-            var query = QuerySpec.Builder.newInstance()
-                    .filter(invalidFilter)
-                    .build();
-
-            var result = service.search(query);
-
-            assertThat(result.failed()).isTrue();
-        }
-
-        @ParameterizedTest
-        @ArgumentsSource(ValidFilters.class)
-        void search_validFilter(Criterion validFilter) {
-            var query = QuerySpec.Builder.newInstance()
-                    .filter(validFilter)
-                    .build();
-            when(store.findAll(query)).thenReturn(Stream.empty());
-
-            service.search(query);
-
-            verify(store).findAll(query);
-        }
-
-        private static class InvalidFilters implements ArgumentsProvider {
-            @Override
-            public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
-                return Stream.of(
-                        arguments(criterion("assetsSelector.leftHand", "=", "foo")), // invalid path
-                        arguments(criterion("accessPolicyId'LIKE/**/?/**/LIMIT/**/?/**/OFFSET/**/?;DROP/**/TABLE/**/test/**/--%20", "=", "%20ABC--")) //some SQL injection
-                );
-            }
-        }
-
-        private static class ValidFilters implements ArgumentsProvider {
-            @Override
-            public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
-                return Stream.of(
-                        arguments(criterion("assetsSelector.operandLeft", "=", "foo")),
-                        arguments(criterion("assetsSelector.operator", "=", "LIKE")),
-                        arguments(criterion("assetsSelector.operandRight", "=", "bar"))
-                );
-            }
-        }
-    }
 
     @NotNull
     private Predicate<ContractDefinition> hasId(String id) {