Converting Integration tests to Local Server tests#278
Converting Integration tests to Local Server tests#278domesticmouse merged 19 commits intogooglemaps:okhttp3from domesticmouse:okhttp3
Conversation
Initial sketch of how I am thinking about crafting non integration tests out of the current integration test suite.
domesticmouse
changed the title
Also converted LocalTestServerContext to Autocloseable, so now every usage uses try-with-resources to force timely closing of sockets.
markmcd
left a comment
markmcd
left a comment
There was a problem hiding this comment.
This is great. I like the try-with-resources local context thingo. Ideally we would not need to use the mock server in the middle and could just provide a transport interface but that's one for the TODO list I think.
Only concerns are:
- There are some floating point comparisons that assume certain precision mismatches, I could see these failing when run on different chips. Ideally we'd use assertEquals with an epsilon but if they come from strings in your serialised responses then maybe it's a non-issue.
- These tests are only really testing that responses are deserialised into objects correctly. Given we're not doing it implicitly anymore, perhaps in the future we could add tests to ensure that objects are serialised correctly to outgoing requests, as well as the reverse.
I like the general structure too - I can imagine a future refactor where we use annotations or something to describe which in/out JSON files to use per-test, but that's one for another day ;)
| import com.google.maps.model.TransitRoutingPreference; | ||
| import com.google.maps.model.TravelMode; | ||
| import com.google.maps.model.Unit; | ||
| import java.util.ArrayList; |
There was a problem hiding this comment.
nit: this looks like it's in the wrong place
There was a problem hiding this comment.
I'd agree, but google-java-format seems happy with it.
|
|
||
| package com.google.maps; | ||
|
|
||
| import static org.junit.Assert.assertEquals; |
There was a problem hiding this comment.
nit: import ordering should follow style guide at go/javastyle (you should be able to set this in your IDE too)
There was a problem hiding this comment.
Turns out there is tooling for it now...
| assertThat(result.routes[0].overviewPolyline.decodePath().size(), not(0)); | ||
| assertTrue(result.routes[0].legs[0].startAddress.startsWith("Sydney NSW")); | ||
| assertTrue(result.routes[0].legs[0].endAddress.startsWith("Melbourne VIC")); | ||
| try (LocalTestServerContext sc = new LocalTestServerContext(getDirectionsResponse)) { |
| assertNotNull(result.routes[0].legs[0]); | ||
| assertNotNull(result.routes[0].legs[0].arrivalTime); | ||
| assertNotNull(result.routes[0].legs[0].departureTime); | ||
| try(LocalTestServerContext sc = new LocalTestServerContext(responseTimesArePopulatedCorrectly)) { |
There was a problem hiding this comment.
nit: space after try (there are a bunch of these)
|
|
||
| assertNotNull(result.routes); | ||
| try(LocalTestServerContext sc = new LocalTestServerContext( | ||
| "{\"routes\": [{}],\"status\": \"OK\"}")){ |
There was a problem hiding this comment.
one day java will support multiline here-strings 😩
| } | ||
|
|
||
| @Test | ||
| public void testAsync() throws Exception { |
There was a problem hiding this comment.
This is a weird test to have in GeocodingApiTests - maybe it belongs it a context or pending result test? Not particularly relevant to your change though so feel free to ignore.
There was a problem hiding this comment.
I'll have some de-dupe and re-org work to do on this test suite next Q
2 participants
Initial sketch of how I am thinking about crafting non integration tests out of the current integration test suite.
PTAL @markmcd