Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 41 additions & 6 deletions .agents/skills/opik-backend/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -295,19 +295,54 @@ assertThat(stored).hasSize(2);
assertThat(version.itemsTotal()).isEqualTo(stored.size());

// ✅ GOOD - names the rows that must survive, so a wrong-winner bug fails
assertThat(stored)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsExactlyInAnyOrder(winningDuplicate, distinctItem);
assertDatasetItemsInAnyOrder(stored, winningDuplicate, distinctItem);
assertThat(version.itemsTotal()).isEqualTo(stored.size());
```

Asserting a derived counter against `stored.size()` is good — it ties the counter to reality
rather than to a literal — but it is only as strong as the assertion on `stored` itself. Pin the
contents first, then tie the counter to them.

Reuse the shared ignore-field constants (`IGNORED_FIELDS_DATA_ITEM` and friends) rather than
declaring a local list: they already encode which server-generated fields are not part of the
contract, and a local copy silently drifts from them.
Reach for the assertion helper before the constant. Where a helper class or method already covers
the entity, call it — reusing only its ignore-field constant still leaves the comparator chain
re-derived at each call site, which is what drifts. The helpers live under
`api/resources/utils/`: `TraceAssertions`, `SpanAssertions`, `DatasetItemAssertions`,
`AlertAssertions`, `PromptTestAssertions` and `ExperimentTestAssertions`.

```java
// ❌ BAD - comparator chain re-derived; the next field to ignore has to be found here too
assertThat(actualItems)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsExactlyElementsOf(expectedItems);

// ✅ GOOD - the helper owns both the ignore list and the comparison
assertDatasetItemsInOrder(actualItems, expectedItems);
```

Each helper owns the ignore-field constant for the comparisons it covers, so those comparisons
have exactly one declaration. Never re-declare that list locally and never import it from another
test class: a second copy drifts silently, and the resulting failure reads like a product bug
rather than a stale ignore list.

A call site whose contract genuinely differs may still ignore a different set — but derive it from
the shared constant rather than rebuilding the list, so the base set stays in one place:

```java
// ✅ GOOD - server generates the id, so the expected item cannot pin it
.ignoringFields(ignoredFieldsPlus("id"))

// ❌ BAD - a hand-written list that silently drifts from the shared one
private static final String[] MY_IGNORED_FIELDS = {"id", "createdAt", /* ...9 more... */};
```

Two cases need care when consolidating. A field that one comparison ignores and another asserts is
a real difference, not drift — folding it into the shared list quietly drops coverage, so keep the
narrower set and assert that field explicitly where it matters. And `ignoredFieldsPlus` only fits
where the helper's comparator semantics already apply; a call site needing a different comparison
shape keeps its own chain.

If no helper exists for the entity yet and more than one test class needs the assertion, add one
under `api/resources/utils/` rather than hoisting a constant into a test class.

These `containsExactly*` variants compare elements with the element type's own `equals`, which is
what you want for exact-valued models. When the elements carry `BigDecimal` or `double`, the same
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package com.comet.opik.api.resources.utils.datasets;

import com.comet.opik.api.DatasetItem;

import java.util.Arrays;
import java.util.List;
import java.util.stream.Stream;

import static org.assertj.core.api.Assertions.assertThat;

public class DatasetItemAssertions {

public static final String[] IGNORED_FIELDS_DATA_ITEM = {"createdAt", "lastUpdatedAt", "experimentItems",
"createdBy", "lastUpdatedBy", "datasetId", "tags", "datasetItemId", "runSummariesByExperiment"};
Comment thread
JetoPistola marked this conversation as resolved.

/**
* Extends {@link #IGNORED_FIELDS_DATA_ITEM} for call sites that ignore an extra field here and then assert it
* separately, because it needs different semantics than plain recursive equality.
*/
public static String[] ignoredFieldsPlus(String... extraFields) {
return Stream.concat(Arrays.stream(IGNORED_FIELDS_DATA_ITEM), Arrays.stream(extraFields))
.toArray(String[]::new);
}

public static void assertDatasetItem(DatasetItem actual, DatasetItem expected) {
assertThat(actual)
.usingRecursiveComparison()
.ignoringFields(IGNORED_FIELDS_DATA_ITEM)
.isEqualTo(expected);
}

public static void assertDatasetItems(List<DatasetItem> actual, List<DatasetItem> expected) {
assertThat(actual)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.isEqualTo(expected);
}

public static void assertDatasetItemsInOrder(List<DatasetItem> actual, List<DatasetItem> expected) {
assertThat(actual)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsExactlyElementsOf(expected);
}

public static void assertDatasetItemsInAnyOrder(List<DatasetItem> actual, DatasetItem... expected) {
assertThat(actual)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsExactlyInAnyOrder(expected);
}

public static void assertDatasetItemsContain(List<DatasetItem> actual, List<DatasetItem> expected) {
assertThat(actual)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsAll(expected);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,11 @@

import static com.comet.opik.api.resources.utils.ClickHouseContainerUtils.DATABASE_NAME;
import static com.comet.opik.api.resources.utils.WireMockUtils.WireMockRuntime;
import static com.comet.opik.api.resources.v1.priv.DatasetsResourceTest.IGNORED_FIELDS_DATA_ITEM;
import static com.comet.opik.api.resources.utils.datasets.DatasetItemAssertions.assertDatasetItem;
import static com.comet.opik.api.resources.utils.datasets.DatasetItemAssertions.assertDatasetItems;
import static com.comet.opik.api.resources.utils.datasets.DatasetItemAssertions.assertDatasetItemsContain;
import static com.comet.opik.api.resources.utils.datasets.DatasetItemAssertions.assertDatasetItemsInAnyOrder;
import static com.comet.opik.api.resources.utils.datasets.DatasetItemAssertions.assertDatasetItemsInOrder;
import static com.comet.opik.infrastructure.db.TransactionTemplateAsync.READ_ONLY;
import static com.comet.opik.infrastructure.db.TransactionTemplateAsync.WRITE;
import static org.assertj.core.api.Assertions.assertThat;
Expand Down Expand Up @@ -1078,9 +1082,7 @@ void applyChanges__whenCombinedAddEditDelete__thenCreateNewVersion() {
var v1ItemsAfter = datasetResourceClient.getDatasetItems(
datasetId, 1, 10, "v1", API_KEY, TEST_WORKSPACE).content();
assertThat(v1ItemsAfter).hasSize(3);
assertThat(v1ItemsAfter)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.isEqualTo(v1Items);
assertDatasetItems(v1ItemsAfter, v1Items);
}

@Test
Expand Down Expand Up @@ -1353,10 +1355,8 @@ void applyChanges__whenBaseVersionItemsTotalIsStale__thenUnchangedItemsArePreser
assertThat(v2Items).hasSize(6);

// Every v1 item must appear in v2 with the same fields. id changes per version, so
// we ignore it via IGNORED_FIELDS_DATA_ITEM and compare the rest of the entity.
assertThat(v2Items)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsAll(v1Items);
// the helper ignores it and compares the rest of the entity.
assertDatasetItemsContain(v2Items, v1Items);
}
}

Expand Down Expand Up @@ -1561,9 +1561,7 @@ void deleteItems__whenBaseVersionItemsTotalIsStale__thenUnchangedItemsArePreserv
assertThat(v2Items.stream().map(DatasetItem::datasetItemId))
.doesNotContain(itemToDelete.datasetItemId());

assertThat(v2Items)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsAll(expectedSurvivors);
assertDatasetItemsContain(v2Items, expectedSurvivors);
}

@Test
Expand Down Expand Up @@ -3613,18 +3611,14 @@ void sortByJsonKeyThroughPushTopLimit(String namespace, String jsonKey, Directio
datasetId, List.of(experimentId), null, null, sorting, API_KEY, TEST_WORKSPACE);

// Compare the whole DatasetItem objects, in order - not just their ids.
assertThat(sorted.content())
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsExactlyElementsOf(expected);
assertDatasetItemsInOrder(sorted.content(), expected);

// Page boundary: with size=2, page 2 returns only the trailing item in sort order, exercising the
// push-top-limit OFFSET :top_offset + outer LIMIT path; total stays at the full matching count.
var pageTwo = datasetResourceClient.getDatasetItemsWithExperimentItems(
datasetId, List.of(experimentId), null, null, sorting, 2, 2, API_KEY, TEST_WORKSPACE);
assertThat(pageTwo.total()).isEqualTo(count);
assertThat(pageTwo.content())
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsExactly(expected.get(count - 1));
assertDatasetItemsInOrder(pageTwo.content(), List.of(expected.get(count - 1)));
}

@Test
Expand Down Expand Up @@ -3842,12 +3836,9 @@ void putItems__whenCreatingBatchRepeatsStableId__thenCountedOnce() {
// counter that agrees with it.
assertThat(stored).extracting(DatasetItem::id)
.containsExactlyInAnyOrder(duplicatedId, distinctId);
assertThat(stored)
.filteredOn(item -> distinctId.equals(item.id()))
.singleElement()
.usingRecursiveComparison()
.ignoringFields(IGNORED_FIELDS_DATA_ITEM)
.isEqualTo(distinctItem);
var storedDistinctItems = stored.stream().filter(item -> distinctId.equals(item.id())).toList();
assertThat(storedDistinctItems).hasSize(1);
assertDatasetItem(storedDistinctItems.getFirst(), distinctItem);

// items_total must agree with what is actually stored.
assertThat(version.itemsTotal()).isEqualTo(stored.size());
Expand Down Expand Up @@ -3903,9 +3894,7 @@ void putItems__whenDuplicateStableIdSpansBatchesInGroup__thenCountedOnce() {
var stored = datasetResourceClient.getDatasetItems(
datasetId, 1, 100, version.versionHash(), API_KEY, TEST_WORKSPACE).content();

assertThat(stored)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsExactlyInAnyOrder(updatedShared, otherItem);
assertDatasetItemsInAnyOrder(stored, updatedShared, otherItem);
assertThat(version.itemsTotal()).isEqualTo(stored.size());
}

Expand Down Expand Up @@ -4801,9 +4790,7 @@ void applyChanges__whenEvaluatorAndDescriptionContainJsonEscapeSequences__thenIt
.evaluators(newEvaluators)
.description(newDescription)
.build();
assertThat(v2Items)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsExactly(expectedItem);
assertDatasetItemsInOrder(v2Items, List.of(expectedItem));
}

@Test
Expand Down Expand Up @@ -5372,10 +5359,7 @@ void createItems__whenDescription__thenFieldReturned() {
var expectedItem = items.getFirst().toBuilder()
.id(returnedItem.id())
.build();
assertThat(returnedItem)
.usingRecursiveComparison()
.ignoringFields(IGNORED_FIELDS_DATA_ITEM)
.isEqualTo(expectedItem);
assertDatasetItem(returnedItem, expectedItem);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Comparator;
import java.util.HashMap;
Expand Down Expand Up @@ -181,6 +180,9 @@
import static com.comet.opik.api.resources.utils.TestUtils.getIdFromLocation;
import static com.comet.opik.api.resources.utils.TestUtils.toURLEncodedQueryParam;
import static com.comet.opik.api.resources.utils.WireMockUtils.WireMockRuntime;
import static com.comet.opik.api.resources.utils.datasets.DatasetItemAssertions.assertDatasetItem;
import static com.comet.opik.api.resources.utils.datasets.DatasetItemAssertions.assertDatasetItemsInOrder;
import static com.comet.opik.api.resources.utils.datasets.DatasetItemAssertions.ignoredFieldsPlus;
import static com.comet.opik.api.resources.v1.priv.OptimizationsResourceTest.OPTIMIZATION_IGNORED_FIELDS;
import static com.comet.opik.infrastructure.auth.RequestContext.SESSION_COOKIE;
import static com.comet.opik.infrastructure.auth.RequestContext.WORKSPACE_HEADER;
Expand Down Expand Up @@ -216,8 +218,6 @@ class DatasetsResourceTest {

public static final String[] IGNORED_FIELDS_LIST = {"feedbackScores", "createdAt", "lastUpdatedAt", "createdBy",
"lastUpdatedBy", "comments", "projectName", "traceMetadata"};
public static final String[] IGNORED_FIELDS_DATA_ITEM = {"createdAt", "lastUpdatedAt", "experimentItems",
"createdBy", "lastUpdatedBy", "datasetId", "tags", "datasetItemId", "runSummariesByExperiment"};
public static final String[] DATASET_IGNORED_FIELDS = {"id", "createdAt", "lastUpdatedAt", "createdBy",
"lastUpdatedBy", "projectName", "experimentCount", "mostRecentExperimentAt", "lastCreatedExperimentAt",
"datasetItemsCount", "lastCreatedOptimizationAt", "mostRecentOptimizationAt", "optimizationCount",
Expand Down Expand Up @@ -4862,9 +4862,7 @@ private DatasetItem getItemAndAssert(DatasetItem expectedDatasetItem, String wor
assertThat(actualResponse.getStatusInfo().getStatusCode()).isEqualTo(200);

assertThat(actualEntity.id()).isEqualTo(expectedDatasetItem.id());
assertThat(actualEntity).usingRecursiveComparison()
.ignoringFields(IGNORED_FIELDS_DATA_ITEM)
.isEqualTo(expectedDatasetItem);
assertDatasetItem(actualEntity, expectedDatasetItem);

assertThat(actualEntity.createdAt()).isInThePast();
assertThat(actualEntity.lastUpdatedAt()).isInThePast();
Expand Down Expand Up @@ -6594,11 +6592,8 @@ private void assertDatasetItemPage(DatasetItemPage actualPage, List<DatasetItem>

private void assertPage(List<DatasetItem> expectedItems, List<DatasetItem> actualItems) {

List<String> ignoredFields = new ArrayList<>(Arrays.asList(IGNORED_FIELDS_DATA_ITEM));
ignoredFields.add("data");

assertThat(actualItems)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(ignoredFields.toArray(String[]::new))
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(ignoredFieldsPlus("data"))
.isEqualTo(expectedItems);

assertThat(actualItems).hasSize(expectedItems.size());
Expand Down Expand Up @@ -9068,9 +9063,7 @@ private void assertSortedByKey(UUID datasetId, String experimentIdsParam, String

// Compare the whole DatasetItem objects, in order - not just their ids - so the assertion proves
// the bound key actually drives the ordering. Volatile/derived fields are ignored per suite convention.
assertThat(actualItems)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsExactlyElementsOf(expectedItems);
assertDatasetItemsInOrder(actualItems, expectedItems);
}

private List<DatasetItem> fetchDatasetItems(UUID datasetId, String experimentIdsParam, String sortField,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@
import java.util.stream.Stream;

import static com.comet.opik.api.resources.utils.ClickHouseContainerUtils.DATABASE_NAME;
import static com.comet.opik.api.resources.utils.datasets.DatasetItemAssertions.assertDatasetItems;
import static org.assertj.core.api.Assertions.assertThat;

@TestInstance(TestInstance.Lifecycle.PER_CLASS)
Expand Down Expand Up @@ -127,9 +128,6 @@ class ExperimentAggregatesIntegrationTest {
private final MySQLContainer MYSQL = MySQLContainerUtils.newMySQLContainer();
private final RandomGenerator random = new Random();

public static final String[] IGNORED_FIELDS_DATA_ITEM = {"createdAt", "lastUpdatedAt", "experimentItems",
"createdBy", "lastUpdatedBy", "datasetId", "tags", "datasetItemId"};

public static final String[] IGNORED_FIELDS_EXPERIMENT_ITEM = {"createdAt", "lastUpdatedAt", "createdBy",
"lastUpdatedBy", "comments", "projectName", "executionPolicy"};

Expand Down Expand Up @@ -1506,11 +1504,15 @@ void getDatasetItemsWithExperimentItemsFromAggregates(
void assertDatasetItemsWithExperimentItems(List<DatasetItem> expectedDatasetItem,
List<DatasetItem> actualDatasetItems) {

assertThat(actualDatasetItems)
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.isEqualTo(expectedDatasetItem);
assertDatasetItems(actualDatasetItems, expectedDatasetItem);

for (var i = 0; i < actualDatasetItems.size(); i++) {
// The shared helper ignores runSummariesByExperiment, but parity between the two read paths is
// exactly what this test exists to prove: the aggregates query omits assertions_array, so a
// divergence here means the aggregates path silently lost its assertion summaries.
assertThat(actualDatasetItems.get(i).runSummariesByExperiment())
.isEqualTo(expectedDatasetItem.get(i).runSummariesByExperiment());

var actualExperiments = actualDatasetItems.get(i).experimentItems();
var expectedExperiments = expectedDatasetItem.get(i).experimentItems();

Expand Down
Loading