Skip to content

test: pin invalid timezone error on EncounterRecencyTransformer - #222

Closed
HarshRajSinghania wants to merge 1 commit into
PhilanthroPy-Project:mainfrom
HarshRajSinghania:test/invalid-timezone-keyerror-204
Closed

HarshRajSinghania wants to merge 1 commit into
PhilanthroPy-Project:mainfrom
HarshRajSinghania:test/invalid-timezone-keyerror-204

Conversation

@HarshRajSinghania

Copy link
Copy Markdown
Contributor

Closes #204.

Summary

Adds a regression test so an invalid timezone= on EncounterRecencyTransformer raises KeyError that names the zone. The pre-#202 fallback turned a bad zone into TypeError("Already tz-aware"); this test would fail if that path returned.

No production code change. __init__ still does not validate timezone (sklearn convention noted in the issue).

Motivation

#204: tests/test_encounter_timezone.py only used valid timezones or none. The behaviour restored in #202 was unguarded.

Implementation

  • Reuse the two-row fixture from test_encounter_recency_localises_to_a_non_utc_timezone.
  • Construct the transformer with timezone="Not/AZone" and call fit_transform.
  • Assert pytest.raises(KeyError, match="Not/AZone") so both pandas 2 (pytz) and pandas 3 (zoneinfo) are covered without importing pytz.
  • Assert the message does not contain Already tz-aware.
  • CHANGELOG.md [Unreleased] entry and CONTRIBUTORS.md line, as required by the project.

Testing

Intended local commands from the issue:

python -m pip install -e ".[dev]"
python -m pytest tests/test_encounter_timezone.py -q
make ci

This environment could not install scikit-learn (PyPI 502), so the new test was not executed here. CI on this PR should run the file and the full matrix.

Closes PhilanthroPy-Project#204.

Signed-off-by: Harsh Raj Singhania <harshrajsinghania@proton.me>
@shivamlalakiya

Copy link
Copy Markdown
Contributor

Closing this as a duplicate: #204 was claimed by @Kaap10 on 2026-09-12 and confirmed on the issue the same day, and #221 has now merged as dba8cd5.

That is the claim-before-PR rule from #187 working as intended, and it is on me that it is easy to miss: the claim is a comment on the issue rather than an assignee, so the issue looks unclaimed from the list view. Checking the issue thread before starting is the only reliable way right now.

Nothing wrong with the test itself. If you want another, #50, #53 and #55 are all open with zero comments, so no claim to collide with:

Thanks for #216 and for this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: no test pins the invalid-timezone error that #202 restored in EncounterRecencyTransformer

2 participants