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

tests: replaced module imports to the top #4656

Merged
merged 3 commits into from
Nov 20, 2024
Merged

Conversation

arohanajit
Copy link
Contributor

@arohanajit arohanajit commented Nov 5, 2024

Fixes E402

@github-actions github-actions bot added Python Related code is in Python libraries labels Nov 5, 2024
@arohanajit arohanajit changed the title test: replaced module imports to the top tests: replaced module imports to the top Nov 6, 2024
@echoix
Copy link
Member

echoix commented Nov 19, 2024

I'm not convinced (yet) that this is a better pattern. We don't see that often, and the real problem is the fact that plying around with the paths was used there, as our Python ctypes binding dont seem available when running plain pytest, instead of the heavy workarounds used when loading grass and executing scripts inside that shell, like our gunittest does.

In my opinion, running Python scripts like with pytest, the expected behaviour should be to just work when grass is installed. But all that is a larger infrastructure-wide issue, that @wenzeslaus has been slowly working on, and is also my common thread across all my efforts in the last year and a half.

So to recap, I think that the problem in the file is the use of the two lines that play with the path (it's really not a good practice, I saw it more in Python 2 code when users didn't understand why a script didn't work and stopped when adding enough so it works on their machine).

If it was really a test file, it could be run with the current workaround for some gunittest-tests to run through pytest: launch a grass shell, (the paths are set up), and inside call Python, pytest, or the script wanted.
After reading the file finally, I see that it is not a test file finally, it is only a standalone tool. The problem with it is that it is inside a "tests" directory, that is normally used for pytest tests. It isn't picked up by pytest as it is called "benchmark", so it doesn't start or end in test (and for now pytest is configured to collect only files ending in "_test.py" inside directories named "tests").
Should it be placed somewhere else, in a graveyard with other scripts and utilities ? I see that there's a makefile that installs this, so what is it used for?

@petrasovaa
Copy link
Contributor

I suppose this benchmark was useful when developing pygrass, less so now.

@petrasovaa petrasovaa added this to the 8.5.0 milestone Nov 20, 2024
Comment on lines 81 to -84
# Configuration file for Sphinx:
# Ignoring import/code mix and line length.
# Files not managed by Black
python/grass/imaging/images2gif.py: E226
Copy link
Member

Choose a reason for hiding this comment

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

At the next PR, I think you could have a clean up of the comments of sections that are now empty.

@echoix echoix enabled auto-merge (squash) November 20, 2024 13:34
@echoix echoix merged commit 36359e2 into OSGeo:main Nov 20, 2024
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants