Skip to content

Commit 10a0683

Browse files
chore: force-overwrite util (#1107)
Enhanced the existing --overwrite flag with deletion detection and added a new --force-overwrite flag to handle intentional field removals safely. # Now detects deletions and blocks with error ``` DELETION DETECTED: Cannot overwrite test data for 'tests/.../file.json' Total fields to be deleted: 3 Fields that will be deleted: - root['...']['field_name'] ... To proceed anyway, use the --force-overwrite flag. ``` Manual testing: https://github.com/user-attachments/assets/ad9d3a0a-f42c-465d-b515-d422a3f09ce7
1 parent d24147f commit 10a0683

5 files changed

Lines changed: 120 additions & 7 deletions

File tree

src/allotropy/testing/utils.py

Lines changed: 94 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import tempfile
1111
from typing import Any
1212
from unittest import mock
13+
import warnings
1314

1415
from deepdiff import DeepDiff
1516
from deepdiff.model import DiffLevel
@@ -184,6 +185,42 @@ def _assert_allotrope_dicts_equal(
184185
raise AssertionError(msg)
185186

186187

188+
def _check_allotrope_dicts_for_deletions(
189+
expected: DictType,
190+
actual: DictType,
191+
) -> tuple[bool, list[str]]:
192+
"""
193+
Check if the actual dict has any deleted fields compared to expected dict.
194+
195+
Returns:
196+
tuple[bool, list[str]]: (has_deletions, deleted_paths)
197+
- has_deletions: True if any fields were deleted
198+
- deleted_paths: List of JSON paths for deleted fields
199+
"""
200+
expected_replaced = _replace_asm_converter_version(expected)
201+
ddiff = DeepDiff(
202+
expected_replaced,
203+
actual,
204+
ignore_type_in_groups=[(float, np.float64)],
205+
ignore_nan_inequality=True,
206+
custom_operators=[DEEPDIFF_PATH_COMPARATOR],
207+
)
208+
209+
deleted_paths = []
210+
if ddiff:
211+
# Check for dictionary item removals
212+
if "dictionary_item_removed" in ddiff:
213+
for path in ddiff["dictionary_item_removed"]:
214+
deleted_paths.append(str(path))
215+
216+
# Check for iterable item removals (list items)
217+
if "iterable_item_removed" in ddiff:
218+
for path in ddiff["iterable_item_removed"]:
219+
deleted_paths.append(str(path))
220+
221+
return len(deleted_paths) > 0, deleted_paths
222+
223+
187224
class TestIdGenerator:
188225
next_id: int
189226
prefix: str | None
@@ -251,6 +288,7 @@ def validate_contents(
251288
allotrope_dict: DictType,
252289
expected_file: Path | str,
253290
write_actual_to_expected_on_fail: bool = False, # noqa: FBT001, FBT002
291+
force_overwrite: bool = False, # noqa: FBT001, FBT002
254292
) -> None:
255293
"""Use the newly created allotrope_dict to validate the contents inside expected_file."""
256294
# Ensure that allotrope_dict can be written via json.dump()
@@ -260,16 +298,67 @@ def validate_contents(
260298
try:
261299
with open(expected_file, encoding=DEFAULT_ENCODING) as f:
262300
expected_dict = json.load(f)
263-
_assert_allotrope_dicts_equal(expected_dict, allotrope_dict)
264-
except Exception as e:
301+
302+
# Check for deletions before doing the assertion if we're going to overwrite
265303
if write_actual_to_expected_on_fail:
304+
has_deletions, deleted_paths = _check_allotrope_dicts_for_deletions(
305+
expected_dict, allotrope_dict
306+
)
307+
308+
if has_deletions and not force_overwrite:
309+
# Show a summary of deleted fields instead of the full list
310+
if len(deleted_paths) <= 10:
311+
# Show all paths if there are few
312+
deleted_paths_str = "\n".join(
313+
f" - {path}" for path in deleted_paths
314+
)
315+
summary_msg = f"Fields that will be deleted:\n{deleted_paths_str}"
316+
else:
317+
# Show first few and summary for many deletions
318+
first_few = "\n".join(f" - {path}" for path in deleted_paths[:5])
319+
summary_msg = (
320+
f"Fields that will be deleted (showing first 5 of {len(deleted_paths)}):\n"
321+
f"{first_few}\n"
322+
f" ... and {len(deleted_paths) - 5} more fields"
323+
)
324+
325+
error_msg = (
326+
f"DELETION DETECTED: Cannot overwrite test data for '{expected_file}'\n"
327+
f"Total fields to be deleted: {len(deleted_paths)}\n\n"
328+
f"{summary_msg}\n\n"
329+
f"To proceed anyway, use the --force-overwrite flag.\n\n"
330+
f"If you use --force-overwrite, the full list will be provided for your PR description."
331+
)
332+
raise AssertionError(error_msg)
333+
elif has_deletions and force_overwrite:
334+
# Issue warning about deletions for PR description
335+
deleted_paths_str = "\n".join(f" - {path}" for path in deleted_paths)
336+
warning_msg = (
337+
f"Fields will be deleted from '{expected_file}':\n\n"
338+
f"Please copy the following information into your PR description:\n\n"
339+
f"**Fields deleted in test data:**\n"
340+
f"{deleted_paths_str}"
341+
)
342+
warnings.warn(warning_msg, UserWarning, stacklevel=2)
343+
344+
# Only do the assertion if we're not force overwriting
345+
if not (write_actual_to_expected_on_fail and force_overwrite):
346+
_assert_allotrope_dicts_equal(expected_dict, allotrope_dict)
347+
elif write_actual_to_expected_on_fail and force_overwrite:
348+
# Force overwrite - write the file and continue
266349
_write_actual_to_expected(allotrope_dict, expected_file)
350+
351+
except Exception as e:
352+
if write_actual_to_expected_on_fail:
267353
if isinstance(e, FileNotFoundError):
354+
# File doesn't exist, safe to create
355+
_write_actual_to_expected(allotrope_dict, expected_file)
268356
msg = f"Missing expected output file '{expected_file}', writing expected output because 'write_actual_to_expected_on_fail=True'"
269357
raise AssertionError(msg) from e
270-
if isinstance(e, AssertionError) and "allotropy output != expected:" in str(
271-
e
272-
):
358+
elif isinstance(
359+
e, AssertionError
360+
) and "allotropy output != expected:" in str(e):
361+
_write_actual_to_expected(allotrope_dict, expected_file)
273362
msg = f"Mismatch between actual and expected for '{expected_file}', writing expected output because 'write_actual_to_expected_on_fail=True'\n\n{e}"
274363
raise AssertionError(msg) from e
275364
raise

tests/conftest.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@ def pytest_addoption(parser: Parser) -> None:
2020
action="store_true",
2121
help="If set, overwrite failing tests with new data.",
2222
)
23+
parser.addoption(
24+
"--force-overwrite",
25+
action="store_true",
26+
help="If set, allow overwriting test data even when fields are deleted. Use with caution!",
27+
)
2328
parser.addoption(
2429
"--exclude",
2530
action="store",
@@ -44,6 +49,11 @@ def overwrite(request: FixtureRequest) -> Any:
4449
return request.config.getoption("--overwrite")
4550

4651

52+
@pytest.fixture
53+
def force_overwrite(request: FixtureRequest) -> Any:
54+
return request.config.getoption("--force-overwrite")
55+
56+
4757
@pytest.fixture
4858
def warn_unread_keys(request: FixtureRequest) -> Any:
4959
return request.config.getoption("--warn_unread_keys")

tests/json_to_csv/json_to_csv_dataset_test.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ def test_json_to_csv_dataset(
7777
expected_results: dict[str, str],
7878
*,
7979
overwrite: bool,
80+
force_overwrite: bool, # noqa: ARG001
8081
) -> None:
8182
with open(input_file) as infile:
8283
input_json = json.load(infile)

tests/parsers/cytiva_biacore_t200_evaluation/to_allotrope_test.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,12 @@ def test_positive_cases(
130130
*,
131131
overwrite: bool,
132132
warn_unread_keys: bool,
133+
force_overwrite: bool,
133134
) -> None:
134135
"""Override the test method with data masking, cycle reduction, and sensorgram data reduction patches."""
135136
return super().test_positive_cases(
136-
test_file_path, overwrite=overwrite, warn_unread_keys=warn_unread_keys
137+
test_file_path,
138+
overwrite=overwrite,
139+
warn_unread_keys=warn_unread_keys,
140+
force_overwrite=force_overwrite,
137141
)

tests/to_allotrope_test.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,12 @@ class ParserTest:
4444

4545
# test_file_path is automatically populated with all files in testdata folder next to the test file.
4646
def test_positive_cases(
47-
self, test_file_path: Path, *, overwrite: bool, warn_unread_keys: bool
47+
self,
48+
test_file_path: Path,
49+
*,
50+
overwrite: bool,
51+
force_overwrite: bool,
52+
warn_unread_keys: bool,
4853
) -> None:
4954
if warn_unread_keys:
5055
os.environ["WARN_UNUSED_KEYS"] = "1"
@@ -61,8 +66,12 @@ def test_positive_cases(
6166
)
6267
# If expected output does not exist, assume this is a new file and write it.
6368
overwrite = overwrite or not expected_filepath.exists()
69+
# Force overwrite should always allow overwriting
70+
if force_overwrite:
71+
overwrite = True
6472
validate_contents(
6573
allotrope_dict,
6674
expected_filepath,
6775
write_actual_to_expected_on_fail=overwrite,
76+
force_overwrite=force_overwrite,
6877
)

0 commit comments

Comments
 (0)