t.rast.mapcalc: migrate additional shell test to pytest by sakirr05 · Pull Request #6967 · OSGeo/grass

sakirr05 · 2026-01-25T23:39:48Z

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.

sakirr05 · 2026-01-25T23:43:54Z

Hey @ninsbl, I followed up on your suggestion and migrated the t.rast.mapcalc shell test to pytest

petrasovaa

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.

temporal/t.rast.mapcalc/tests/test_t_rast_mapcalc_basic_pytest.py

sakirr05 · 2026-01-27T18:14:22Z

Hi @petrosavova, thanks for the detailed review and suggestions
I’ve now migrated the remaining shell tests to pytest, added stronger assertions (map count, name, temporal type), fixed overwrite handling, and cleaned up the file naming as you suggested.

…l tests

…nClass

petrasovaa · 2026-01-28T14:56:07Z

Any reason you migrated it to unittest? It's not necessarily wrong, but pytest is preferred.

petrasovaa · 2026-01-28T14:57:18Z

temporal/t.rast.mapcalc/testsuite/test_t_rast_mapcalc_basic.py

+        strds_info = gs.parse_key_val(info.outputs.stdout, sep="=")
+        self.assertEqual(strds_info["name"], "precip_abs4")
+        # With null() multiplication, maps may be empty but STRDS should exist
+        self.assertIn("number_of_maps", strds_info)


Please check the actual number here (applies to other tests as well).

temporal/t.rast.mapcalc/testsuite/test_t_rast_mapcalc_operators.py

sakirr05 · 2026-01-28T16:01:02Z

Thanks for the review @petrasovaa

The switch to grass.gunittest was mainly because pytest was not available in the
temporal CI jobs for this PR, which caused the original failures. Happy to follow
the preferred direction once pytest is consistently available there.

I’ll recheck the expected number_of_maps assertions and remove the incorrect
comment unnecessary flag in a follow-up commit.

Fix incorrect map count assertions, ensure proper STRDS cleanup, and align tests with actual t.rast.mapcalc behavior across CI environments.

ninsbl · 2026-01-28T23:22:04Z

The switch to grass.gunittest was mainly because pytest was not available in the temporal CI jobs for this PR, which caused the original failures. Happy to follow the preferred direction once pytest is consistently available there.

Hi @sakirr05 , please not that tests in the testsuite directory are picked up as gunit-tests, while tests in a testsdirectory would be picked up by pytest.
So, pytest is fine and available also for temporal tests, just put them in a ./temporal/t.rast.mapcalc/tests folder instead of ./temporal/t.rast.mapcalc/testsuite, as you did with the test you added ~two days ago...

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....

…irectory

- 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

sakirr05 · 2026-01-29T21:57:00Z

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.

petrasovaa · 2026-01-29T22:10:51Z

This is not going to work, pytests are expected to create their own session, see all the other fixtures.

ninsbl · 2026-01-29T22:16:53Z

I see you use grass.script read_command/run_command family of functions. While that is not wrong, the Tools API would be preferred...

petrasovaa · 2026-01-29T22:23:34Z

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

sakirr05 · 2026-01-29T22:29:17Z

@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)

…session init

ninsbl

This is going into the right direction. Please use the Tools API consistently throughout the tests.

ninsbl · 2026-01-31T08:21:32Z

temporal/t.rast.mapcalc/tests/test_t_rast_mapcalc_basic.py

+def test_basic_addition(mapcalc_session_basic):
+    """Test basic addition of two STRDS with -n flag (register null maps)."""
+    env = mapcalc_session_basic.env
+    gs.run_command(


Why not using tools also here? They also provide functios for stdout parsing, like e.g. `tools.t_info(...).json?

hey @ninsbl Thanks for pointing this out. I’ve now switched the remaining test code to use the Tools API consistently — the earlier version was mainly to confirm CI stability.Please let me know if anything else should be adjusted.

ninsbl · 2026-01-31T21:22:09Z

Thanks, @sakirr05 ! Much better. I see you define a copy of _assert_tinfo_key_value in each test. Not sure where to place that function... in #6995 such functions were added to python/grass/script/pytest_utils.py...

sakirr05 · 2026-01-31T22:24:39Z

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.

petrasovaa · 2026-02-02T03:43:01Z

temporal/t.rast.mapcalc/tests/test_t_rast_mapcalc_operators.py

+    reference = dict(
+        line.strip().split(sep, 1)
+        for line in reference_string.strip().split("\n")
+        if sep in line
+    )


Hm, just represent the reference as dict instead of multiline string and you can avoid all this parsing.

hey @petrasovaa I’ve addressed the requested changes

petrasovaa · 2026-02-02T03:47:59Z

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.

t.rast.mapcalc: migrate additional shell test to pytest

bafc6a1

github-actions bot added temporal Related to temporal data processing Python Related code is in Python module tests Related to Test Suite labels Jan 25, 2026

petrasovaa requested changes Jan 27, 2026

View reviewed changes

petrasovaa reviewed Jan 27, 2026

View reviewed changes

temporal/t.rast.mapcalc/tests/test_t_rast_mapcalc_basic_pytest.py Outdated Show resolved Hide resolved

t.rast.mapcalc: migrate remaining shell tests to pytest

f8212dc

sakirr05 requested a review from petrasovaa January 27, 2026 18:14

sakirr added 2 commits January 28, 2026 11:40

tests(t.rast.mapcalc): replace pytest with grass.gunittest in tempora…

bf53070

…l tests

tests(t.rast.mapcalc): explicitly remove temporal datasets in tearDow…

09ed4f3

…nClass

petrasovaa reviewed Jan 28, 2026

View reviewed changes

temporal/t.rast.mapcalc/testsuite/test_t_rast_mapcalc_operators.py Outdated Show resolved Hide resolved

t.rast.mapcalc: fix pytest failures and temporal cleanup

bda705a

Fix incorrect map count assertions, ensure proper STRDS cleanup, and align tests with actual t.rast.mapcalc behavior across CI environments.

sakirr added 3 commits January 29, 2026 09:20

tests(t.rast.mapcalc): convert tests to pytest and place in correct d…

054669c

…irectory

tests(t.rast.mapcalc): run fixtures in existing GRASS session

ef84825

sakirr added 3 commits January 29, 2026 23:15

tests(t.rast.mapcalc): remove create_project and use standard pytest …

e2f9cc1

…session init

tests(t.rast.mapcalc): create GRASS project before gs.setup.init

239be0c

tests(t.rast.mapcalc): use Tools API in pytest fixtures

a83590a

ninsbl requested changes Jan 31, 2026

View reviewed changes

tests(t.rast.mapcalc): use Tools API consistently in tests

5296679

sakirr05 requested a review from ninsbl January 31, 2026 13:31

sakirr05 requested a review from petrasovaa February 1, 2026 17:36

petrasovaa reviewed Feb 2, 2026

View reviewed changes

tests(t.rast.mapcalc): simplify t.info reference to dict

9b4594b

sakirr05 requested a review from petrasovaa February 2, 2026 10:26

Uh oh!

Conversation

sakirr05 commented Jan 25, 2026

Uh oh!

sakirr05 commented Jan 25, 2026

Uh oh!

petrasovaa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sakirr05 commented Jan 27, 2026

Uh oh!

petrasovaa commented Jan 28, 2026

Uh oh!

petrasovaa Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sakirr05 commented Jan 28, 2026

Uh oh!

ninsbl commented Jan 28, 2026

Uh oh!

sakirr05 commented Jan 29, 2026

Uh oh!

petrasovaa commented Jan 29, 2026

Uh oh!

ninsbl commented Jan 29, 2026

Uh oh!

petrasovaa commented Jan 29, 2026

Uh oh!

sakirr05 commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ninsbl left a comment

Choose a reason for hiding this comment

Uh oh!

ninsbl Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

sakirr05 Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

ninsbl commented Jan 31, 2026

Uh oh!

sakirr05 commented Jan 31, 2026

Uh oh!

petrasovaa Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

sakirr05 Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

petrasovaa commented Feb 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sakirr05 commented Jan 29, 2026 •

edited

Loading