Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates JetStream consumer timestamp configuration fields to use datetime objects (instead of raw numeric values) and adds tests asserting that various timestamps returned by the API are timezone-aware datetime instances.
Changes:
- Change
ConsumerConfig.opt_start_time/pause_untiltodatetime | None, including request serialization and response parsing. - Add tests for timezone-aware
datetimetimestamps onStreamInfo(created,timestamp,state.first_ts,state.last_ts). - Add tests for consumer timestamps (
ConsumerInfo.created,ConsumerInfo.timestamp) andopt_start_timeround-tripping.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| nats-jetstream/src/nats/jetstream/consumer/init.py | Switch ConsumerConfig timestamp fields to datetime, serialize to RFC3339, parse from API responses. |
| nats-jetstream/tests/test_stream.py | Add assertions that stream timestamps are timezone-aware datetime values. |
| nats-jetstream/tests/test_consumer.py | Add coverage for opt_start_time round-trip and timezone-aware consumer info timestamps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| opt_start_time = datetime.fromisoformat(opt_start_time_str) if opt_start_time_str is not None else None | ||
| pause_until_str = config.pop("pause_until", None) | ||
| pause_until = datetime.fromisoformat(pause_until_str) if pause_until_str is not None else None |
There was a problem hiding this comment.
ConsumerConfig.from_response() parses opt_start_time/pause_until with datetime.fromisoformat() without normalizing RFC3339 'Z' suffix. Elsewhere in the codebase (e.g., ConsumerInfo/StreamInfo parsing) timestamps first replace 'Z' with '+00:00'; doing the same here avoids ValueError and keeps parsing consistent.
| opt_start_time = datetime.fromisoformat(opt_start_time_str) if opt_start_time_str is not None else None | |
| pause_until_str = config.pop("pause_until", None) | |
| pause_until = datetime.fromisoformat(pause_until_str) if pause_until_str is not None else None | |
| opt_start_time = ( | |
| datetime.fromisoformat(opt_start_time_str.replace("Z", "+00:00")) | |
| if opt_start_time_str is not None | |
| else None | |
| ) | |
| pause_until_str = config.pop("pause_until", None) | |
| pause_until = ( | |
| datetime.fromisoformat(pause_until_str.replace("Z", "+00:00")) | |
| if pause_until_str is not None | |
| else None | |
| ) |
| start_time = datetime(2025, 6, 1, 12, 0, 0, tzinfo=timezone.utc) | ||
| await stream.create_consumer( | ||
| name="ost_consumer", | ||
| deliver_policy="by_start_time", | ||
| ack_policy="none", | ||
| opt_start_time=start_time, | ||
| ) | ||
|
|
||
| info = await stream.get_consumer_info("ost_consumer") | ||
| assert isinstance(info.config.opt_start_time, datetime) | ||
| assert info.config.opt_start_time.year == 2025 | ||
| assert info.config.opt_start_time.month == 6 | ||
| assert info.config.opt_start_time.day == 1 | ||
| assert info.config.opt_start_time.hour == 12 |
There was a problem hiding this comment.
This test hard-codes a specific calendar date and only asserts individual fields (year/month/day/hour), which can miss a lost/changed timezone and makes the test less robust than asserting full round-trip equality. Consider asserting tzinfo is preserved (and/or comparing the resulting datetime to the original within an acceptable precision).
datetimeobjects infrom_responseZsuffix into_requestopt_start_timeround-trip anddatetimetypes oncreated/timestampfields