GNOME Bugzilla – Bug 774631
Add a mock backend implementation
Last modified: 2017-01-21 00:06:30 UTC
Following on from the introduction of GeocodeBackend in bug #756311, here’s an implementation of a mock backend, GeocodeMockBackend. This is an implementation of GeocodeBackend which returns results from an in-memory map of parameters to result sets, for both forward and reverse queries. That map is set up using API exposed by the GeocodeMockBackend — so a unit test harness for an application or library which uses geocode-glib can create a GeocodeMockBackend, set up some expected queries in it, then test the application or library using the mock GeocodeBackend instance. This allows, for example, unit tests to be written which do not require internet access, which is useful for many build and CI machines. Secondly, it allows unit tests to reliably test the error paths in an application or library’s code, as the map of queries can include error values. This allows testing an application’s handling of network timeouts, for example, which is otherwise tricky to set up (it requires a network namespace and a mock server).
Created attachment 340145 [details] [review] tests: Drop unnecessary g_type_init() call We now depend on a more recent version of GLib than the one which deprecated it, so we can drop the call entirely.
Created attachment 340146 [details] [review] bounding-box: Add equality method Add a method for checking that two #GeocodeBoundingBox instances are equal. This will be used in upcoming commits.
Created attachment 340147 [details] [review] location: Add equality method Add a method for checking that two #GeocodeLocation instances are equal. This will be used in upcoming commits.
Created attachment 340148 [details] [review] place: Add equality method Add a method for checking that two #GeocodePlace instances are equal. This will be used in upcoming commits.
Created attachment 340149 [details] [review] forward: Restrict answer count to be positive Require it to be a positive integer. This is technically an API break, but any code which used a non-positive integer before would not have worked anyway.
Created attachment 340150 [details] [review] docs: Clarify error returns from GeocodeBackend These are a bit inconsistent, especially in the case of returning no results (which I would expect to return an empty result list, rather than an error), but are what the Nominatim code has always done, so we have to keep that API for backwards compatibility.
Created attachment 340151 [details] [review] reverse: Fix type of lat/lon parameters to backend reverse query method The backend methods are documented as taking parameters which follow the Telepathy Location mapping (https://telepathy.freedesktop.org/spec/Connection_Interface_Location.html#Mapping:Location), or alternatively XEP-0080. Both of these specify that the ‘lat’ and ‘lon’ parameters are numeric, not numeric strings. Since the GeocodeBackend interface has not been released yet, fix the implementation to use a GValue holding a gdouble, rather than a GValue holding a string representation of a double.
Created attachment 340152 [details] [review] mock-backend: Add a mock backed implementation This is an implementation of GeocodeBackend which returns results from an in-memory map of parameters to result sets, for both forward and reverse queries. That map is set up using API exposed by the GeocodeMockBackend — so a unit test harness for an application or library which uses geocode-glib can create a GeocodeMockBackend, set up some expected queries in it, then test the application or library using the mock GeocodeBackend instance. This allows, for example, unit tests to be written which do not require internet access, which is useful for many build and CI machines. Secondly, it allows unit tests to reliably test the error paths in an application or library’s code, as the map of queries can include error values. This allows testing an application’s handling of network timeouts, for example, which is otherwise tricky to set up (it requires a network namespace and a mock server). The GeocodeMockBackend is public API. Unit tests for it will be added in a follow-up commit.
Created attachment 340153 [details] [review] tests: Add tests for GeocodeMockBackend
Created attachment 340154 [details] [review] backend: Add introductory docs for GeocodeBackend Add a longer introduction section explaining why backends exist, and what backends do exist.
Review of attachment 340145 [details] [review]: Yep.
Review of attachment 340146 [details] [review]: Sure.
Review of attachment 340147 [details] [review]: ::: geocode-glib/geocode-location.c @@ +140,3 @@ + g_return_val_if_fail (GEOCODE_IS_LOCATION (b), FALSE); + + return (a->priv->longitude == b->priv->longitude && Should this include a delta? Direct comparisons of double/floats are usually pretty difficult. @@ +144,3 @@ + a->priv->altitude == b->priv->altitude && + a->priv->accuracy == b->priv->accuracy && + a->priv->timestamp == b->priv->timestamp && You're sure this is needed? I'd also mention it in the API doc after the example about the locations not matching.
Review of attachment 340148 [details] [review]: ::: geocode-glib/geocode-place.c @@ +654,3 @@ + return (g_strcmp0 (a->priv->name, b->priv->name) == 0 && + a->priv->place_type == b->priv->place_type && + ((a->priv->location == NULL && b->priv->location == NULL) || This is difficult to read, can you split off the location and bbox equality checks into separate functions? Make them inline if you think this is going to make a difference, and let the compiler optimise that.
Review of attachment 340149 [details] [review]: Sure.
Review of attachment 340150 [details] [review]: ::: geocode-glib/geocode-backend.c @@ +233,3 @@ * Gets the result of a reverse geocoding query using the @backend. * + * If no result could be found, a %GEOCODE_ERROR_NOT_SUPPORTED error will be Might be a good idea to use something other than NOT_SUPPORTED here, right? NO_MATCHES looks good as well. Separate patch to change that?
Review of attachment 340151 [details] [review]: Sure.
Review of attachment 340152 [details] [review]: That looks fine. ::: geocode-glib/geocode-mock-backend.c @@ +1,2 @@ +/* + Copyright (C) 2016 Collabora Ltd. "*" ::: geocode-glib/geocode-mock-backend.h @@ +1,2 @@ +/* + Copyright (C) 2016 Collabora Ltd. "*"
Review of attachment 340153 [details] [review]: Top test data :) ::: geocode-glib/tests/mock-backend.c @@ +1,2 @@ +/* + Copyright (C) 2016 Collabora Ltd. "*" @@ +48,3 @@ + place_b = GEOCODE_PLACE (b->data); + + g_assert (place_a != NULL && place_b != NULL); Separate assertions with g_assert_nonnull()? @@ +52,3 @@ + } + + g_assert (a == NULL && b == NULL); Separate assertions again?
Review of attachment 340154 [details] [review]: Yep.
Review of attachment 340150 [details] [review]: ::: geocode-glib/geocode-backend.c @@ +233,3 @@ * Gets the result of a reverse geocoding query using the @backend. * + * If no result could be found, a %GEOCODE_ERROR_NOT_SUPPORTED error will be As the commit message says: this is what the code has always done. It would be an API break to change the error code it returns now. :-( (Unless we considered the GeocodeBackend API to be separate from the old GeocodeReverse API, changed the error code in GeocodeBackend, and put some remapping code in GeocodeReverse to ensure there’s no API break there? I suspect that would be unnecessarily confusing though.)
Thanks for the reviews; I agree with all your comments and will address them all just as soon as you’ve reviewed the patches on bug #772928 which this bug’s branch depends on. :-)
Attachment 340145 [details] pushed as 2c13145 - tests: Drop unnecessary g_type_init() call Attachment 340149 [details] pushed as 2a10522 - forward: Restrict answer count to be positive Attachment 340150 [details] pushed as 7ec672c - docs: Clarify error returns from GeocodeBackend Attachment 340151 [details] pushed as c104273 - reverse: Fix type of lat/lon parameters to backend reverse query method
New patches coming shortly. (In reply to Bastien Nocera from comment #13) > Review of attachment 340147 [details] [review] [review]: > > ::: geocode-glib/geocode-location.c > @@ +140,3 @@ > + g_return_val_if_fail (GEOCODE_IS_LOCATION (b), FALSE); > + > + return (a->priv->longitude == b->priv->longitude && > > Should this include a delta? Direct comparisons of double/floats are usually > pretty difficult. I don’t think it needs to. Comparisons of doubles/floats typically need a delta when you’re doing arithmetic with them, because (for example) summing two sets of numbers to produce the same numeric result might produce two different binary floating point representations, so they can’t be compared bitwise. However, the typical use case here is for locations to be copied around, rather than fiddled with arithmetic, so a bitwise comparison is OK. Flipping it around slightly, what delta is a reasonable one here? 1m? 1mm? 1µm? > @@ +144,3 @@ > + a->priv->altitude == b->priv->altitude && > + a->priv->accuracy == b->priv->accuracy && > + a->priv->timestamp == b->priv->timestamp && > > You're sure this is needed? I'd also mention it in the API doc after the > example about the locations not matching. Yes, for consistency it’s needed. If people want to compare locations by just their latitude and longitude, they can do that by using the getters (and if it’s sufficiently popular we can always add another comparison function later). (In reply to Bastien Nocera from comment #14) > Review of attachment 340148 [details] [review] [review]: > > ::: geocode-glib/geocode-place.c > @@ +654,3 @@ > + return (g_strcmp0 (a->priv->name, b->priv->name) == 0 && > + a->priv->place_type == b->priv->place_type && > + ((a->priv->location == NULL && b->priv->location == NULL) || > > This is difficult to read, can you split off the location and bbox equality > checks into separate functions? Make them inline if you think this is going > to make a difference, and let the compiler optimise that. Good point, done. (In reply to Bastien Nocera from comment #16) > Review of attachment 340150 [details] [review] [review]: > > ::: geocode-glib/geocode-backend.c > @@ +233,3 @@ > * Gets the result of a reverse geocoding query using the @backend. > * > + * If no result could be found, a %GEOCODE_ERROR_NOT_SUPPORTED error will be > > Might be a good idea to use something other than NOT_SUPPORTED here, right? > NO_MATCHES looks good as well. > Separate patch to change that? Discussed in comment #21. (In reply to Bastien Nocera from comment #18) > Review of attachment 340152 [details] [review] [review]: > > That looks fine. > > ::: geocode-glib/geocode-mock-backend.c > @@ +1,2 @@ > +/* > + Copyright (C) 2016 Collabora Ltd. > > "*" All done. (In reply to Bastien Nocera from comment #19) > Review of attachment 340153 [details] [review] [review]: > > Top test data :) :-D > @@ +48,3 @@ > + place_b = GEOCODE_PLACE (b->data); > + > + g_assert (place_a != NULL && place_b != NULL); > > Separate assertions with g_assert_nonnull()? Good point; all done.
Created attachment 343757 [details] [review] bounding-box: Add equality method Add a method for checking that two #GeocodeBoundingBox instances are equal. This will be used in upcoming commits.
Created attachment 343758 [details] [review] location: Add equality method Add a method for checking that two #GeocodeLocation instances are equal. This will be used in upcoming commits.
Created attachment 343759 [details] [review] place: Add equality method Add a method for checking that two #GeocodePlace instances are equal. This will be used in upcoming commits.
Created attachment 343760 [details] [review] mock-backend: Add a mock backed implementation This is an implementation of GeocodeBackend which returns results from an in-memory map of parameters to result sets, for both forward and reverse queries. That map is set up using API exposed by the GeocodeMockBackend — so a unit test harness for an application or library which uses geocode-glib can create a GeocodeMockBackend, set up some expected queries in it, then test the application or library using the mock GeocodeBackend instance. This allows, for example, unit tests to be written which do not require internet access, which is useful for many build and CI machines. Secondly, it allows unit tests to reliably test the error paths in an application or library’s code, as the map of queries can include error values. This allows testing an application’s handling of network timeouts, for example, which is otherwise tricky to set up (it requires a network namespace and a mock server). The GeocodeMockBackend is public API. Unit tests for it will be added in a follow-up commit.
Created attachment 343761 [details] [review] tests: Add tests for GeocodeMockBackend
Created attachment 343762 [details] [review] backend: Add introductory docs for GeocodeBackend Add a longer introduction section explaining why backends exist, and what backends do exist.
Review of attachment 343757 [details] [review]: Yep.
Review of attachment 343758 [details] [review]: Sure.
Review of attachment 343759 [details] [review]: Yep.
Review of attachment 343760 [details] [review]: Looks good.
Review of attachment 343761 [details] [review]: Great
Review of attachment 343762 [details] [review]: Nice.
Super, all fixed. Thanks for the reviews (sorry for the completely unwarranted pestering). :-) Attachment 343757 [details] pushed as 9eba9f7 - bounding-box: Add equality method Attachment 343758 [details] pushed as 473ffcb - location: Add equality method Attachment 343759 [details] pushed as a05220e - place: Add equality method Attachment 343760 [details] pushed as 1a52772 - mock-backend: Add a mock backed implementation Attachment 343761 [details] pushed as 15ba67c - tests: Add tests for GeocodeMockBackend Attachment 343762 [details] pushed as 7b16216 - backend: Add introductory docs for GeocodeBackend