sergehuber commented on code in PR #776:
URL: https://github.com/apache/unomi/pull/776#discussion_r3433750756
##########
services/src/test/java/org/apache/unomi/services/impl/TestConditionEvaluators.java:
##########
@@ -431,7 +423,7 @@ private static boolean evaluateCollectionCondition(Object
actualValue, Condition
switch (operator) {
case "in": return actual.stream().anyMatch(expected::contains);
case "inContains": return actual.stream().anyMatch(a ->
- (a instanceof String) && expected.stream().anyMatch(b -> (b
instanceof String) && ((String) a).contains((String) b)));
+ expected.stream().anyMatch(b -> ((String) a).contains((String)
b)));
case "notIn": return actual.stream().noneMatch(expected::contains);
Review Comment:
Valid. The removal of `(a instanceof String)` and `(b instanceof String)`
guards causes `ClassCastException` when the collection contains non-string
elements. Restored the original guards.
##########
services/src/test/java/org/apache/unomi/services/impl/TestConditionEvaluators.java:
##########
@@ -532,13 +524,8 @@ private static ConditionEvaluator
createPastEventConditionEvaluator() {
String key = (String) parameters.get("generatedPropertyKey");
tracer.trace(condition, "Using generated property key: " +
key);
- Object pastEventsObj =
profile.getSystemProperties().get("pastEvents");
- if (!(pastEventsObj instanceof List)) {
- tracer.trace(condition, "No pastEvents found in profile
system properties");
- count = 0;
- } else {
- @SuppressWarnings("unchecked")
- List<Map<String, Object>> pastEvents = (List<Map<String,
Object>>) pastEventsObj;
+ List<Map<String, Object>> pastEvents = (ArrayList<Map<String,
Object>>) profile.getSystemProperties().get("pastEvents");
+ if (pastEvents != null) {
tracer.trace(condition, "Found pastEvents in profile
system properties");
Number l = (Number) pastEvents
.stream()
Review Comment:
Valid. Casting directly to `ArrayList` throws `ClassCastException` if the
stored value is any other `List` implementation, and also bypasses the original
type-safety check. Restored the original `instanceof List` guard with a cast to
`List<Map<String, Object>>`.
##########
services/src/test/java/org/apache/unomi/services/impl/TestConditionEvaluators.java:
##########
@@ -916,7 +906,7 @@ private static Parameter createParameterRecommended(String
name, String type, St
}
public static Map<String, ConditionType> getConditionTypes() {
- return Collections.unmodifiableMap(conditionTypes);
+ return conditionTypes;
}
Review Comment:
Valid. Returning the mutable backing map directly allows callers to
accidentally modify shared state across tests. Restored
`Collections.unmodifiableMap(conditionTypes)`.
##########
services/src/test/java/org/apache/unomi/services/impl/TestBundleContext.java:
##########
@@ -149,7 +148,7 @@ public <S> ServiceReference<S> getServiceReference(Class<S>
clazz) {
@Override
public <S> Collection<ServiceReference<S>> getServiceReferences(Class<S>
clazz, String filter) {
- return Collections.emptyList();
+ return null;
}
Review Comment:
Valid. The OSGi `BundleContext` contract specifies an empty collection when
no references exist, not null. Returning null can cause `NullPointerException`
in production code under test that iterates over the result. Fixed to return
`Collections.emptyList()`.
##########
services/src/test/java/org/apache/unomi/services/TestHelper.java:
##########
@@ -651,7 +796,7 @@ public static void cleanDefaultStorageDirectory(int
maxRetries) {
return;
}
} catch (IOException e) {
- LOGGER.warn("Error deleting default storage directory, will
retry", e);
+ LOGGER.warn("Error deleting default storage directory, will
retry: {}", e.getMessage());
}
Review Comment:
Valid. Using `e.getMessage()` as a format argument loses the stack trace and
exception chain, which makes intermittent CI failures harder to diagnose.
Restored passing the `Throwable` directly as the last argument to
`LOGGER.warn(...)` so SLF4J includes the full stack trace.
##########
services/src/test/java/org/apache/unomi/services/TestHelper.java:
##########
@@ -749,7 +894,7 @@ public static <T> T retryQueryUntilAvailable(Supplier<T>
querySupplier, Integer
Thread.sleep(retryDelay);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
- throw new RuntimeException("Retry interrupted", e);
+ break;
}
Review Comment:
Valid. Breaking out of the loop and falling through to a generic timeout
message hides the fact that the thread was interrupted, making failures caused
by JUnit timeouts or test shutdown confusing. Restored the original behaviour:
re-interrupt the thread and throw `new RuntimeException("Retry interrupted",
e)`.
##########
services/src/test/java/org/apache/unomi/services/impl/segments/SegmentServiceImplTest.java:
##########
@@ -0,0 +1,1163 @@
+/*
+ * 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.unomi.services.impl.segments;
+
+import org.apache.unomi.api.Event;
+import org.apache.unomi.api.Metadata;
+import org.apache.unomi.api.PartialList;
+import org.apache.unomi.api.Profile;
+import org.apache.unomi.api.actions.Action;
+import org.apache.unomi.api.conditions.Condition;
+import org.apache.unomi.api.conditions.ConditionType;
+import org.apache.unomi.api.exceptions.BadSegmentConditionException;
+import org.apache.unomi.api.rules.Rule;
+import org.apache.unomi.api.segments.Segment;
+import org.apache.unomi.api.segments.SegmentsAndScores;
+import org.apache.unomi.api.services.EventService;
+import org.apache.unomi.api.services.SchedulerService;
+import org.apache.unomi.api.services.cache.CacheableTypeConfig;
+import org.apache.unomi.persistence.spi.PersistenceService;
+import
org.apache.unomi.persistence.spi.conditions.evaluator.ConditionEvaluatorDispatcher;
+import org.apache.unomi.services.TestHelper;
+import org.apache.unomi.services.common.security.ExecutionContextManagerImpl;
+import org.apache.unomi.services.common.security.KarafSecurityService;
+import org.apache.unomi.services.impl.InMemoryPersistenceServiceImpl;
+import org.apache.unomi.services.impl.TestConditionEvaluators;
+import org.apache.unomi.services.impl.TestEventAdmin;
+import org.apache.unomi.services.impl.TestTenantService;
+import org.apache.unomi.services.impl.cache.MultiTypeCacheServiceImpl;
+import org.apache.unomi.services.impl.definitions.DefinitionsServiceImpl;
+import org.apache.unomi.services.impl.events.EventServiceImpl;
+import org.apache.unomi.services.impl.rules.RulesServiceImpl;
+import org.apache.unomi.services.impl.rules.TestActionExecutorDispatcher;
+import org.apache.unomi.services.impl.rules.TestEvaluateProfileSegmentsAction;
+import org.apache.unomi.services.impl.rules.TestSetEventOccurrenceCountAction;
+import org.apache.unomi.tracing.api.RequestTracer;
+import org.apache.unomi.tracing.api.TracerService;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.junit.jupiter.MockitoExtension;
+import org.mockito.junit.jupiter.MockitoSettings;
+import org.mockito.quality.Strictness;
+import org.osgi.framework.BundleContext;
+
+import java.io.IOException;
+import java.net.URL;
+import java.util.*;
+
+import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.when;
+
+@ExtendWith(MockitoExtension.class)
+@MockitoSettings(strictness = Strictness.LENIENT)
+public class SegmentServiceImplTest {
+
+ private SegmentServiceImpl segmentService;
+ private EventServiceImpl eventService;
+ private TestTenantService tenantService;
+ private PersistenceService persistenceService;
+ private DefinitionsServiceImpl definitionsService;
+ private MultiTypeCacheServiceImpl multiTypeCacheService;
+ private ExecutionContextManagerImpl executionContextManager;
+ private KarafSecurityService securityService;
+ private RulesServiceImpl rulesService;
+ private TestActionExecutorDispatcher actionExecutorDispatcher;
+
+ private BundleContext bundleContext;
+ private TracerService tracerService;
+ private RequestTracer requestTracer;
+
+ private SchedulerService schedulerService;
+
+ private static final String TENANT_1 = "tenant1";
+ private static final String TENANT_2 = "tenant2";
+ private static final String SYSTEM_TENANT = "system";
+
+ @BeforeEach
+ public void setUp() throws IOException {
+
+ tenantService = new TestTenantService();
+ securityService = TestHelper.createSecurityService();
+ executionContextManager =
TestHelper.createExecutionContextManager(securityService);
+ TracerService tracerService = TestHelper.createTracerService();
+
+ // Create tenants using TestHelper
+ TestHelper.setupCommonTestData(tenantService);
+
+ ConditionEvaluatorDispatcher conditionEvaluatorDispatcher =
TestConditionEvaluators.createDispatcher();
+
+ // Set up bundle context using TestHelper
+ bundleContext = TestHelper.createMockBundleContext();
+
+ persistenceService = new
InMemoryPersistenceServiceImpl(executionContextManager,
conditionEvaluatorDispatcher);
+
+ // Create scheduler service using TestHelper
+ schedulerService =
TestHelper.createSchedulerService("segment-service-scheduler-node",
persistenceService, executionContextManager, bundleContext, null, -1, true,
true);
+
+ multiTypeCacheService = new MultiTypeCacheServiceImpl();
+
+ // Create definitions service with EventAdmin
+ java.util.Map.Entry<DefinitionsServiceImpl, TestEventAdmin>
servicePair =
+
TestHelper.createDefinitionServiceWithEventAdmin(persistenceService,
bundleContext, schedulerService,
+ multiTypeCacheService, executionContextManager, tenantService);
+ definitionsService = servicePair.getKey();
+ TestEventAdmin testEventAdmin = servicePair.getValue();
+
+ // Inject definitionsService into the dispatcher
+
TestHelper.injectDefinitionsServiceIntoDispatcher(conditionEvaluatorDispatcher,
definitionsService);
+
+ TestConditionEvaluators.getConditionTypes().forEach((key, value) ->
definitionsService.setConditionType(value));
+
+ // Set up event service using TestHelper
+ eventService = TestHelper.createEventService(persistenceService,
bundleContext, definitionsService, tenantService, tracerService);
+ TestConditionEvaluators.setEventService(eventService);
+
+ // Set up action executor dispatcher
+ actionExecutorDispatcher = new
TestActionExecutorDispatcher(definitionsService, persistenceService);
+
actionExecutorDispatcher.setDefaultReturnValue(EventService.PROFILE_UPDATED);
+ actionExecutorDispatcher.setTracer(requestTracer);
+
+ // Set up rules service using TestHelper with EventAdmin
+ rulesService = TestHelper.createRulesService(persistenceService,
bundleContext, schedulerService, definitionsService, eventService,
executionContextManager, tenantService, multiTypeCacheService,
actionExecutorDispatcher, testEventAdmin);
+ rulesService.setTracerService(tracerService);
+
+ // Set up segment service
+ segmentService = new SegmentServiceImpl();
+ segmentService.setBundleContext(bundleContext);
+ segmentService.setPersistenceService(persistenceService);
+ segmentService.setDefinitionsService(definitionsService);
+ segmentService.setRulesService(rulesService);
+ segmentService.setEventService(eventService);
+ segmentService.setContextManager(executionContextManager);
+ segmentService.setSchedulerService(schedulerService);
+ segmentService.setCacheService(multiTypeCacheService);
+ segmentService.setTenantService(tenantService);
+ segmentService.setTracerService(tracerService);
+
+ actionExecutorDispatcher.addExecutor("setEventOccurenceCountAction",
+ new TestSetEventOccurrenceCountAction(definitionsService,
persistenceService));
+ actionExecutorDispatcher.addExecutor("evaluateProfileSegments",
+ new TestEvaluateProfileSegmentsAction(segmentService));
+
+ // Register TestEvaluateProfileSegmentsAction
+ actionExecutorDispatcher.addExecutor("evaluateProfileSegments", new
TestEvaluateProfileSegmentsAction(segmentService));
+
Review Comment:
Valid. The executor was registered twice in consecutive lines with separate
`new TestEvaluateProfileSegmentsAction(segmentService)` instances, which is
redundant and risks divergence in future edits. Removed the duplicate
registration.
##########
services/src/test/java/org/apache/unomi/services/impl/rules/TestSetEventOccurrenceCountAction.java:
##########
@@ -32,7 +32,7 @@
/**
* Test implementation of the SetEventOccurrenceCountAction.
- * TODO: This is a duplicate of the SetEventOccurrenceCountAction in the
unomi-plugins-base project to avoid introducing a dependency on it but should
be cleaned up.
+ * @TODO This is a duplicate of the SetEventOccurrenceCountAction in the
unomi-plugins-base project to avoid introducing a depending to it but should be
cleaned up.
*/
Review Comment:
Valid. `@TODO` is not a recognized Javadoc tag and can cause tooling
warnings. Also "introducing a depending to it" was a grammatical error. Changed
to `TODO:` (plain text) and corrected to "introducing a dependency on it".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
