Tests For Geocoder

[ihcsim]

Hi Kelly,

I notice that the test coverage for the *Geocoder is a bit low. Will you be interested in a PR where I refactor the Geocode() and ReverseGeocode() methods to mock out the API calls.

For example, in GoogleGeocoder, I update the struct to have a field function:

type GoogleGeocoder struct {
    sendRequest func(params string) ([]byte, error)
}

Then I changed the Geocode() method to call g.sendRequest() like this:

func (g *GoogleGeocoder) Geocode(address string) (*Point, error) {
    if g.sendRequest == nil {   // this can go into the struct constructor
        g.sendRequest = g.Request
    }

    data, err := g.sendRequest(queryStr)
    if err != nil {
        return nil, err
    }

This will makes it easier to test these methods. For example, I can mock the sendRequest field in the test like this:

func TestGeocode(t *testing.T) {
  address := "1600 Amphitheatre Parkway, Mountain View, CA 94043, USA"
  g := GoogleGeocoder{
    sendRequest: func(params string) ([]byte, error) {
      return []byte(`{
        "results":[
          {"formatted_address":"1600 Amphitheatre Parkway, Mountain View, CA 94043, USA",
            "geometry":{"location":{
              "lat":37.4224764,
              "lng":-122.0842499}
            }
          }
        ],
        "status":"OK"
      }`), nil
    },
  }
  actual, err := g.Geocode(address)
  if err != nil {
    t.Error("Unexpected error: ", err)
  }

  expected := &Point{lat: 37.4224764, lng: -122.0842499}
  if !assertPointsEqual(actual, expected, 4) {
    t.Errorf("Expected point to be %+v, but got %+v", expected, actual)
  }
}

Obviously, this can be done for all the different types of Geocoders. With this change, existing tests continue to pass, with test coverage for the Geocoders increased from around 30% to 78%.

WDYT?

[kellydunn]

Heya @ihcsim !

Thanks so much for chiming in on how to increase the code coverage of golang-geo! I think your idea is definitely along the correct path!

One thing I really like about go and testing is when libraries provide well-defined interfaces that also serve well for testing purposes. For example, the io.Writer Interface is super neat when you want to mock out writing bytes, like so:

type MockWriter struct {}

func (m MockWriter) Write(data []byte) (n int, err error) {
  // Have your mocked functionality here
}

In that regard, I think I'd like to see something along the lines of having the Geocoder Interface supplying the ability to mock out HTTP requests with more standard golang libraries. Perhaps something like:

type Geocoder interface {
  Geocode(string) Point
  ReverseGeocode(lat, lng) string
  HttpClient() *http.Client
}

This way, we can set the http client in our tests and use the httptest package to create mock servers that send us back the responses we want to test.

What do you think about that approach?

[ihcsim]

@kellydunn I like your approach. I will give it a try, and get back to you.

[ihcsim]

@kellydunn Following your thoughts on using the httptest package to create mock servers, it feels like the only part that we need to mock out is really just this googleGeocodeURL variable, where we replace it with the mock server URL.

Expanding the Geocoder interface with a HttpClient() probably won't help, because the client will still try to hit the Google API. Also, I feel the current Geocoder interface is very clean in that it only has functions that its name clearly communicates.

Alternately (even this doesn't feel fully right), we can consider expanding the Geocoder interface to:

type Geocoder interface {
  Geocode(string) Point
  ReverseGeocode(lat, lng) string
  Request(params string) ([]byte, error)
}

The test coverage will be lower (since we mock out the Request() function in tests), but the interface reads as a Geocoder can:

1. Geo-code an address
2. Reverse geo-code a coordinate
3. Send a request

WDYT?