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

Many C API functions lack tests #396

Closed
dbaston opened this issue Feb 1, 2021 · 10 comments
Closed

Many C API functions lack tests #396

dbaston opened this issue Feb 1, 2021 · 10 comments
Labels
Good First Issue New developers consider for experience.

Comments

@dbaston
Copy link
Member

dbaston commented Feb 1, 2021

Functions and code paths not tested can be seen in this codecov report: https://codecov.io/gh/libgeos/geos/src/master/capi/geos_ts_c.cpp. You should be able to view it without an account at least once, but it seems to require login after a few page views.

This would be a great task for anyone who wants to begin contributing to GEOS.

The behavior of the underlying operations is generally tested elsewhere, but C API tests should at least verify that

  • the function correctly operates with trivial inputs
  • it preserves SRID, if applicable
  • it returns the documented error code on error (if you can think of a way to trigger an error)
@dbaston dbaston added the Good First Issue New developers consider for experience. label Feb 1, 2021
@jericks
Copy link
Contributor

jericks commented Mar 26, 2021

I would like to help out with this. Would you prefer one big pull request or several smaller pull requests (one for function)?

@dbaston
Copy link
Member Author

dbaston commented Mar 26, 2021

However you prefer to do it is good by me. Thanks!

@jericks
Copy link
Contributor

jericks commented Mar 27, 2021

I started by writing tests for GEOSDisjoint and GEOSTouches:

master...jericks:codecov

@dbaston
Copy link
Member Author

dbaston commented Mar 27, 2021

Thanks! Here are my suggestions:

  • instead of creating local variables for the test geometries, use the built-in member variables geom1_, geom2_, etc. These will be automatically cleaned up, so you don't need to call GEOSGeom_destroy. You can peek at the capitest::utility class to see a full list.
  • if you can test the necessary code paths with two geometries instead of three, that's better
  • if you can test the necessary code paths with easily-readable geometries like POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0)), that's better

@jericks
Copy link
Contributor

jericks commented Apr 4, 2021

To test all the *_r functions should add a struct utility_r to the capi_test_utils?

@dbaston
Copy link
Member Author

dbaston commented Apr 4, 2021

I see the appeal but am not sure it's worth it, since the non-_r functions are just trivial wrappers around the _r versions.

@dbaston
Copy link
Member Author

dbaston commented Oct 20, 2021

@jericks , any objection to me merging in the commits from your codecov branch?

@jericks
Copy link
Contributor

jericks commented Oct 21, 2021 via email

strk pushed a commit that referenced this issue Oct 21, 2021
@pramsey pramsey closed this as completed Nov 26, 2021
@dbaston
Copy link
Member Author

dbaston commented Nov 26, 2021

Still seems a worthy goal, IMO.

@dbaston
Copy link
Member Author

dbaston commented Jan 4, 2023

This is in pretty good shape now.

image

@dbaston dbaston closed this as completed Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue New developers consider for experience.
Projects
None yet
Development

No branches or pull requests

3 participants