Many C API functions lack tests

libgeos / geos

Geometry Engine, Open Source

https://libgeos.org

GNU Lesser General Public License v2.1

1.21k stars 362 forks source link

Many C API functions lack tests #396

Closed dbaston closed 1 year ago

dbaston commented 3 years ago

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)

jericks commented 3 years ago

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

dbaston commented 3 years ago

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

jericks commented 3 years ago

I started by writing tests for GEOSDisjoint and GEOSTouches:

https://github.com/libgeos/geos/compare/master...jericks:codecov

dbaston commented 3 years ago

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 commented 3 years ago

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

dbaston commented 3 years ago

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 commented 3 years ago

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

jericks commented 3 years ago

No objections here.

On Wed, Oct 20, 2021, 4:19 PM Dan Baston @.***> wrote:

@jericks https://github.com/jericks , any objection to me merging in the commits from your codecov branch?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/libgeos/geos/issues/396#issuecomment-948107222, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABUKTYA3OGLKJJUCCWO4V3UH5FBHANCNFSM4W5CK7QQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

dbaston commented 2 years ago

Still seems a worthy goal, IMO.

dbaston commented 1 year ago

This is in pretty good shape now.