Repository navigation
t.rast.mapcalc: migrate additional shell test to pytest - #6967
Conversation
github-actions
Bot
added
temporal
|
Hey @ninsbl, I followed up on your suggestion and migrated the t.rast.mapcalc shell test to pytest |
petrasovaa
left a comment
petrasovaa
left a comment
There was a problem hiding this comment.
Thanks! Could you please migrate the entire test, it shouldn't be that much more work. It would be nice to add at least little bit more testing of the results beyond a smoke test, e.g. that the number of maps is correct.
Also the "_pytest.py" is unnecessary, so please rename this test file and the other you added already.
|
Hi @petrosavova, thanks for the detailed review and suggestions |
|
Any reason you migrated it to unittest? It's not necessarily wrong, but pytest is preferred. |
|
Thanks for the review @petrasovaa The switch to grass.gunittest was mainly because pytest was not available in the I’ll recheck the expected number_of_maps assertions and remove the incorrect |
Fix incorrect map count assertions, ensure proper STRDS cleanup, and align tests with actual t.rast.mapcalc behavior across CI environments.
Hi @sakirr05 , please not that tests in the With regards to efficiency of tests, it might be an advantage to have cases for t.rast.mapcalc collected in test so fixures can be shared across cases, at least until fixures get more centraly organized.... |
- Migrates remaining t.rast.mapcalc shell tests to pytest - Stabilizes fixtures for execution in an existing GRASS session - Aligns failure assertions with original shell test behavior - Makes teardown idempotent to avoid failures when data is already removed
|
Hi @petrosavova, @ninsbl I’ve addressed the review comments and squashed the related changes into a single commit for clarity. The tests are now fully migrated to pytest, fixtures are stabilized for execution in an existing GRASS session, failure assertions match the original shell tests, and teardown is idempotent to avoid failures when data is already removed. All tests pass locally; CI is running. |
|
This is not going to work, pytests are expected to create their own session, see all the other fixtures. |
|
I see you use grass.script read_command/run_command family of functions. While that is not wrong, the Tools API would be preferred... |
https://grass.osgeo.org/grass-devel/manuals/python_intro.html#running-tools |
|
@petrasovaa and @ninsbl, thanks a lot for taking the time to explain this. I realize now that I misunderstood how pytest sessions are expected to work in GRASS. I’m still getting familiar with the project’s testing patterns, I appreciate you pointing this out. Sorry about the confusion on my side. I’ll rework the tests to follow the existing fixture-based session setup and switch to the preferred Tools API where appropriate. I’ll push an updated version once that’s done. Thanks again for the patience and guidance. |
Minimal fix following temporal/t.rast.list/tests/conftest.py pattern: - Fixtures now create temporary GRASS project via gs.create_project() - Session initialized via gs.setup.init() context manager - All commands use session.env parameter - Test files unchanged (still use gs.run_command) - No manual cleanup needed (temp directory auto-deleted)
ninsbl
left a comment
ninsbl
left a comment
There was a problem hiding this comment.
This is going into the right direction. Please use the Tools API consistently throughout the tests.
|
hey @ninsbl I added the helper locally to keep the migration minimal, but I’m happy to move it to a shared pytest utility (e.g. grass.script.pytest_utils) and import it in the tests if that’s preferred. Please let me know if you want me to do that in this PR or as a follow-up. |
Just keep it local as you have it to not complicate this further, we may want to evaluate these later. |
|
Hey @petrasovaa and @ninsbl all the previous review feedback has been addressed: tests are now in the correct tests/ directory, using pytest. The _assert_tinfo_key_value helper is kept local as suggested. Would really appreciate a final review whenever you are free |
ninsbl
left a comment
ninsbl
left a comment
There was a problem hiding this comment.
Looks good to me. A second opinion would be very much appreciated.... So I leave it for some else to merge...
@echoix or @wenzeslaus please review it whenever you are free, |
|
@petrasovaa are all your requested changes addressed? |
|
I am working on a t.rast.mapcalc PR. Would be great to have this merged first, @petrasovaa do you think this is good to go? |
Collapsed the duplicated fixtures and helper into a single mapcalc_session + assert_tinfo in conftest.py, replaced the destructive g.remove in the failure test with a rename/restore via try/finally, switched assertions from manual parse_key_val to .keyval (with int values), and trimmed cosmetic noise (boilerplate @author, leading/trailing-space expressions, misleading docstring); also reduced nprocs from 5 to 3.
Migrates one additional shell-based t.rast.mapcalc test to pytest and removes
the corresponding shell test block. This is a small follow-up to #6948.