Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

r.unpack: Rename folder test_suite to testsuite to discover tests #3358

Merged
merged 1 commit into from
Jan 14, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Jan 13, 2024

Found out in #3340 that this module didn't discover the test scripts, as it had a different folder name. It doesn't break gunittest with this change, locally, for at least Python 3.8.16 and Python 3.12.1 (using pyenv, and on gitpod). Does it mean that the test actually report a failure? I'm not sure, since I didn't look for how assertions were made at the time with shell scripts, but at least now some more code is run during tests.

It seems only r.unpack has tests that uses any of the files I changed. But they are not found by gunittest, since the shell script and data are in a test_suite directory instead of testsuite. Locally, I renamed the folder and was able to see that the test script was picked up and passed. To make sure, I added flags to both code paths, and they were handled correctly for Python 3.8.16 (before the fix in 3.8.17) and 3.12.1. Let's assume that it works for the other files, since CI can't really prove it to us.

Originally posted by @echoix in #3340 (comment)

@github-actions github-actions bot added raster Related to raster data processing module docs labels Jan 13, 2024
@echoix
Copy link
Member Author

echoix commented Jan 13, 2024

This too is ready to review!

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test runs the t.unpack tool with different data inputs, so this is at least a good smoke test. grass.gunittest runs the shell scripts with -e to fail on non-zero return code (otherwise this would just happily pass even if it fails). I think there were test which needed fixing before being enabled, but this one looks good.

@echoix echoix merged commit 607400a into OSGeo:main Jan 14, 2024
23 checks passed
@echoix echoix deleted the r-unpack-testsuite branch January 14, 2024 03:43
@neteler neteler added this to the 8.4.0 milestone Jan 14, 2024
@neteler
Copy link
Member

neteler commented Jan 14, 2024

(Un)related: here are some more test_suite/ folders like that:

ag -g test_suite | xargs dirname | sort -u
imagery/i.atcorr/test_suite
lib/vector/rtree/test_suite
raster3d/r3.in.bin/test_suite
raster3d/r3.neighbors/test_suite
raster3d/r3.out.netcdf/test_suite
raster3d/r3.timestamp/test_suite
raster/r.cost/test_suite
raster/r.mapcalc/test_suite
raster/r.series.accumulate/test_suite
raster/r.series/test_suite
raster/r.stats/test_suite
raster/r.timestamp/test_suite
raster/r.to.rast3/test_suite
vector/v.surf.bspline/test_suite
vector/v.surf.rst/test_suite
vector/v.timestamp/test_suite
vector/v.what.rast3/test_suite

All to be renamed?

@echoix
Copy link
Member Author

echoix commented Jan 14, 2024

I didn't really know about ag and ag -g, it's probably what I was missing. VS code code search was searching only the contents, I found it weird that I only found references for lib/vector/rtree/test_suite, but only since it was known a translation file, not because of the contents. For lib/vector/rtree/test_suite, it is some C code, I'm not sure it would be picked up.

For the others, of course I can try them out to see if they are working or not.

@neteler
Copy link
Member

neteler commented Jan 14, 2024

I didn't really know about ag and ag -g, it's probably what I was missing.

(ag is the "Silver Searcher", a tool for searching code (and file names) which is ultra-fast. One can also restrict the search in a directory tree to selected programming languages, and more. See https://geoff.greer.fm/ag/)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module raster Related to raster data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants