Copilot commented on code in PR #776:
URL: https://github.com/apache/unomi/pull/776#discussion_r3433349608
##########
services/src/test/java/org/apache/unomi/services/impl/TestEventAdmin.java:
##########
@@ -426,24 +418,23 @@ public int getHandlerCount() {
*/
public boolean waitForEventProcessing(long timeoutMs) {
long deadline = System.currentTimeMillis() + timeoutMs;
-
- // Wait until all queues are drained AND all in-flight handleEvent
calls have returned.
- // Checking only queue.isEmpty() is insufficient: workers remove
events from the queue
- // before calling handleEvent, so the queue can appear empty while
processing is ongoing.
- while (System.currentTimeMillis() < deadline) {
- boolean allQueuesEmpty =
handlerQueues.values().stream().allMatch(BlockingQueue::isEmpty);
- if (allQueuesEmpty && inFlightCount.get() == 0) {
- return true;
+
+ // Wait for all handler queues to be empty
+ for (BlockingQueue<Event> queue : handlerQueues.values()) {
+ while (!queue.isEmpty() && System.currentTimeMillis() < deadline) {
+ try {
+ Thread.sleep(10);
+ } catch (InterruptedException e) {
Review Comment:
`waitForEventProcessing` only waits for handler queues to become empty, but
in `registerHandler` the worker removes events from the queue *before* calling
`handleEvent`. This can make the queue empty while a handler is still
processing, causing this method to return `true` too early and introduce test
flakiness.
##########
services/src/test/java/org/apache/unomi/services/impl/TestEventAdmin.java:
##########
@@ -125,9 +117,13 @@ public class TestEventAdmin implements EventAdmin {
*/
private final List<Event> sentEvents = new CopyOnWriteArrayList<>();
+ /**
+ * Creates a TestEventAdmin with a single-threaded executor for ordered
event delivery.
+ */
public TestEventAdmin() {
- this.asyncExecutor = Executors.newCachedThreadPool(r -> {
- Thread t = new Thread(r, "TestEventAdmin-Handler-" +
System.identityHashCode(this));
+ // Use single-threaded executor to guarantee ordered delivery
+ this.asyncExecutor = Executors.newSingleThreadExecutor(r -> {
+ Thread t = new Thread(r, "TestEventAdmin-Async-" +
System.identityHashCode(this));
t.setDaemon(true);
return t;
});
Review Comment:
`asyncExecutor` is created as a single-threaded executor, but
`registerHandler` submits a long-lived worker per handler that blocks on
`queue.take()`. With a single thread, the first worker will block forever and
all subsequently registered handlers will never get a running worker, so their
queued events will never be delivered.
##########
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:
The `inContains` branch casts collection elements to `String`
unconditionally. If `actual` or `expected` contains a non-string element, this
will throw `ClassCastException` during evaluation.
##########
services/src/test/java/org/apache/unomi/services/impl/TestConditionEvaluators.java:
##########
@@ -373,13 +370,8 @@ private static boolean evaluateBetweenCondition(Object
actualValue, Condition co
private static boolean evaluateDateCondition(Object actualValue, Object
expectedValueDate,
Object expectedValueDateExpr,
String operator) {
Object expectedDate = expectedValueDate == null ?
expectedValueDateExpr : expectedValueDate;
- Date actualDateVal = getDate(actualValue);
- Date expectedDateVal = getDate(expectedDate);
- if (actualDateVal == null || expectedDateVal == null) {
- return false;
- }
- boolean isSameDay =
YEAR_MONTH_DAY_FORMAT.format(actualDateVal.toInstant())
-
.equals(YEAR_MONTH_DAY_FORMAT.format(expectedDateVal.toInstant()));
+ boolean isSameDay = yearMonthDayDateFormat.format(getDate(actualValue))
+ .equals(yearMonthDayDateFormat.format(getDate(expectedDate)));
return operator.equals("isDay") ? isSameDay : !isSameDay;
Review Comment:
`evaluateDateCondition` now calls
`yearMonthDayDateFormat.format(getDate(...))` without checking for null.
`DateUtils.getDate` returns null for unparsable inputs, so this can throw
`NullPointerException` and make evaluators fail unexpectedly.
##########
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:
`pastEvents` is cast to `ArrayList<Map<String,Object>>`, which will throw
`ClassCastException` if the stored list is another `List` implementation (or if
the value isn't a list at all). The previous code handled missing/wrong types
gracefully.
##########
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:
This log line drops the exception stack trace by only logging
`e.getMessage()`. When test cleanup fails intermittently on CI, having the full
exception (and its causes) is important for diagnosis.
##########
services/src/test/java/org/apache/unomi/services/impl/TestConditionEvaluators.java:
##########
@@ -83,15 +86,9 @@ private static ConditionEvaluator
createBooleanConditionEvaluator() {
return (condition, item, context, dispatcher) -> {
tracer.startOperation("boolean", "Evaluating boolean condition
with operator: " + condition.getParameter("operator"), condition);
String operator = (String) condition.getParameter("operator");
- Object subConditionsParam =
condition.getParameter("subConditions");
- if (!(subConditionsParam instanceof List)) {
- tracer.endOperation(true, "No subconditions found, returning
true");
- return true;
- }
- @SuppressWarnings("unchecked")
- List<Condition> subConditions = (List<Condition>)
subConditionsParam;
+ List<Condition> subConditions = (List<Condition>)
condition.getParameter("subConditions");
- if (subConditions.isEmpty()) {
+ if (subConditions == null || subConditions.isEmpty()) {
tracer.endOperation(true, "No subconditions found, returning
true");
Review Comment:
`subConditions` is cast directly to `List<Condition>` without checking the
runtime type. If the parameter is missing or not a `List`, this will throw
`ClassCastException` instead of behaving like the previous implementation
(treating it as no subconditions).
##########
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:
`getConditionTypes()` returns the mutable static map directly. Because this
map is shared across the entire test JVM, callers can accidentally mutate it
and create cross-test interference; the previous unmodifiable view avoided that.
##########
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:
On `InterruptedException`, the loop breaks and the method later throws a
generic timeout exception, discarding the original interrupt cause. Tests that
are interrupted (timeouts, shutdown) should preserve the interrupt reason to
avoid confusing failures.
##########
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:
`BundleContext#getServiceReferences(Class, String)` should return an empty
collection when no references exist; returning `null` can cause
`NullPointerException` in code under test that iterates over the result.
##########
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:
`evaluateProfileSegments` executor is registered twice. This is redundant
and makes it easier for future edits to accidentally diverge (e.g., different
executor instance/config).
##########
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:
Javadoc typo/grammar: "introducing a depending" should be "introducing a
dependency". Also `@TODO` isn't a standard Javadoc tag; use `TODO:` in plain
text to avoid Javadoc tooling warnings.
--
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]
