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: Revise content of the root testsuite dir #4516

Merged
merged 6 commits into from
Oct 15, 2024

Conversation

wenzeslaus
Copy link
Member

  • Remove Makefiles.
  • Move the functioning test to the testsuite directory for the gunittest to pick it up.
  • Remove the extended example how to run gunittest. Point to the CI in the README instead.
  • Remove hemisphere generator because it does not test anything by itself (without further processing and a human).
  • Remove GPS import tests as the tools are not in core.
  • Remove division into subdirectories. The directories with the code should be used instead.
  • Improve the test for gunittest loader function with some duplication but now creating an overview of .py and .sh tests.

* Remove Makefiles.
* Move the functioning test to the testsuite directory for the gunittest to pick it up.
* Remove the extended example how to run gunittest. Point to the CI in the README instead.
* Remove hemisphere generator because it does not test anything by itself (without further processing and a human).
* Remove GPS import tests as the tools are not in core.
* Remove division into subdirectories. The directories with the code should be used instead.
* Improve the test for gunittest loader function with some duplication but now creating an overview of .py and .sh tests.
@github-actions github-actions bot added Python Related code is in Python libraries docs tests Related to Test Suite labels Oct 14, 2024
@wenzeslaus
Copy link
Member Author

Do we want to install md5sum on macOS, replace the md5sum call with inlined Python, or disable the test on macOS? (I'm not ready to rewrite the whole test to Python, although that's an option, too.)

md5sum required, please install first

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@echoix
Copy link
Member

echoix commented Oct 14, 2024

I think installing md5sum might be appropriate for now, I don't see a quick way to have it called on cli without having a full script written. Once cached, installs on macOS CI are quite fast.

@nilason
Copy link
Contributor

nilason commented Oct 14, 2024

On Mac there is md5 available by default.

@echoix
Copy link
Member

echoix commented Oct 14, 2024

Regarding the note that the modern way is using pytest (for gunittest) in the modified testsuite/README.md, theres a couple of points that I think are misunderstood or has limitations.

  • When pytest runs (pure) unittest-based tests, it supports all of the features.
  • When pytest runs gunittest-based tests, we still can use the custom asserts that were developed within gunittest.
  • When pytest runs gunittest-based tests, tests needing to copy the data folder don't work as is, as that data folder is not found. In gunittest, there was an operation to copy that folder somewhere that isn't part of the test class itself (probably done in the layer that runs the test instead of the test classes themselves (like fixtures).
  • For pytest to run sucessfully gunittest-based tests, we still need to run them called from a grass session with exec (for now).
  • For pytest to run sucessfully gunittest-based tests, a single line must be added to gunittest to mark that the function test() in python/grass/gunittest/main.py isn't a test. Pytest can pick that method attribute. Otherwise, all tests that import from grass.gunittest.main import test outside of if __name__ == "__main__": will have a method in that pytest thinks needs to be tested. Like:
    from grass.gunittest.main import test
    • The line to add is : test.__test__ = False # prevent running this function as a test
  • For pytest to discover gunittest-based tests, we need to edit the allowed patterns in pytest's config in the pyproject.toml file to include testsuite subfolders, and accept files that start with test_*. An example of what was needed was in a branch of cyliang368, when we tried it at the last community meeting in 2024. It would be better to let pytest use its defaults and add the testsuite folder compared to what is in this branch, as I retried later on (but can't find that branch in my fork yet). main...cyliang368:grass:pytest_gunittest https://github.com/cyliang368/grass/blob/efd60912963a4301b02c4e16ae6673b9fa1e6ac9/pyproject.toml#L12-L24

@echoix
Copy link
Member

echoix commented Oct 14, 2024

On Mac there is md5 available by default.

It seems there's some differences to be aware of: https://security.stackexchange.com/questions/65499/difference-between-os-x-md5-and-gnu-md5sum

@nilason
Copy link
Contributor

nilason commented Oct 14, 2024

On Mac there is md5 available by default.

It seems there's some differences to be aware of: https://security.stackexchange.com/questions/65499/difference-between-os-x-md5-and-gnu-md5sum

Of course, minor adjustments are needed, but in this case better than add a new dependency.

@nilason
Copy link
Contributor

nilason commented Oct 15, 2024

Do we want to install md5sum on macOS, replace the md5sum call with inlined Python, or disable the test on macOS? (I'm not ready to rewrite the whole test to Python, although that's an option, too.)

md5sum required, please install first

#4527 will fix this. If there would not have been file rename involved I would have added it here.

@nilason
Copy link
Contributor

nilason commented Oct 15, 2024

Test now passes on Mac too!

@wenzeslaus
Copy link
Member Author

Thanks @nilason!

@echoix If that's okay with you, I will go with the current text in the readme and you can adjust it later. ...or we can revise this further. I was just trying to quick fix a top-level directory content while getting the one automated test there running.

@wenzeslaus wenzeslaus enabled auto-merge (squash) October 15, 2024 15:14
@echoix
Copy link
Member

echoix commented Oct 15, 2024

It's ok, as technically, that's what we would want it to be at one point.

@wenzeslaus wenzeslaus merged commit 9a79aa4 into OSGeo:main Oct 15, 2024
26 checks passed
@wenzeslaus wenzeslaus deleted the revise-root-testsuite-dir branch October 15, 2024 23:09
@github-actions github-actions bot added this to the 8.5.0 milestone Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs libraries Python Related code is in Python tests Related to Test Suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants