Repository navigation
Make time with colons test deterministic - #190
Merged
blairconrad merged 1 commit intoOct 31, 2026
Merged
blairconrad merged 1 commit into
blairconrad merged 1 commit into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The test-only change removes flaky equality outcomes and adds explicit checks that minutes, seconds, and fractional seconds are preserved.
No open issues 🎉
What changed in this PR
This pull request makes the colon-formatted DICOM time anonymization test deterministic and verifies that the time components are preserved.
Changes:
- Uses
Anonymizer(seed="")to prevent intermittent equality with the original time. - Verifies that minutes, seconds, and fractional seconds remain unchanged.
- Updates tests only; no production code is modified.
| File | Description |
|---|---|
tests/test_anonymize_single_instance.py |
Makes the time anonymization test deterministic and strengthens its output assertions. |
Review effort: Lite
Owner
|
Hi, @pradeeppeddineni. Thanks for the contribution! Sorry for the delay; I had been travelling and then... well, life. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #188.
The failing run was test_time_with_colons_gets_anonymized. The date offset is always a whole number of hours, so whenever a random seed lands on a whole number of days the time of day comes out the same and the new_time != "121558.123456" check fails. I measured it locally and the time came back unchanged in 36 of 1000 runs with a random seed, which lines up with roughly 1 in 24.
I switched the test to Anonymizer(seed=""), same as test_multivalued_date_and_time_pair_gets_anonymized does, and added a comment saying why. With that seed the offset is -6553 hours so the time always comes out as 111558.123456. I also added a check that the minutes and seconds are kept (new_time[2:] == "1558.123456"), which shows the colons got stripped and the value was really parsed rather than just mangled.
test only change, no code touched. uv run test.py passes (ruff format, ruff check, mypy, 328 tests).