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

v.what.rast3: Rename test folder and fix execution of test #3362

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Jan 14, 2024

Following @neteler's comment #3358 (comment), I started to explore to rename and include older tests in our CI, since they exist but weren't checked.

I started with one, it got picked up but errored out. I found that I had to place the inputs and golden copies into the data folder, so they get copied by gunittest. Then the test didn't error out. But if I changed the values in the golden copy, the test still passed. Digging up a little, I found one of the existing legacy tests scripts that used a pattern using diff and sets an exit code if the diff isn't empty. I tried it and works correctly.

Since I didn't have any time left, I started with just this one to have feedback on the pattern. I also wanted to test out if the changes were small enough that GitHub understood the rename with changes and would still allow to follow the file history, and by merging a squashed PR in my fork, yes it does. So the changes are enough to be in a single PR, yay!

So, this is kind of a WIP since it's only one, and I should probably find how to make #3358 fail too if the output isn't correct. If you'd like to merge this one this week before I get to the rest next weekend, just change the title to remove [WIP], and this PR is still valid standalone. If not, I'll continue it next weekend, if I can get to it :)

@github-actions github-actions bot added vector Related to vector data processing module labels Jan 14, 2024
@neteler neteler added this to the 8.4.0 milestone Jan 15, 2024
@echoix
Copy link
Member Author

echoix commented Jan 18, 2024

Is there any comments on this pattern before I apply it many other times this weekend? Or is it good like this and could continue without problems?

@neteler
Copy link
Member

neteler commented Jan 19, 2024

To me it looks like a good step forward to streamline the test folder names.

@echoix
Copy link
Member Author

echoix commented Jan 22, 2024

I didn't forget it, I ended up working on 2-3 big upcoming changes that configured code coverage for the repo, in order to answer ourselves: does it really matter adding this. And it seems for v.what.rast3d yes. But from the list of other test_suite folders, it's not so simple. I think only two have reference data like v.what.rast3d, but some of them already have Python tests. The code coverage is useful to see if it would be redundant or not, if it really brings something or not. But I'm not finished polishing out the quirks of the config to have it rolled out smoothly here, right on the first time.

For the other uses of test_suite, some might be harder to port, and some don't make sense (one is only another c program with a makefile and empty html. What should I do with it).

@echoix echoix changed the title v.what.rast3: [WIP] Rename test folder and fix execution of test v.what.rast3: Rename test folder and fix execution of test Jan 22, 2024
@echoix
Copy link
Member Author

echoix commented Jan 22, 2024

So if you'd like, you can merge this as is and when I figure out something better for the others like r.series, I'll just do a follow up. There wasn't enough of a pattern here.

Copy link
Member

@ninsbl ninsbl left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Activating tests makes sense, even if they are "just" shell scripts (we have other tests in shell).

vector/v.what.rast3/testsuite/test.v.what.rast3.sh Outdated Show resolved Hide resolved
@ninsbl
Copy link
Member

ninsbl commented Jan 23, 2024

I didn't forget it, I ended up working on 2-3 big upcoming changes that configured code coverage for the repo, in order to answer ourselves: does it really matter adding this. And it seems for v.what.rast3d yes. But from the list of other test_suite folders, it's not so simple. I think only two have reference data like v.what.rast3d, but some of them already have Python tests. The code coverage is useful to see if it would be redundant or not, if it really brings something or not. But I'm not finished polishing out the quirks of the config to have it rolled out smoothly here, right on the first time.

For some areas of the code we do have tests both in shell and Python. So re-activating shell tests should be fine, Just making sure that a command runs as it is supposed to is better than no test, I guess.

For the other uses of test_suite, some might be harder to port, and some don't make sense (one is only another c program with a makefile and empty html. What should I do with it).

Tackling them on a case by case basis is probably the best way to proceed, It may be more efficient to copy and adjust python tests from similar modules than porting shell tests, but that depends...

In any case good to see improvements in the testsuite!

@echoix echoix enabled auto-merge (squash) January 26, 2024 04:47
@echoix echoix merged commit f7522f8 into OSGeo:main Jan 26, 2024
23 checks passed
@echoix echoix deleted the rename-test_suite branch January 27, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants