Skip to content

Commit 4725ef7

Browse files
thiagohoraCometActions
authored andcommitted
[OPIK-2856] [BE] Refactor test code to use TraceResourceClient instead of direct URL_TEMPLATE calls (#3947)
* [NA] [BE] Upgrade MySQL container from Testcontainers * Fix imports order * [OPIK-2856] [BE] Implement UUIDv7 time-based filtering for traces - Add InstantToUUIDMapper to convert Instant timestamps to UUIDv7 bounds - Add InstantParamConverter to parse ISO-8601 and epoch millisecond time parameters - Update TracesResource to accept from_time and to_time parameters on /traces and /traces/stats endpoints - Update TraceDAO to apply UUID-based time filtering using BETWEEN clause on id column - Update TraceSearchCriteria to include uuidFromTime and uuidToTime fields - Add comprehensive integration tests for time filtering with boundary conditions - All tests passing: 10/10 time filtering tests + validation tests * [OPIK-2856] Address PR review comments: improve exception handling and type safety - Fix InstantParamConverter to catch specific DateTimeParseException instead of generic Exception - Add debug logging when falling back to epoch milliseconds parsing - Refactor anonymous ParamConverter class to named InstantConverter inner class for clarity - Suppress unchecked cast with @SuppressWarnings annotation - Fix MySQLContainerUtils return type to use MySQLContainer<?> for type safety * [OPIK-2856] Fix InstantToUUIDMapperTest to match implementation - Update tests to reflect that toUpperBound uses next millisecond (+1ms) for inclusive BETWEEN queries - Remove outdated assertions expecting same timestamp in both bounds - Verify upper bound is lexicographically greater than lower bound - All 13 tests now passing * Remove setup duplicated code * Revision 2: Address PR review comments - LOW priority fixes - #7: Add INSTANCE singleton pattern for InstantConverter - #8: Use StringUtils.isEmpty() for null-safe empty check - Note: #2 and #3 already addressed in previous commit * Revision 3: Use IdGenerator.getTimeOrderedEpoch() for UUID bounds - Simplified InstantToUUIDMapper to use IdGenerator.getTimeOrderedEpoch() instead of convertOtelIdToUUIDv7 - Per UUIDv7 RFC, sub-millisecond 12 bits are optional with millisecond granularity - Start/end interval semantics with ±1ms ensures correct BETWEEN query results - This approach has been battle-tested for months without issues per reviewer recommendation - Converted InstantToUUIDMapper to @singleton service for proper DI integration - Updated TracesResource to inject InstantToUUIDMapper dependency - Updated tests to properly mock IdGenerator dependency * Fix tests * [OPIK-2856] [BE] Split Get spans Tests * Fix format * Revision 2: Extract duplicated span creation logic into createSpanWithTimestamp helper method * [OPIK-2856] [BE] Implement UUIDv7 time-based filtering for spans * Revision 3: Extract workspace setup duplication into setupTestWorkspace helper method and fix transformTestParams call * [OPIK-2856] Refactor TracesResourceTest to use TraceResourceClient instead of direct URL_TEMPLATE calls - Replace all direct client.target(URL_TEMPLATE) calls with TraceResourceClient methods - Add callFeedbackScoresWithCookie method to TraceResourceClient for session token authentication - Add callRetrieveThreadResponseWithCookie method to TraceResourceClient for session token authentication - Fix feedback batch endpoint by using callFeedbackScores and callFeedbackScoresWithCookie - Add null checks for query parameters to prevent NPE errors - Fix API key vs session token usage in authentication tests - Rename get__whenApiKeyIsPresent__thenReturnTraceThread to get__whenSessionTokenIsPresent__thenReturnTraceThread in SessionTokenCookie class - Add mockGetWorkspaceIdByName() calls for proper workspace mocking - Preserve original test assertions and behavior - All tests properly refactored to use resource client methods instead of direct HTTP calls * [OPIK-2856] Remove duplicate methods from TraceResourceClient - Remove callGetTraces() - duplicate of callGetTracesWithQueryParams() - Remove callSearchTraces() - duplicate of callSearchTracesStream() - Reduced code duplication and maintenance burden * Fix tests * Revision 2: Address Copilot review comments - remove redundant wrapper method and add clarifying comment * Revision 3: Extract duplicated path splitting logic into helper method addPathSegments() * Revision 4: Make getWebTarget() private and add callGetTraceThreadsWithSorting() public method * Revision 7: Move addPathSegments() and addQueryParameters() helper methods to BaseCommentResourceClient
1 parent 945b0f6 commit 4725ef7

6 files changed

Lines changed: 1699 additions & 1327 deletions

File tree

apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/resources/BaseCommentResourceClient.java

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,14 @@
66
import com.comet.opik.podam.PodamFactoryUtils;
77
import jakarta.ws.rs.HttpMethod;
88
import jakarta.ws.rs.client.Entity;
9+
import jakarta.ws.rs.client.WebTarget;
910
import jakarta.ws.rs.core.HttpHeaders;
1011
import jakarta.ws.rs.core.MediaType;
1112
import lombok.RequiredArgsConstructor;
1213
import ru.vyarus.dropwizard.guice.test.ClientSupport;
1314
import uk.co.jemos.podam.api.PodamFactory;
1415

16+
import java.util.Map;
1517
import java.util.UUID;
1618

1719
import static com.comet.opik.infrastructure.auth.RequestContext.WORKSPACE_HEADER;
@@ -105,4 +107,33 @@ public void deleteComments(BatchDelete request, String apiKey, String workspaceN
105107
assertThat(actualResponse.hasEntity()).isFalse();
106108
}
107109
}
110+
111+
/**
112+
* Helper method to add path segments to a WebTarget.
113+
* Splits the pathSuffix by "/" and adds each non-empty part as a path segment.
114+
*/
115+
protected WebTarget addPathSegments(WebTarget target, String pathSuffix) {
116+
if (pathSuffix != null && !pathSuffix.isEmpty()) {
117+
String[] pathParts = pathSuffix.split("/");
118+
for (String part : pathParts) {
119+
if (!part.isEmpty()) {
120+
target = target.path(part);
121+
}
122+
}
123+
}
124+
return target;
125+
}
126+
127+
/**
128+
* Helper method to add query parameters to a WebTarget.
129+
* Iterates through the queryParams map and adds each entry as a query parameter.
130+
*/
131+
protected WebTarget addQueryParameters(WebTarget target, Map<String, String> queryParams) {
132+
if (queryParams != null) {
133+
for (Map.Entry<String, String> entry : queryParams.entrySet()) {
134+
target = target.queryParam(entry.getKey(), entry.getValue());
135+
}
136+
}
137+
return target;
138+
}
108139
}

apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/resources/SpanResourceClient.java

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import com.comet.opik.api.DeleteFeedbackScore;
44
import com.comet.opik.api.FeedbackScore;
5+
import com.comet.opik.api.FeedbackScoreBatchContainer;
56
import com.comet.opik.api.FeedbackScoreBatchContainer.FeedbackScoreBatch;
67
import com.comet.opik.api.ProjectStats;
78
import com.comet.opik.api.Span;
@@ -11,6 +12,7 @@
1112
import com.comet.opik.api.filter.SpanFilter;
1213
import com.comet.opik.api.sorting.SortingField;
1314
import com.comet.opik.domain.SpanType;
15+
import com.comet.opik.infrastructure.auth.RequestContext;
1416
import com.comet.opik.infrastructure.llm.openai.OpenaiModelName;
1517
import com.comet.opik.utils.JsonUtils;
1618
import com.fasterxml.jackson.core.type.TypeReference;
@@ -128,6 +130,47 @@ public void feedbackScores(List<FeedbackScoreBatchItem> score, String apiKey, St
128130
}
129131
}
130132

133+
public Response callFeedbackScoresWithContainer(FeedbackScoreBatchContainer request, String apiKey,
134+
String workspaceName) {
135+
return client.target(RESOURCE_PATH.formatted(baseURI))
136+
.path("feedback-scores")
137+
.request()
138+
.header(HttpHeaders.AUTHORIZATION, apiKey)
139+
.header(WORKSPACE_HEADER, workspaceName)
140+
.put(Entity.json(request));
141+
}
142+
143+
public Response callGetSpansWithQueryParams(String apiKey, String workspaceName, Map<String, String> queryParams) {
144+
WebTarget target = addQueryParameters(client.target(RESOURCE_PATH.formatted(baseURI)), queryParams);
145+
146+
return target
147+
.request()
148+
.header(HttpHeaders.AUTHORIZATION, apiKey)
149+
.header(WORKSPACE_HEADER, workspaceName)
150+
.get();
151+
}
152+
153+
public Response callGetSpansWithQueryParamsAndCookie(String sessionToken, String workspaceName,
154+
Map<String, String> queryParams) {
155+
WebTarget target = addQueryParameters(client.target(RESOURCE_PATH.formatted(baseURI)), queryParams);
156+
157+
return target
158+
.request()
159+
.cookie(RequestContext.SESSION_COOKIE, sessionToken)
160+
.header(WORKSPACE_HEADER, workspaceName)
161+
.get();
162+
}
163+
164+
public Response callFeedbackScoresWithContainerAndCookie(FeedbackScoreBatchContainer request, String sessionToken,
165+
String workspaceName) {
166+
return client.target(RESOURCE_PATH.formatted(baseURI))
167+
.path("feedback-scores")
168+
.request()
169+
.cookie(RequestContext.SESSION_COOKIE, sessionToken)
170+
.header(WORKSPACE_HEADER, workspaceName)
171+
.put(Entity.json(request));
172+
}
173+
131174
public void feedbackScore(UUID entityId, FeedbackScore score, String workspaceName, String apiKey) {
132175
try (var actualResponse = client.target(RESOURCE_PATH.formatted(baseURI))
133176
.path(entityId.toString())

0 commit comments

Comments
 (0)