Refactored GitHub api layer for easier and more flexible testing #875

begedin · 2017-08-30T15:53:57Z

What's in this PR?

This PR ought to perform the refactor of the github layer for easier and more flexible mocking.

It cleans up the CodeCorps.Github namespace, adds a mocking macro and infrastructure and refactors code so the part that needs to be mocked is minimal.

It also starts converting tests from bypass to using the mocking macro, with some examples already in, and a bunch left to do.

Ideally, what we'll end up with is most test using the default mock module, some specific tests using custom modules and the only place bypass will be used is in tests/lib/codecorps/github/api_test.exs

I'll comment on the code as soon as I can.

begedin · 2017-08-30T19:35:34Z

lib/code_corps/github/api.ex

+  defp marshall_response({:error, reason}) do
+    {:error, HTTPClientError.new(reason: reason)}
+  end
+end


This should be the only place where requests to github are actually made, and even then, we should keep it as flat as simple as possible.

We need to write a test module for this file to, and it should be the only one to use bypass.

begedin · 2017-08-30T19:35:54Z

lib/code_corps/github/bypass.ex

-    Application.delete_env(:code_corps, :github_base_url)
-    Application.delete_env(:code_corps, :github_oauth_url)
-  end
-end


Since it's only used from tests, I figured it makes more sense to place it inside the tests folder

begedin · 2017-08-30T19:36:42Z

lib/code_corps/github/github.ex

@@ -1,6 +1,5 @@
 defmodule CodeCorps.GitHub do


This file is split out a bit. Parts have been moved to github/api.ex and parts to github/headers.ex. Both these files probably need some form or testing, api more so.

begedin · 2017-08-30T19:38:09Z

lib/code_corps/github/installation.ex

@@ -61,7 +61,7 @@ defmodule CodeCorps.GitHub.Installation do
  def refresh_token(%GithubAppInstallation{github_id: installation_id} = installation) do
    endpoint = "installations/#{installation_id}/access_tokens"
    with {:ok, %{"token" => token, "expires_at" => expires_at}} <-
-           GitHub.authenticated_integration_request(%{}, :post, endpoint, %{}, []),
+           GitHub.integration_request(:post, endpoint, %{}, %{}, []),


I went with the method, url, headers, body, options order of arguments for all request functions where it applies. My reasoning is that :hackney uses the same ordering.

begedin · 2017-08-30T19:39:45Z

test/lib/code_corps/github/event/installation/repos_test.exs

+    def request(method, endpoint, headers, body, options) do
+      GitHubCase.SuccessAPI.request(method, endpoint, headers, body, options)
+    end
+  end


This is an example of using mock modules to replace the api layer temporarily for specific tests.

The code being tested here makes two requests and the tests specify what happens when one of them fails in a specific way.

We've defined modules for those two failure cases where the method signature which ought to fail, fails in that specific way, while all other method signatures delegate to the default GitHubCase.SuccessAPI module.

begedin · 2017-08-30T19:41:27Z

test/lib/code_corps/github/event/installation/repos_test.exs

+
+      with_mock_api(BadRepoRequest) do
+        {:error, %GithubAppInstallation{state: state}, %CodeCorps.GitHub.APIError{}} = Repos.process(installation)
+      end


This is how we use the macro to mock out an api request.

With that done, the bypass tag is no longer needed. Our goal in this PR is to remove all existing bypass tags and switch to with_mock_api.

While doing that, we will fill out the default SuccessAPI module with default responses.

Something to consider is that it may also make sense to define a default FailureAPI if we see enough code being repeated.

begedin · 2017-08-30T19:42:26Z

test/lib/code_corps_web/controllers/github_events_controller_test.exs

-  @tag bypass: %{
-    "/installation/repositories" => {200, @installation_repositories},
-    "/installations/#{@installation_created["installation"]["id"]}/access_tokens" => {200, @access_token_create_response}
-  }
  test "responds with 200 for a supported event", %{conn: conn} do


This is an example of simply taking away the bypass tag. The default SuccessAPI is enough to keep the tests passing.

begedin · 2017-08-30T19:43:43Z

test/support/github_case/success_api.ex

+    %{"access_token" => "foo_auth_token"}
+  end
+  defp mock_response(method, "https://api.github.com/" <> endpoint, headers, body, options) do
+    mock_api_response(method, endpoint |> String.split("/"), headers, body, options)


There can be dynamic parts to a request url, so this is how we deal with those. We can't pattern match a string when a dynamic part is not the last part of the string, so we split it into a list of parts and pattern match the list in a subfunction

begedin · 2017-08-30T19:44:21Z

test/support/github_case/success_api.ex

+      "token" => "v1.1f699f1069f60xxx",
+      "expires_at" => Timex.now() |> Timex.shift(hours: 1) |> DateTime.to_iso8601
+    }
+  end


Further requests can be added here. We should not have a catch-all request, since an umatched clause is the quickest way to detect that our SuccessAPI isn't complete

begedin · 2017-08-30T19:46:28Z

TODO LIST

Fix existing tests by switching them away from tag bypass: to with_mock_api
Add tests for CodeCorps.GitHub.API and CodeCorps.GitHub.Headers. The first is the complex one and should be the only one that still uses bypass (or we could even consider ExVCR for it. The second is far less complex in comparison.
Add any tests that may still be required for CodeCorps.GitHub

- removed bypass from all tests except lib/code_corps/github/api_test.exs - introduced `with_mock_api` macro to mock out an api layer module for usage in misc github tests - implementd proper bypass tests in `api_test` - removed github_case since it's bypass facilities are now only existing in `api_test` - fixed several minor bugs and issues revealed through this process

joshsmith

Looks great! Merge at will!

begedin commented Aug 30, 2017

View reviewed changes

joshsmith force-pushed the refactor-github-api-layer branch from 3d2a97d to 6c099a0 Compare September 2, 2017 23:40

begedin force-pushed the refactor-github-api-layer branch 2 times, most recently from 114f1e0 to b974245 Compare September 4, 2017 12:18

begedin changed the title ~~Refactor github - Initial commit~~ Refactored GitHub api layer for easier and more flexible testing Sep 4, 2017

begedin force-pushed the refactor-github-api-layer branch from b974245 to 833dce1 Compare September 4, 2017 13:11

joshsmith approved these changes Sep 4, 2017

View reviewed changes

joshsmith merged commit 32ce8ef into develop Sep 4, 2017

joshsmith deleted the refactor-github-api-layer branch September 4, 2017 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored GitHub api layer for easier and more flexible testing #875

Refactored GitHub api layer for easier and more flexible testing #875

begedin commented Aug 30, 2017

begedin Aug 30, 2017

begedin Aug 30, 2017

begedin Aug 30, 2017

begedin Aug 30, 2017

begedin Aug 30, 2017

begedin Aug 30, 2017

begedin Aug 30, 2017

begedin Aug 30, 2017

begedin Aug 30, 2017

begedin commented Aug 30, 2017 •

edited

Loading

joshsmith left a comment

Refactored GitHub api layer for easier and more flexible testing #875

Refactored GitHub api layer for easier and more flexible testing #875

Conversation

begedin commented Aug 30, 2017

What's in this PR?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begedin commented Aug 30, 2017 • edited Loading

TODO LIST

joshsmith left a comment

Choose a reason for hiding this comment

begedin commented Aug 30, 2017 •

edited

Loading