[DAR-4041][External] Add multi-file E2E tests #931
Conversation
e2e_tests/cli/test_import.py
Outdated
| config_values.team_slug, | ||
| config_values.server, | ||
| ) | ||
| return items[item_idx]["slots"][0]["slot_name"] |
There was a problem hiding this comment.
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?
e2e_tests/cli/test_import.py
Outdated
| @@ -78,15 +80,55 @@ def assert_same_annotation_properties( | |||
| assert expected_property in actual_properties # type : ignore | |||
|
|
|||
|
|
|||
| def get_base_slot_of_item( | |||
There was a problem hiding this comment.
get_base_slot_name_of_item it returns the name not the slot right?
| if expected_annotation.slot_names: | ||
| assert expected_annotation.slot_names == actual_annotation.slot_names | ||
| else: | ||
| assert actual_annotation.slot_names == [base_slot] |
There was a problem hiding this comment.
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?
e2e_tests/cli/test_import.py
Outdated
| assert_same_annotation_properties(expected_annotation, actual_annotation) | ||
| assert_annotation_slot_alignment( |
There was a problem hiding this comment.
nit:I'd probably expect something named assert_same_annotation_slot_name?
e2e_tests/cli/test_import.py
Outdated
| 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 | ||
| ) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
DRYing the code would make it way faster to read and understand
e2e_tests/cli/test_import.py
Outdated
| local_dataset: E2EDataset, config_values: ConfigValues | ||
| ) -> None: | ||
| """ | ||
| Upload annotations to a multi-channel item where each annotation is aligned with a |
There was a problem hiding this comment.
What happens if I have at least one annotation that is aligned to a base_slot? Would that be the only one imported?
There was a problem hiding this comment.
No - If there is any annotation that is not aligned with the base slot, the importer will throw an error
e2e_tests/cli/test_push.py
Outdated
| 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, | ||
| ) |
There was a problem hiding this comment.
The logic here repeats in every test. Maybe you can de-duplicate it. At least it is easier to dot it from line 261
Useful points, thank you! All addressed |
All addressed, thank you! |
Problem
No E2E tests for:
Solution
Add the following E2E tests:
push:test_push_multi_slotted_item: Upload a multi-slotted item via the CLItest_push_multi_channel_item: Upload a multi-channel item via the CLItest_push_dicom_series: Upload a multi-file DICOM series via the CLIimport: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 slottest_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 slottest_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 slottest_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 slottest_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 errorChangelog
Added E2E test coverage for uploading & importing annotations to multi-file items