temporal: add unit tests for AbstractSpaceTimeDataset shift() and snap()#7233
temporal: add unit tests for AbstractSpaceTimeDataset shift() and snap()#7233selma-Bentaiba wants to merge 3 commits intoOSGeo:mainfrom
Conversation
Introduces unittests_temporal_stds_management.py with dedicated coverage for core management methods that had no existing tests. - test_shift_absolute_time: timestamps advance by the given granularity - test_shift_invalid_granularity: regression test for OSGeo#7228, ensures invalid input returns False rather than a formatting TypeError - test_snap_closes_gaps: end times are snapped to next map's start time test_shift_relative_time is commented out because relative time unit type resolution fails in GRASS 7.8.7 with 'unsupported unit type: None'. Needs verification against GRASS 8.x once OSGeo#7231 is merged. Related: OSGeo#7228 (fix string formatting in fatal error messages) OSGeo#7231 (add explicit return True in shift())
github-actions
bot
added
Python
|
How did you even get 7.8.7 working? Yes it is a release, but hidden multiple pages after in the releases, like 11 releases behind. |
|
Please install and use pre-commit hooks in your dev setup. |
|
You know you could just run |
Haha, you definitely caught me there! I just ran a standard sudo apt install grass on my Ubuntu machine, As you can see from the distutils warnings in the screenshot, Python 3.10 is definitely screaming at Version 7! I actually moved forward with the initial PR because the core TGIS logic I was studying seemed consistent across the versions, but I completely see why that architectural gap is a major issue for a modern contribution. I’ve already purged the legacy version and am currently moving to the 8.4 dev branch (via the UbuntuGIS PPA) to ensure the PR and my propo |
Message received. |
To be honest, I have uv installed, but I’ve been too focused on the TGIS logic and mistakenly relied on the CI for the formatting checks. I definitely see now how that impacts project resources. I’ve officially integrated uvx prek and the pre-commit hooks into my local save-flow so I don't waste any more shared CI time on linting. Lesson learned ++ |
|
7.8.7 from a distro’s repo makes sense, sorry about my tone. It’s because of the years, it coincides with what many LLMs were trained on, and there’s no way a contributor making efforts would choose that to test on. Even 8.4 is quite old. 8.5 (not out yet, but should be soon), has more than 1.5 years of improvements, especially in code quality. Theres not that much in 8.4, but way more in 8.5. In the years before, code quality wasn’t as of a priority as it is now. So python errors in 7.8.7 mean nothing now |
selma-Bentaiba
force-pushed
the
add-tests-stds-management
branch
from
b1f7523 to
45bbb53
Compare
After further local testing and refactoring, I have updated this PR to meet the latest standards:
Ready for final review. @ninsbl @echoix Thank you for the guidance on setting up the environment! |
I appreciate the explanation @echoix ! |
|
Sorry for not checking earlier, did you see the t.shift tool tests: https://github.com/OSGeo/grass/blob/main/temporal/t.shift/testsuite/test_shift.py ? What do these unittests add? |
| """Unit tests for shift() and snap() methods | ||
| of AbstractSpaceTimeDataset | ||
|
|
||
| (C) 2026 by the GRASS Development Team | ||
| This program is free software under the GNU General Public | ||
| License (>=v2). Read the file COPYING that comes with GRASS | ||
| for details. | ||
|
|
||
| :authors: Selma Bentaiba | ||
| """ |
There was a problem hiding this comment.
For new files we usually include a SPDX licence line, there's some place somewhere where we have an example.
There was a problem hiding this comment.
The name of the file might not match the patterns that we are checking for. Double check in CI logs that this file is actually ran.
| # TODO: test_shift_relative_time is disabled pending GRASS 8.x testing. | ||
| # The registration of relative time maps with unit="seconds" fails in | ||
| # GRASS 7.8.7 with "Unsupported relative time unit type: None". | ||
| # This should be re-enabled and verified once tested against GRASS 8.x. | ||
| # Related to #7231. | ||
| # | ||
| # def test_shift_relative_time(self) -> None: | ||
| # """shift() moves timestamps forward for relative time datasets""" | ||
| # uid = uuid.uuid4().hex[:6] | ||
| # map_rel_1 = f"map_rel_1_{uid}" | ||
| # map_rel_2 = f"map_rel_2_{uid}" | ||
| # strds_rel_name = f"shift_test_rel_{uid}" | ||
| # self.runModule( | ||
| # "r.mapcalc", overwrite=True, quiet=True, | ||
| # expression=f"{map_rel_1} = 1" | ||
| # ) | ||
| # self.runModule( | ||
| # "r.mapcalc", overwrite=True, quiet=True, | ||
| # expression=f"{map_rel_2} = 2" | ||
| # ) | ||
| # strds_rel = tgis.open_new_stds( | ||
| # name=strds_rel_name, | ||
| # type="strds", | ||
| # temporaltype="relative", | ||
| # title="Test shift relative", | ||
| # descr="Test shift relative", | ||
| # semantic="field", | ||
| # overwrite=True, | ||
| # ) | ||
| # tgis.register_maps_in_space_time_dataset( | ||
| # type="raster", | ||
| # name=strds_rel.get_name(), | ||
| # maps=f"{map_rel_1},{map_rel_2}", | ||
| # start=0, | ||
| # increment=1, | ||
| # unit="seconds", | ||
| # interval=True, | ||
| # ) | ||
| # strds_rel.shift(gran="5") | ||
| # maps = strds_rel.get_registered_maps_as_objects( | ||
| # where=None, order="start_time" | ||
| # ) | ||
| # start, end = maps[0].get_temporal_extent_as_tuple() | ||
| # self.assertEqual(start, 5) | ||
| # self.assertEqual(end, 6) | ||
| # self.runModule( | ||
| # "t.unregister", | ||
| # type="raster", | ||
| # maps=f"{map_rel_1},{map_rel_2}", | ||
| # quiet=True, | ||
| # ) | ||
| # strds_rel.delete() | ||
| # self.runModule( | ||
| # "g.remove", flags="f", type="raster", | ||
| # name=f"{map_rel_1},{map_rel_2}", quiet=True | ||
| # ) | ||
|
|
There was a problem hiding this comment.
Hmm, better to check it now, and act accordingly. Commented code is not really maintained and gets forgotten for always.
8.x is already a couple years out, so it's time for it.
After digging into the existing t.shift and t.snap suites, I’ll admit my approach was a bit redundant; I’m still learning the layers ... That said, I found a few gaps where this PR may actually pulls its weight:
I’m happy to keep this as a new file or just migrate these 'gap-fillers' into the existing suites to keep things lean. What’s the cleaner move?" @ninsbl |
|
If there’s a test file per Python class, usually that’s good. If the tests could benefit from the same setup/teardown as the existing file -> keep in the same file. If your test setup/teardown is simpler (and faster to run) -> maybe a different file? temporal tests are way longer than they should be, probably something in the library, but either way, temporal tests take the same time as all the rest, combined. |
|
@echoix Thank you for your notes, it helps a bunch .
Additionally, I've run the pre-commit hooks to ensure the file meets all project formatting standards (Ruff/formatting) 
|
|
For your warning, install pytest-timeout package. It is partly for temporal tests taking way too long that we limited the time taken, but temporal tests usually run through gunittest. But if your test can run through pytest, it could be great to whitelist that gunittest file to be ran with pytest (in .github/workflows/pytest**.txt files), and blacklist it in .gunittest.cfg). It would need to pass CI on all our platforms, but it would be great! |
4 participants
Summary
Adds missing test coverage for
shift()andsnap()methods ofAbstractSpaceTimeDataset, which had no dedicated unit tests.Tests added
test_shift_absolute_time: timestamps advance by the given granularitytest_shift_invalid_granularity: regression test for temporal: fix string formatting in fatal error messages #7228, ensuresinvalid input returns False rather than a formatting TypeError
test_snap_closes_gaps: end times are snapped to next map's start timeNotes
test_shift_relative_timeis commented out. Relative time unit typeresolution fails in GRASS 7.8.7 with 'unsupported unit type: None'.
Needs verification against GRASS 8.x once #7231 is merged.
Related
#7228 #7231