Repository navigation
lib/datetime: Add pytest unit tests for datetime_scan and datetime_format - #7871
Merged
Merged
Conversation
Contributor
Author
|
@echoix, Can you review the PR? |
Contributor
Author
|
@ninsbl review? |
ninsbl
approved these changes
Aug 31, 2026
ninsbl
left a comment
Member
There was a problem hiding this comment.
Really comprehensive tests. Thanks for the effort.
They look good to me. Only thing to consider is whether we want to add so much test code for a rather small part of the code-base (datetime only).. It does not seem to add noticable time to the pytest execution though, so that part seems fine too...
I leave that for other maintainers to decide and merge (or request changes)...
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.
Add ctypes unit tests for
lib/datetimeSummary
lib/datetimeis the C library behindG_format_timestampandG_scan_timestamp, and is used by the timestamp functionality across GRASS, includingr.timestamp,v.timestamp,r3.timestamp, and thet.*modules.As discussed in #7802, the library did not previously have a dedicated test suite. This PR adds direct unit tests for the C datetime API using the existing
grass.lib.datectypes bindings.The tests call the C API directly. They do not invoke CLI tools, start a GRASS session, or depend on test fixtures. Since
lib/datetimedoes not depend onG_gisinitor other GIS state, this keeps the tests at the unit-test level rather than turning them into integration tests.The suite contains 107 tests across 10 test files, following the existing ctypes test naming convention used by
lib_vector_rtree_ctypes_test.py.Test Coverage
lib_datetime_ctypes_test.pyCovers
datetime_scananddatetime_format, including:lib_datetime_type_ctypes_test.pyCovers:
datetime_set_typedatetime_check_typelib_datetime_values_ctypes_test.pyCovers the field-level getters and setters for:
Day validation is also covered against leap-year boundaries rather than treating February as having a fixed number of days.
lib_datetime_calendar_ctypes_test.pyCovers:
datetime_is_leap_yeardatetime_days_in_yeardatetime_days_in_monthlib_datetime_sign_ctypes_test.pyCovers the sign and comparison helpers, including:
lib_datetime_timezone_ctypes_test.pyCovers:
lib_datetime_increment_ctypes_test.pyCovers
datetime_incrementand its associated validation helpers, including incrementing values across relevant calendar boundaries.lib_datetime_difference_ctypes_test.pyCovers
datetime_differenceacross:lib_datetime_change_from_to_ctypes_test.pyCovers precision changes between datetime representations, including:
The more involved rounding modes are intentionally left for a follow-up rather than expanding the scope of this PR.
lib_datetime_error_ctypes_test.pyCovers the library's global error-state API and verifies the expected error handling behavior.
Existing Behavior Noted During Testing
While testing
datetime_change_timezoneanddatetime_change_to_utc, I noticed that both functions correctly adjust the clock fields but do not updatedt->tz, despite the function documentation stating that the timezone field should also be updated.This does not currently affect the only caller,
datetime_difference, because that caller discards the timezone field after the conversion.I have not changed that behavior as part of this PR. Instead, the tests document the current behavior and include a comment explaining the discrepancy rather than treating it as an intentional API guarantee.
I think this would be better handled as a separate issue/PR so that the behavioral change can be reviewed independently.
Validation
I tested the suite against an actual compiled version of
lib/datetimerather than relying only on source-level inspection.For the verification:
lib/datetime/*.csources into a shared library.ctypesgentooling against the real headers to generate thegrass.lib.datebindings.ruffversion from.pre-commit-config.yaml(v0.15.17).This confirms that the tests exercise the compiled C implementation through the same ctypes interface used by the project, rather than testing a Python reimplementation or mocked behavior.
AI Disclosure
I used AI assistance (Claude) while drafting and iterating on the tests, including helping verify test cases against a real compiled
lib/datetimebuild.I reviewed the generated tests and validation results myself and made the final decisions about coverage, expected behavior, and the scope of the changes.