diff --git a/src/allotropy/testing/utils.py b/src/allotropy/testing/utils.py index bc8853d1a3..db50218178 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,67 @@ 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}':\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}" + ) + 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..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,6 +77,7 @@ def test_json_to_csv_dataset( expected_results: dict[str, str], *, 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 044d84485b..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, 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" @@ -61,8 +66,12 @@ 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, write_actual_to_expected_on_fail=overwrite, + force_overwrite=force_overwrite, )