[DAR-4041][External] Add multi-file E2E tests #931

JBWilkie · 2024-09-26T14:31:21Z

Problem

No E2E tests for:

Pushing multi-file items
Importing annotations to multi-file items

Solution

Add the following E2E tests:
push:

test_push_multi_slotted_item: Upload a multi-slotted item via the CLI
test_push_multi_channel_item: Upload a multi-channel item via the CLI
test_push_dicom_series: Upload a multi-file DICOM series via the CLI

import:

test_import_annotations_to_multi_slotted_item_without_slots_defined: Upload annotations to a multi-slotted item without aligning each annotation to a slot. All annotations should end up in the item's first slot
test_import_annotations_to_multi_slotted_item_with_slots_defined: Upload annotations to a multi-slotted item where each annotation is aligned with a particular slot. Each annotation should end up in the correct slot
test_import_annotations_to_multi_channel_item_without_slots_defined: Upload annotations to a multi-channel item without aligning each annotation to a slot. All annotations should end up the base slot
test_import_annotations_to_multi_channel_item_with_slots_defined: Upload annotations to a multi-channel item where each annotation is aligned with the base slot. All annotations should end up in the base slot
test_import_annotations_to_multi_channel_item_non_base_slot: Upload annotations to a multi-channel item where each annotation is aligned with a non-base slot. The importer should throw an error

Changelog

Added E2E test coverage for uploading & importing annotations to multi-file items

linear · 2024-09-26T14:31:28Z

DAR-4041 E2E tests for multi-file `push` and `import`

umbertoDifa

These tests are 🔥 , I just left minor comments

umbertoDifa · 2024-10-08T08:41:03Z

e2e_tests/cli/test_import.py

+        config_values.team_slug,
+        config_values.server,
+    )
+    return items[item_idx]["slots"][0]["slot_name"]


This might not be an issue now, but what if the BE decided to return the list of items in a different order? Wouldn't it be safer to find an item by item_id or name?

umbertoDifa · 2024-10-08T08:41:40Z

e2e_tests/cli/test_import.py

            assert expected_property in actual_properties  # type : ignore


+def get_base_slot_of_item(


get_base_slot_name_of_item it returns the name not the slot right?

umbertoDifa · 2024-10-08T08:44:54Z

e2e_tests/cli/test_import.py

+        if expected_annotation.slot_names:
+            assert expected_annotation.slot_names == actual_annotation.slot_names
+        else:
+            assert actual_annotation.slot_names == [base_slot]


Trying to think when we have this case: if it's a multislot and I don't specify the slot_name of an annotation then by default it gets uploaded to the base slot?

umbertoDifa · 2024-10-08T08:46:15Z

e2e_tests/cli/test_import.py

-            assert_same_annotation_data(actual_annotation, expected_annotation)
+            assert_same_annotation_data(expected_annotation, actual_annotation)
            assert_same_annotation_properties(expected_annotation, actual_annotation)
+            assert_annotation_slot_alignment(


nit:I'd probably expect something named assert_same_annotation_slot_name?

umbertoDifa · 2024-10-08T08:52:27Z

e2e_tests/cli/test_import.py

+def test_import_annotations_to_multi_slotted_item_without_slots_defined(
+    local_dataset: E2EDataset, config_values: ConfigValues
+) -> None:
+    """
+    Upload annotations to a multi-slotted item without aligning each annotation to a
+    slot. All annotations should end up in the item's first slot
+    """
+    item_type = "multi_slotted"
+    local_dataset.register_read_only_items(config_values, item_type)
+    expected_annotations_dir = (
+        Path(__file__).parents[1]
+        / "data"
+        / "import"
+        / "multi_slotted_annotations_without_slots_defined"
+    )
+    result = run_cli_command(
+        f"darwin dataset import {local_dataset.name} darwin {expected_annotations_dir}"
+    )
+    assert_cli(result, 0)
+    base_slot = get_base_slot_of_item(config_values, local_dataset.id, item_idx=0)
+    with tempfile.TemporaryDirectory() as tmp_dir_str:
+        actual_annotations_dir = Path(tmp_dir_str)
+        export_and_download_annotations(
+            actual_annotations_dir, local_dataset, config_values
+        )
+        compare_annotations_export(
+            actual_annotations_dir, expected_annotations_dir, item_type, base_slot
+        )
+
+
+def test_import_annotations_to_multi_slotted_item_with_slots_defined(
+    local_dataset: E2EDataset, config_values: ConfigValues
+) -> None:
+    """
+    Upload annotations to a multi-slotted item where each annotation is aligned with a
+    particular slot. Each annotation should end up in the correct slot
+    """
+    item_type = "multi_slotted"
+    local_dataset.register_read_only_items(config_values, item_type)
+    expected_annotations_dir = (
+        Path(__file__).parents[1]
+        / "data"
+        / "import"
+        / "multi_slotted_annotations_with_slots_defined"
+    )
+    result = run_cli_command(
+        f"darwin dataset import {local_dataset.name} darwin {expected_annotations_dir}"
+    )
+    assert_cli(result, 0)
+    base_slot = get_base_slot_of_item(config_values, local_dataset.id, item_idx=0)
+    with tempfile.TemporaryDirectory() as tmp_dir_str:
+        actual_annotations_dir = Path(tmp_dir_str)
+        export_and_download_annotations(
+            actual_annotations_dir, local_dataset, config_values
+        )
+        compare_annotations_export(
+            actual_annotations_dir, expected_annotations_dir, item_type, base_slot
+        )
+
+
+def test_import_annotations_to_multi_channel_item_without_slots_defined(
+    local_dataset: E2EDataset, config_values: ConfigValues
+) -> None:
+    """
+    Upload annotations to a multi-channel item without aligning each annotation to a
+    slot. All annotations should end up the base slot
+    """
+    item_type = "multi_channel"
+    local_dataset.register_read_only_items(config_values, item_type)
+    expected_annotations_dir = (
+        Path(__file__).parents[1]
+        / "data"
+        / "import"
+        / "multi_channel_annotations_without_slots_defined"
+    )
+    result = run_cli_command(
+        f"darwin dataset import {local_dataset.name} darwin {expected_annotations_dir}"
+    )
+    assert_cli(result, 0)
+    base_slot = get_base_slot_of_item(config_values, local_dataset.id, item_idx=0)
+    with tempfile.TemporaryDirectory() as tmp_dir_str:
+        actual_annotations_dir = Path(tmp_dir_str)
+        export_and_download_annotations(
+            actual_annotations_dir, local_dataset, config_values
+        )
+        compare_annotations_export(
+            actual_annotations_dir, expected_annotations_dir, item_type, base_slot
+        )
+
+
+def test_import_annotations_to_multi_channel_item_with_slots_defined(
+    local_dataset: E2EDataset, config_values: ConfigValues
+) -> None:
+    """
+    Upload annotations to a multi-channel item where each annotation is aligned with
+    the base slot. Each annotation should end up in the base slot
+    """
+    item_type = "multi_channel"
+    local_dataset.register_read_only_items(config_values, item_type="multi_channel")
+    expected_annotations_dir = (
+        Path(__file__).parents[1]
+        / "data"
+        / "import"
+        / "multi_channel_annotations_with_slots_defined"
+    )
+    result = run_cli_command(
+        f"darwin dataset import {local_dataset.name} darwin {expected_annotations_dir}"
+    )
+    assert_cli(result, 0)
+    base_slot = get_base_slot_of_item(config_values, local_dataset.id, item_idx=0)
+    with tempfile.TemporaryDirectory() as tmp_dir_str:
+        actual_annotations_dir = Path(tmp_dir_str)
+        export_and_download_annotations(
+            actual_annotations_dir, local_dataset, config_values
+        )
+        compare_annotations_export(
+            actual_annotations_dir, expected_annotations_dir, item_type, base_slot
+        )


it looks like these methods are identical and only the expected_annotations_dir changes. I wonder if it can be useful to simplify. The rule of thumb is: if you repeat it 3 times than you can DRY the code. Up to you

DRYing the code would make it way faster to read and understand

umbertoDifa · 2024-10-08T08:53:30Z

e2e_tests/cli/test_import.py

+    local_dataset: E2EDataset, config_values: ConfigValues
+) -> None:
+    """
+    Upload annotations to a multi-channel item where each annotation is aligned with a


What happens if I have at least one annotation that is aligned to a base_slot? Would that be the only one imported?

No - If there is any annotation that is not aligned with the base slot, the importer will throw an error

AndriiKlymchuk

Nothing critical. Just consider to address existing comments before merge.

AndriiKlymchuk · 2024-10-08T11:20:18Z

e2e_tests/cli/test_push.py

+    with tempfile.TemporaryDirectory() as tmp_dir_str:
+        tmp_dir = Path(tmp_dir_str)
+        with zipfile.ZipFile(push_dir) as z:
+            z.extractall(tmp_dir)
+            result = run_cli_command(
+                f"darwin dataset push {local_dataset.name} {tmp_dir}/flat_directory_of_2_dicom_files --item-merge-mode {merge_mode}"
+            )
+            assert_cli(result, 0)
+            wait_until_items_processed(config_values, local_dataset.id)
+            items = list_items(
+                config_values.api_key,
+                local_dataset.id,
+                config_values.team_slug,
+                config_values.server,
+            )


The logic here repeats in every test. Maybe you can de-duplicate it. At least it is easier to dot it from line 261

JBWilkie · 2024-10-08T11:24:51Z

These tests are 🔥 , I just left minor comments

Useful points, thank you! All addressed

JBWilkie · 2024-10-08T12:00:09Z

Nothing critical. Just consider to address existing comments before merge.

All addressed, thank you!

Added multi-file E2E tests

49dd277

Import E2E tests for multi-file items

b7349ec

JBWilkie force-pushed the DAR-4041 branch from 8a6ff20 to b7349ec Compare September 27, 2024 00:36

JBWilkie requested review from AndriiKlymchuk and dorfmanrobert October 7, 2024 10:40

umbertoDifa reviewed Oct 8, 2024

View reviewed changes

AndriiKlymchuk approved these changes Oct 8, 2024

View reviewed changes

AndriiKlymchuk reviewed Oct 8, 2024

View reviewed changes

Style improvements

89b1ece

JBWilkie force-pushed the DAR-4041 branch from 6cee7f2 to 89b1ece Compare October 8, 2024 11:58

JBWilkie merged commit 25aff99 into master Oct 8, 2024

		assert expected_property in actual_properties # type : ignore


		def get_base_slot_of_item(

[DAR-4041][External] Add multi-file E2E tests #931

[DAR-4041][External] Add multi-file E2E tests #931

Uh oh!

Conversation

JBWilkie commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Changelog

Uh oh!

linear bot commented Sep 26, 2024

Uh oh!

umbertoDifa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndriiKlymchuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JBWilkie commented Oct 8, 2024

Uh oh!

JBWilkie commented Oct 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JBWilkie commented Sep 26, 2024 •

edited

Loading