From 34b982de3b6643f94ef872c42e985b7280f85d68 Mon Sep 17 00:00:00 2001 From: Joshua Hernandez Date: Thu, 23 Oct 2025 17:13:03 -0500 Subject: [PATCH 1/4] add util to overwrite data safely by warning developer when fields are to be removed from final ASM. --- src/allotropy/testing/utils.py | 93 ++++++++++++++++++- tests/conftest.py | 10 ++ tests/json_to_csv/json_to_csv_dataset_test.py | 1 + tests/to_allotrope_test.py | 3 +- 4 files changed, 101 insertions(+), 6 deletions(-) diff --git a/src/allotropy/testing/utils.py b/src/allotropy/testing/utils.py index bc8853d1a3..a4b3d69cac 100644 --- a/src/allotropy/testing/utils.py +++ b/src/allotropy/testing/utils.py @@ -10,6 +10,7 @@ import tempfile from typing import Any from unittest import mock +import warnings from deepdiff import DeepDiff from deepdiff.model import DiffLevel @@ -184,6 +185,42 @@ def _assert_allotrope_dicts_equal( raise AssertionError(msg) +def _check_allotrope_dicts_for_deletions( + expected: DictType, + actual: DictType, +) -> tuple[bool, list[str]]: + """ + Check if the actual dict has any deleted fields compared to expected dict. + + Returns: + tuple[bool, list[str]]: (has_deletions, deleted_paths) + - has_deletions: True if any fields were deleted + - deleted_paths: List of JSON paths for deleted fields + """ + expected_replaced = _replace_asm_converter_version(expected) + ddiff = DeepDiff( + expected_replaced, + actual, + ignore_type_in_groups=[(float, np.float64)], + ignore_nan_inequality=True, + custom_operators=[DEEPDIFF_PATH_COMPARATOR], + ) + + deleted_paths = [] + if ddiff: + # Check for dictionary item removals + if "dictionary_item_removed" in ddiff: + for path in ddiff["dictionary_item_removed"]: + deleted_paths.append(str(path)) + + # Check for iterable item removals (list items) + if "iterable_item_removed" in ddiff: + for path in ddiff["iterable_item_removed"]: + deleted_paths.append(str(path)) + + return len(deleted_paths) > 0, deleted_paths + + class TestIdGenerator: next_id: int prefix: str | None @@ -251,6 +288,7 @@ def validate_contents( allotrope_dict: DictType, expected_file: Path | str, write_actual_to_expected_on_fail: bool = False, # noqa: FBT001, FBT002 + force_overwrite: bool = False, # noqa: FBT001, FBT002 ) -> None: """Use the newly created allotrope_dict to validate the contents inside expected_file.""" # Ensure that allotrope_dict can be written via json.dump() @@ -260,16 +298,61 @@ def validate_contents( try: with open(expected_file, encoding=DEFAULT_ENCODING) as f: expected_dict = json.load(f) - _assert_allotrope_dicts_equal(expected_dict, allotrope_dict) - except Exception as e: + + # Check for deletions before doing the assertion if we're going to overwrite if write_actual_to_expected_on_fail: + has_deletions, deleted_paths = _check_allotrope_dicts_for_deletions(expected_dict, allotrope_dict) + + if has_deletions and not force_overwrite: + # Show a summary of deleted fields instead of the full list + if len(deleted_paths) <= 10: + # Show all paths if there are few + deleted_paths_str = "\n".join(f" - {path}" for path in deleted_paths) + summary_msg = f"Fields that will be deleted:\n{deleted_paths_str}" + else: + # Show first few and summary for many deletions + first_few = "\n".join(f" - {path}" for path in deleted_paths[:5]) + summary_msg = ( + f"Fields that will be deleted (showing first 5 of {len(deleted_paths)}):\n" + f"{first_few}\n" + f" ... and {len(deleted_paths) - 5} more fields" + ) + + error_msg = ( + f"DELETION DETECTED: Cannot overwrite test data for '{expected_file}'\n" + f"Total fields to be deleted: {len(deleted_paths)}\n\n" + f"{summary_msg}\n\n" + f"To proceed anyway, use the --force-overwrite flag.\n\n" + f"If you use --force-overwrite, the full list will be provided for your PR description." + ) + raise AssertionError(error_msg) + elif has_deletions and force_overwrite: + # Issue warning about deletions for PR description + deleted_paths_str = "\n".join(f" - {path}" for path in deleted_paths) + warning_msg = ( + f"Fields will be deleted from '{expected_file}': {deleted_paths_str}. " + f"Please copy the following information into your PR description:\n" + f"**Fields deleted in test data:**\n" + f"{deleted_paths_str}" + ) + warnings.warn(warning_msg, UserWarning, stacklevel=2) + + # Only do the assertion if we're not force overwriting + if not (write_actual_to_expected_on_fail and force_overwrite): + _assert_allotrope_dicts_equal(expected_dict, allotrope_dict) + elif write_actual_to_expected_on_fail and force_overwrite: + # Force overwrite - write the file and continue _write_actual_to_expected(allotrope_dict, expected_file) + + except Exception as e: + if write_actual_to_expected_on_fail: if isinstance(e, FileNotFoundError): + # File doesn't exist, safe to create + _write_actual_to_expected(allotrope_dict, expected_file) msg = f"Missing expected output file '{expected_file}', writing expected output because 'write_actual_to_expected_on_fail=True'" raise AssertionError(msg) from e - if isinstance(e, AssertionError) and "allotropy output != expected:" in str( - e - ): + elif isinstance(e, AssertionError) and "allotropy output != expected:" in str(e): + _write_actual_to_expected(allotrope_dict, expected_file) msg = f"Mismatch between actual and expected for '{expected_file}', writing expected output because 'write_actual_to_expected_on_fail=True'\n\n{e}" raise AssertionError(msg) from e raise diff --git a/tests/conftest.py b/tests/conftest.py index c01e21d1a8..e7eca5a351 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,6 +20,11 @@ def pytest_addoption(parser: Parser) -> None: action="store_true", help="If set, overwrite failing tests with new data.", ) + parser.addoption( + "--force-overwrite", + action="store_true", + help="If set, allow overwriting test data even when fields are deleted. Use with caution!", + ) parser.addoption( "--exclude", action="store", @@ -44,6 +49,11 @@ def overwrite(request: FixtureRequest) -> Any: return request.config.getoption("--overwrite") +@pytest.fixture +def force_overwrite(request: FixtureRequest) -> Any: + return request.config.getoption("--force-overwrite") + + @pytest.fixture def warn_unread_keys(request: FixtureRequest) -> Any: return request.config.getoption("--warn_unread_keys") diff --git a/tests/json_to_csv/json_to_csv_dataset_test.py b/tests/json_to_csv/json_to_csv_dataset_test.py index ca32f22ad8..4778e49464 100644 --- a/tests/json_to_csv/json_to_csv_dataset_test.py +++ b/tests/json_to_csv/json_to_csv_dataset_test.py @@ -77,6 +77,7 @@ def test_json_to_csv_dataset( expected_results: dict[str, str], *, overwrite: bool, + force_overwrite: bool, ) -> None: with open(input_file) as infile: input_json = json.load(infile) diff --git a/tests/to_allotrope_test.py b/tests/to_allotrope_test.py index 044d84485b..c00586bfca 100644 --- a/tests/to_allotrope_test.py +++ b/tests/to_allotrope_test.py @@ -44,7 +44,7 @@ class ParserTest: # test_file_path is automatically populated with all files in testdata folder next to the test file. def test_positive_cases( - self, test_file_path: Path, *, overwrite: bool, warn_unread_keys: bool + self, test_file_path: Path, *, overwrite: bool, force_overwrite: bool, warn_unread_keys: bool ) -> None: if warn_unread_keys: os.environ["WARN_UNUSED_KEYS"] = "1" @@ -65,4 +65,5 @@ def test_positive_cases( allotrope_dict, expected_filepath, write_actual_to_expected_on_fail=overwrite, + force_overwrite=force_overwrite, ) From 6821a6a3bad992499c70f423e7a0a5b0e446a5c0 Mon Sep 17 00:00:00 2001 From: Joshua Hernandez Date: Thu, 23 Oct 2025 17:19:15 -0500 Subject: [PATCH 2/4] fix error when force overwrite not overwriting files --- src/main.py | 7 +++++++ tests/to_allotrope_test.py | 3 +++ 2 files changed, 10 insertions(+) create mode 100644 src/main.py diff --git a/src/main.py b/src/main.py new file mode 100644 index 0000000000..fd421ef1d8 --- /dev/null +++ b/src/main.py @@ -0,0 +1,7 @@ +from allotropy.parser_factory import Vendor +from allotropy.testing.utils import from_file + +file_name = "multi-plate_example01.xlsx" + +test_filepath = f"../tests/parsers/thermo_skanit/testdata/{file_name}" +allotrope_dict = from_file(test_filepath, Vendor.THERMO_SKANIT) diff --git a/tests/to_allotrope_test.py b/tests/to_allotrope_test.py index c00586bfca..afe7f9edb8 100644 --- a/tests/to_allotrope_test.py +++ b/tests/to_allotrope_test.py @@ -61,6 +61,9 @@ def test_positive_cases( ) # If expected output does not exist, assume this is a new file and write it. overwrite = overwrite or not expected_filepath.exists() + # Force overwrite should always allow overwriting + if force_overwrite: + overwrite = True validate_contents( allotrope_dict, expected_filepath, From feb6be65f2d7cc3e86c222b0a1f42e061144cc10 Mon Sep 17 00:00:00 2001 From: Joshua Hernandez Date: Thu, 23 Oct 2025 19:45:39 -0500 Subject: [PATCH 3/4] lint, fix test with overwrite flag --- src/allotropy/testing/utils.py | 12 +++++++++--- src/main.py | 7 ------- tests/json_to_csv/json_to_csv_dataset_test.py | 2 +- .../to_allotrope_test.py | 6 +++++- tests/to_allotrope_test.py | 7 ++++++- 5 files changed, 21 insertions(+), 13 deletions(-) delete mode 100644 src/main.py diff --git a/src/allotropy/testing/utils.py b/src/allotropy/testing/utils.py index a4b3d69cac..354e80197a 100644 --- a/src/allotropy/testing/utils.py +++ b/src/allotropy/testing/utils.py @@ -301,13 +301,17 @@ def validate_contents( # Check for deletions before doing the assertion if we're going to overwrite if write_actual_to_expected_on_fail: - has_deletions, deleted_paths = _check_allotrope_dicts_for_deletions(expected_dict, allotrope_dict) + has_deletions, deleted_paths = _check_allotrope_dicts_for_deletions( + expected_dict, allotrope_dict + ) if has_deletions and not force_overwrite: # Show a summary of deleted fields instead of the full list if len(deleted_paths) <= 10: # Show all paths if there are few - deleted_paths_str = "\n".join(f" - {path}" for path in deleted_paths) + deleted_paths_str = "\n".join( + f" - {path}" for path in deleted_paths + ) summary_msg = f"Fields that will be deleted:\n{deleted_paths_str}" else: # Show first few and summary for many deletions @@ -351,7 +355,9 @@ def validate_contents( _write_actual_to_expected(allotrope_dict, expected_file) msg = f"Missing expected output file '{expected_file}', writing expected output because 'write_actual_to_expected_on_fail=True'" raise AssertionError(msg) from e - elif isinstance(e, AssertionError) and "allotropy output != expected:" in str(e): + elif isinstance( + e, AssertionError + ) and "allotropy output != expected:" in str(e): _write_actual_to_expected(allotrope_dict, expected_file) msg = f"Mismatch between actual and expected for '{expected_file}', writing expected output because 'write_actual_to_expected_on_fail=True'\n\n{e}" raise AssertionError(msg) from e diff --git a/src/main.py b/src/main.py deleted file mode 100644 index fd421ef1d8..0000000000 --- a/src/main.py +++ /dev/null @@ -1,7 +0,0 @@ -from allotropy.parser_factory import Vendor -from allotropy.testing.utils import from_file - -file_name = "multi-plate_example01.xlsx" - -test_filepath = f"../tests/parsers/thermo_skanit/testdata/{file_name}" -allotrope_dict = from_file(test_filepath, Vendor.THERMO_SKANIT) diff --git a/tests/json_to_csv/json_to_csv_dataset_test.py b/tests/json_to_csv/json_to_csv_dataset_test.py index 4778e49464..2af121a96b 100644 --- a/tests/json_to_csv/json_to_csv_dataset_test.py +++ b/tests/json_to_csv/json_to_csv_dataset_test.py @@ -77,7 +77,7 @@ def test_json_to_csv_dataset( expected_results: dict[str, str], *, overwrite: bool, - force_overwrite: bool, + force_overwrite: bool, # noqa: ARG001 ) -> None: with open(input_file) as infile: input_json = json.load(infile) diff --git a/tests/parsers/cytiva_biacore_t200_evaluation/to_allotrope_test.py b/tests/parsers/cytiva_biacore_t200_evaluation/to_allotrope_test.py index 9611259db5..929507b003 100644 --- a/tests/parsers/cytiva_biacore_t200_evaluation/to_allotrope_test.py +++ b/tests/parsers/cytiva_biacore_t200_evaluation/to_allotrope_test.py @@ -130,8 +130,12 @@ def test_positive_cases( *, overwrite: bool, warn_unread_keys: bool, + force_overwrite: bool, ) -> None: """Override the test method with data masking, cycle reduction, and sensorgram data reduction patches.""" return super().test_positive_cases( - test_file_path, overwrite=overwrite, warn_unread_keys=warn_unread_keys + test_file_path, + overwrite=overwrite, + warn_unread_keys=warn_unread_keys, + force_overwrite=force_overwrite, ) diff --git a/tests/to_allotrope_test.py b/tests/to_allotrope_test.py index afe7f9edb8..fbc2c135bb 100644 --- a/tests/to_allotrope_test.py +++ b/tests/to_allotrope_test.py @@ -44,7 +44,12 @@ class ParserTest: # test_file_path is automatically populated with all files in testdata folder next to the test file. def test_positive_cases( - self, test_file_path: Path, *, overwrite: bool, force_overwrite: bool, warn_unread_keys: bool + self, + test_file_path: Path, + *, + overwrite: bool, + force_overwrite: bool, + warn_unread_keys: bool, ) -> None: if warn_unread_keys: os.environ["WARN_UNUSED_KEYS"] = "1" From a92b73a971e8fa8e898efc0ef1607c2cca0ef027 Mon Sep 17 00:00:00 2001 From: Joshua Hernandez Date: Mon, 27 Oct 2025 14:08:40 -0500 Subject: [PATCH 4/4] delete second print on removed paths, add line jumps for better readability --- src/allotropy/testing/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allotropy/testing/utils.py b/src/allotropy/testing/utils.py index 354e80197a..db50218178 100644 --- a/src/allotropy/testing/utils.py +++ b/src/allotropy/testing/utils.py @@ -334,8 +334,8 @@ def validate_contents( # Issue warning about deletions for PR description deleted_paths_str = "\n".join(f" - {path}" for path in deleted_paths) warning_msg = ( - f"Fields will be deleted from '{expected_file}': {deleted_paths_str}. " - f"Please copy the following information into your PR description:\n" + f"Fields will be deleted from '{expected_file}':\n\n" + f"Please copy the following information into your PR description:\n\n" f"**Fields deleted in test data:**\n" f"{deleted_paths_str}" )