From 666d6f818986f4e0a0fa855f468e849debc7ade1 Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Thu, 29 Oct 2020 08:57:26 -0400 Subject: [PATCH] fix tests that fail for the wrong reasons 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. --- pom.xml | 2 +- .../github/v3/clients/GitHubClient.java | 14 +++++- .../github/v3/clients/GitHubAuthTest.java | 44 +++++++++++++++---- 3 files changed, 49 insertions(+), 11 deletions(-) diff --git a/pom.xml b/pom.xml index b9162179..873c5a6d 100644 --- a/pom.xml +++ b/pom.xml @@ -92,7 +92,7 @@ 28.0-jre 3.0.2 1.1.1 - 4.13 + 4.13.1 1.7.12 3.2.4 2.6 diff --git a/src/main/java/com/spotify/github/v3/clients/GitHubClient.java b/src/main/java/com/spotify/github/v3/clients/GitHubClient.java index d8bacb04..1bd3957d 100644 --- a/src/main/java/com/spotify/github/v3/clients/GitHubClient.java +++ b/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); diff --git a/src/test/java/com/spotify/github/v3/clients/GitHubAuthTest.java b/src/test/java/com/spotify/github/v3/clients/GitHubAuthTest.java index 71a210c7..2996e8a6 100644 --- a/src/test/java/com/spotify/github/v3/clients/GitHubAuthTest.java +++ b/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