Skip to content

Commit ce3e7bb

Browse files
test(datasets): assert the surviving rows, not just how many
Both OPIK-7891 tests checked stored.size() and left the row identities unasserted. On a de-duplication test that is the weakest possible check: the count is exactly what a bug is most likely to keep right. Keeping the wrong revision of the duplicate, or dropping the distinct row and keeping both duplicates, would each leave the size at 2 and pass. Now both name the rows that must survive and compare whole objects via the shared IGNORED_FIELDS_DATA_ITEM element comparator, so a wrong-winner bug fails. This also pins behaviour the tests previously only assumed: the last submitted revision of a repeated id is the one that survives. The itemsTotal assertion still ties the counter to stored.size() rather than a literal, which is only meaningful now that stored itself is pinned. Addresses review feedback on #7966. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 10a4d68 commit ce3e7bb

1 file changed

Lines changed: 47 additions & 23 deletions

File tree

apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/DatasetVersionResourceTest.java

Lines changed: 47 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -3568,22 +3568,31 @@ void putItems__whenCreatingBatchRepeatsStableId__thenCountedOnce() {
35683568
// (ClickHouse async inserts report 0 before commit). A stable id repeated inside that
35693569
// first batch is therefore counted twice, while ClickHouse collapses it to one row.
35703570
var duplicatedId = TestIdGeneratorFactory.create().generateId();
3571+
var distinctId = TestIdGeneratorFactory.create().generateId();
3572+
3573+
// The repeated id wins with its LAST submitted content: ClickHouse keeps one row per
3574+
// dataset_item_id and reads take the newest, so "second" survives and "first" does not.
3575+
var winningDuplicate = DatasetItem.builder()
3576+
.id(duplicatedId)
3577+
.datasetItemId(duplicatedId)
3578+
.source(DatasetItemSource.SDK)
3579+
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"second\"")))
3580+
.build();
3581+
var distinctItem = DatasetItem.builder()
3582+
.id(distinctId)
3583+
.datasetItemId(distinctId)
3584+
.source(DatasetItemSource.SDK)
3585+
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"third\"")))
3586+
.build();
3587+
35713588
var items = List.of(
35723589
DatasetItem.builder()
35733590
.id(duplicatedId)
35743591
.source(DatasetItemSource.SDK)
35753592
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"first\"")))
35763593
.build(),
3577-
DatasetItem.builder()
3578-
.id(duplicatedId)
3579-
.source(DatasetItemSource.SDK)
3580-
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"second\"")))
3581-
.build(),
3582-
DatasetItem.builder()
3583-
.id(TestIdGeneratorFactory.create().generateId())
3584-
.source(DatasetItemSource.SDK)
3585-
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"third\"")))
3586-
.build());
3594+
winningDuplicate,
3595+
distinctItem);
35873596

35883597
datasetResourceClient.createDatasetItems(DatasetItemBatch.builder()
35893598
.datasetId(datasetId)
@@ -3593,10 +3602,14 @@ void putItems__whenCreatingBatchRepeatsStableId__thenCountedOnce() {
35933602

35943603
var version = getLatestVersion(datasetId);
35953604

3596-
// Two distinct ids went in, so the version holds two rows.
3605+
// Assert on the rows themselves, not just how many: a bug that kept the wrong revision
3606+
// of the duplicate, or dropped the distinct item and kept both duplicates, would leave
3607+
// the count at 2 and slip through a size-only check.
35973608
var stored = datasetResourceClient.getDatasetItems(
35983609
datasetId, 1, 100, version.versionHash(), API_KEY, TEST_WORKSPACE).content();
3599-
assertThat(stored).hasSize(2);
3610+
assertThat(stored)
3611+
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
3612+
.containsExactlyInAnyOrder(winningDuplicate, distinctItem);
36003613

36013614
// items_total must agree with what is actually stored.
36023615
assertThat(version.itemsTotal()).isEqualTo(stored.size());
@@ -3612,6 +3625,14 @@ void putItems__whenDuplicateStableIdSpansBatchesInGroup__thenCountedOnce() {
36123625
// one version. The first batch creates it, later batches append. A stable id present in
36133626
// both must contribute exactly one item to the total.
36143627
var sharedId = TestIdGeneratorFactory.create().generateId();
3628+
var otherId = TestIdGeneratorFactory.create().generateId();
3629+
3630+
var otherItem = DatasetItem.builder()
3631+
.id(otherId)
3632+
.datasetItemId(otherId)
3633+
.source(DatasetItemSource.SDK)
3634+
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"other\"")))
3635+
.build();
36153636

36163637
datasetResourceClient.createDatasetItems(DatasetItemBatch.builder()
36173638
.datasetId(datasetId)
@@ -3622,28 +3643,31 @@ void putItems__whenDuplicateStableIdSpansBatchesInGroup__thenCountedOnce() {
36223643
.source(DatasetItemSource.SDK)
36233644
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"first\"")))
36243645
.build(),
3625-
DatasetItem.builder()
3626-
.id(TestIdGeneratorFactory.create().generateId())
3627-
.source(DatasetItemSource.SDK)
3628-
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"other\"")))
3629-
.build()))
3646+
otherItem))
36303647
.build(), TEST_WORKSPACE, API_KEY);
36313648

3649+
// The second batch re-sends the shared id with new content, so it is an update:
3650+
// one row, holding the later revision.
3651+
var updatedShared = DatasetItem.builder()
3652+
.id(sharedId)
3653+
.datasetItemId(sharedId)
3654+
.source(DatasetItemSource.SDK)
3655+
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"updated\"")))
3656+
.build();
3657+
36323658
datasetResourceClient.createDatasetItems(DatasetItemBatch.builder()
36333659
.datasetId(datasetId)
36343660
.batchGroupId(batchGroupId)
3635-
.items(List.of(DatasetItem.builder()
3636-
.id(sharedId)
3637-
.source(DatasetItemSource.SDK)
3638-
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"updated\"")))
3639-
.build()))
3661+
.items(List.of(updatedShared))
36403662
.build(), TEST_WORKSPACE, API_KEY);
36413663

36423664
var version = getLatestVersion(datasetId);
36433665
var stored = datasetResourceClient.getDatasetItems(
36443666
datasetId, 1, 100, version.versionHash(), API_KEY, TEST_WORKSPACE).content();
36453667

3646-
assertThat(stored).hasSize(2);
3668+
assertThat(stored)
3669+
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
3670+
.containsExactlyInAnyOrder(updatedShared, otherItem);
36473671
assertThat(version.itemsTotal()).isEqualTo(stored.size());
36483672
}
36493673

0 commit comments

Comments
 (0)