fix tests that fail for the wrong reasons (#45)

I noticed that the test `assertJwtEndpointWithNoKeyThrows()` was taking
10 seconds to run so I took a look into why that was.

The test method has `@Test(expected=Exception.class)` but the exception
that is thrown is different than the one that the test is expected - the
test was throwing a SocketTimeoutException after 10 seconds (which is
the OkHttpClient's default read timeout setting) because there was no
mock requests enqueued in the mock web server.

I refactored these tests to use `assertThrows` instead to assert against
the details of the thrown Exceptions, and also fixed what seems like a
bug in the `getAuthorizationHeader` method - if the client has an
accessToken, it will be used as the header, even if the request path is
one that needs a private-key-signed JWT token.

Author: mattnworb

pom.xml

@@ -92,7 +92,7 @@
         <guava.version>28.0-jre</guava.version>
         <jsr305.version>3.0.2</jsr305.version>
         <jsr311-api.version>1.1.1</jsr311-api.version>
-        <junit.version>4.13</junit.version>
+        <junit.version>4.13.1</junit.version>
         <slf4j-api.version>1.7.12</slf4j-api.version>
         <mockito-core.version>3.2.4</mockito-core.version>
         <commons-io.version>2.6</commons-io.version>

src/main/java/com/spotify/github/v3/clients/GitHubClient.java

@@ -542,6 +542,9 @@ private Request.Builder requestBuilder(final String path) {
    (3) Installation Token, generated from the JWT token. Also used in Github Apps.
   */
   private String getAuthorizationHeader(final String path) {
+    if (isJwtRequest(path) && getPrivateKey().isEmpty()) {
+      throw new IllegalStateException("This endpoint needs a client with a private key for an App");
+    }
     if (getAccessToken().isPresent()) {
       return String.format("token %s", token);
     } else if (getPrivateKey().isPresent()) {
@@ -603,12 +606,19 @@ private AccessToken generateInstallationToken(final String jwtToken, final int i
 
     final Response response = client.newCall(request).execute();
 
-    if (response.body() == null) {
+    if (!response.isSuccessful()) {
       throw new Exception(
           String.format(
-              "Got status %s when getting an access token from GitHub: %s",
+              "Got non-2xx status %s when getting an access token from GitHub: %s",
               response.code(), response.message()));
     }
+
+    if (response.body() == null) {
+      throw new Exception(
+          String.format(
+              "Got empty response body when getting an access token from GitHub, HTTP status was: %s",
+              response.message()));
+    }
     final String text = response.body().string();
     response.body().close();
     return Json.create().fromJson(text, AccessToken.class);

src/test/java/com/spotify/github/v3/clients/GitHubAuthTest.java

@@ -22,9 +22,12 @@
 
 import static com.spotify.github.v3.clients.ChecksClientTest.loadResource;
 import static org.hamcrest.CoreMatchers.hasItem;
+import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.notNullValue;
 import static org.hamcrest.CoreMatchers.startsWith;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertThrows;
 
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.spotify.github.jackson.Json;
@@ -33,8 +36,10 @@
 import java.io.File;
 import java.io.IOException;
 import java.net.URI;
+import java.time.Duration;
 import java.time.ZonedDateTime;
 import java.util.Objects;
+import java.util.concurrent.TimeUnit;
 import okhttp3.OkHttpClient;
 import okhttp3.mockwebserver.MockResponse;
 import okhttp3.mockwebserver.MockWebServer;
@@ -72,7 +77,12 @@ public GitHubAuthTest() throws JsonProcessingException {}
 
   @Before
   public void setUp() throws IOException {
-    client = new OkHttpClient();
+    client =
+        new OkHttpClient.Builder()
+            .connectTimeout(Duration.ofSeconds(1))
+            .readTimeout(Duration.ofSeconds(1))
+            .build();
+
     mockServer.start();
     url = mockServer.url("/").uri();
     checksClient =
@@ -139,10 +149,20 @@ public void fetchesANewInstallationTokenIfExpired() throws Exception {
     assertThat(mockServer.takeRequest().getPath(), is("/repos/foo/bar/check-runs/123"));
   }
 
-  @Test(expected = Exception.class)
-  public void throwsIfFetchingInstallationTokenRequestIsUnsuccessful() {
+  @Test
+  public void throwsIfFetchingInstallationTokenRequestIsUnsuccessful() throws Exception {
     mockServer.enqueue(new MockResponse().setResponseCode(500));
-    checksClient.getCheckRun(123).join();
+    RuntimeException ex =
+        assertThrows(RuntimeException.class, () -> checksClient.getCheckRun(123).join());
+
+    assertThat(ex.getMessage(), is("Could not generate access token for github app"));
+
+    assertThat(ex.getCause(), is(notNullValue()));
+    assertThat(ex.getCause().getMessage(), startsWith("Got non-2xx status 500 when getting an access token from GitHub"));
+
+    RecordedRequest recordedRequest = mockServer.takeRequest(1, TimeUnit.MILLISECONDS);
+    // make sure it was the expected request that threw
+    assertThat(recordedRequest.getRequestUrl().encodedPath(), is("/app/installations/1/access_tokens"));
   }
 
   @Test
@@ -178,16 +198,24 @@ public void assertChecksApiContainsCorrectHeader() throws Exception {
     assertThat(request2.getMethod(), is("PATCH"));
   }
 
-  @Test(expected = Exception.class)
+  @Test
   public void assertInstallationEndpointWithoutInstallationThrows() {
     final GitHubClient github = GitHubClient.create(client, url, key, 123);
-    github.createRepositoryClient("foo", "bar").createChecksApiClient().getCheckRun(123).join();
+    final RuntimeException ex = assertThrows(RuntimeException.class,
+        () -> github.createRepositoryClient("foo", "bar").createChecksApiClient().getCheckRun(123)
+            .join());
+    assertThat(ex.getMessage(), is("This endpoint needs a client with an installation ID"));
   }
 
-  @Test(expected = Exception.class)
+  @Test
   public void assertJwtEndpointWithNoKeyThrows() {
     final GitHubClient github = GitHubClient.create(client, url, "a-token");
-    github.createRepositoryClient("foo", "bar").createGithubAppClient().getInstallations().join();
+
+    final IllegalStateException ex = assertThrows(IllegalStateException.class,
+        () -> github.createRepositoryClient("foo", "bar").createGithubAppClient().getInstallations()
+            .join());
+
+    assertThat(ex.getMessage(), is("This endpoint needs a client with a private key for an App"));
   }
 
   @Test