[jalpatel11]

Summary

r.timestamp had no test coverage. This adds a pytest suite (tests/r_timestamp_test.py) covering:

• Reading a map with no timestamp set (should fail)
• Setting and reading back a single absolute date
• Setting and reading back a date range
• Overwriting an existing timestamp
• Removing a timestamp with date=none
• Rejecting an invalid date string
• Failing on a non-existent map when reading
• Failing on a non-existent map when setting

8 tests total. Uses the shared session_tools fixture from raster/conftest.py (same one r.colors's tests use) instead of a local setup, per AGENTS.md's guidance to write new tests in pytest when the NC SPM sample dataset isn't needed.

Testing

Ran the suite against a real main-branch GRASS session (osgeo/grass-gis:main-alpine Docker image) — all 8 tests pass. Checked formatting/lint with the pinned ruff version from .pre-commit-config.yaml (v0.15.17).

AI disclosure

I used AI assistance (Claude) to draft and verify these tests, then reviewed and tested them myself as described above.

[ninsbl]

Hi, and thanks for your PR. Please note that we are moving tests from unittests to pytest. So, new tests should be written using pytest. Since you are using AI, please note that there are Agent instructions in the repo. So please make use of them.

[jalpatel11]

@ninsbl Thanks for the feedback! I've updated the PR to use pytest instead of gunittest.

The old testsuite/test_r_timestamp.py is removed, and the tests now live in raster/r.timestamp/tests/r_timestamp_test.py. Since these tests don't need the NC SPM sample dataset, pytest was the right call per AGENTS.md. They use grass.tools and the session_tools fixture from raster/conftest.py (same one r.colors's tests use), with pytest.raises(ToolError) for the failure cases.

I ran the full suite against a real GRASS session and all 8 tests pass.

[jalpatel11]

@ninsbl, can you review it?

[ninsbl]

Thanks for rewriting it with pytest.
Some more comments:

1. there are no tests that include time, please add some
2. you are often creating a raster map as part of the test. That is probably better included in a fixure. Maybe @echoix or @wenzeslaus know what would be best practice here...
3. There have been questions about handling of timezone information, which - to my knowledge - r.timestamp does not do. I would say having a test failing if timezone information is included would be good, as demonstration, clarification and target if that should be changed at some point in future...

[jalpatel11]

@ninsbl I've pushed an update addressing all three points:

Time-of-day tests — added a parametrized test covering hour-only, hour:minute, hour:minute:second, and fractional-second granularity, since the format function handles each of those as a separate case.

Raster setup moved to a fixture — added a conftest.py with a raster fixture that creates a fresh, timestamp-less raster in the current mapset. All tests now take raster as a parameter instead of repeating the r_mapcalc call. Let me know if you or @echoix , @wenzeslaus have a different pattern in mind — happy to adjust.

Timezone handling — I actually tested this directly (set a timestamp with +0100, -0500, and a range with a timezone on both ends, then read it back) and r.timestamp round-trips the timezone offset correctly at this level. So I don't think it's accurate that r.timestamp itself drops timezone info. Digging further, I found the actual known gap: grass.temporal's _convert_timestamp_from_grass() (in c_libraries_interface.py) has a # TODO: Write time zone support comment and explicitly discards the timezone when converting to a Python datetime object — that's tracked in #7166. That's a separate code path from r.timestamp's own get/set, which goes through G_format_timestamp/G_scan_timestamp directly. So instead of a failing test against r.timestamp (which would be testing something that isn't actually broken), I added passing tests that lock in the correct round-trip behavior, with a docstring pointing to #7166 for context on where the real gap lives.

All 15 tests pass against a real GRASS session.

[ninsbl]

Thanks. Looks good to me. Someone else willing to have a second look and merge, in case?

[echoix]

Could there be something to make sure there isn't a future mix up between American style 12 hour times and 24 hour times? I only saw times in the 10:30 range (except a round trip using a timezone).

[jalpatel11]

@echoix Good point, and it made me look more closely at why hour 10 wasn't a great choice rather than just picking a different arbitrary hour.

Pushed an update:

• Added a test for midnight (00:00) and noon (12:00) specifically. Those are the two values where a naive 24h→12h conversion classically breaks (0 % 12 == 12 % 12 == 0), so an arbitrary afternoon hour like 14 wouldn't have caught that failure mode as precisely.
• Added a test confirming "2:30 pm" / "02:30 PM" style input is rejected outright. The more concerning failure mode than a display bug would be silent misinterpretation (e.g. the pm suffix getting silently stripped and 2:30 being read as 2:30 AM), so this locks in that it errors instead.

Both pass against a real GRASS session, 19 tests total now.

One thing worth flagging for future scope, not this PR: lib/datetime (the C library actually doing this formatting/parsing) has no dedicated test suite of its own anywhere in the repo. Right now these edge cases are only reachable indirectly, through tools like r.timestamp that happen to expose G_format_timestamp/G_scan_timestamp via CLI. A proper lib/datetime test suite — BC dates, relative time formats, fractional-second precision limits, more timezone edge cases — would benefit every tool that touches timestamps (v.timestamp, r3.timestamp, the t.* modules), not just this one. Happy to look into that as a separate PR if useful.

[echoix]

One thing worth flagging for future scope, not this PR: lib/datetime (the C library actually doing this formatting/parsing) has no dedicated test suite of its own anywhere in the repo. Right now these edge cases are only reachable indirectly, through tools like r.timestamp that happen to expose G_format_timestamp/G_scan_timestamp via CLI. A proper lib/datetime test suite — BC dates, relative time formats, fractional-second precision limits, more timezone edge cases — would benefit every tool that touches timestamps (v.timestamp, r3.timestamp, the t.* modules), not just this one. Happy to look into that as a separate PR if useful.

Please do, if you're able to make pytests from the exposed c api, and have the bandwidth to do so. (Probably from our bindings (based on ctypes, the same way Python functions access the c api). It would probably be cleaner and real unit tests, not integration tests.

Start with something reviewable, as there are not many of these kinds of tests, so we might need to iterate a bit to find a good pattern

[jalpatel11]

@echoix @ninsbl
Sounds good, I'll open a separate PR for that, using the ctypes bindings to hit datetime_format/datetime_scan directly rather than going through a CLI tool. Will start small and reviewable given there isn't an established pattern for this yet.

If this PR looks good otherwise, can we move forward with it?