GitHub pagination #1113
GitHub pagination #1113
Conversation
begedin
force-pushed
the
github-pagination
branch
from
74a53e5 to
5f92720
Compare
|
Note that if we want to figure out the eagier approach, we could still do it in a semi-streamable fashion. For example, we can get a stream of requests, then evaluate it using |
begedin
force-pushed
the
github-pagination
branch
from
7f7eb6c to
ea36be3
Compare
joshsmith
force-pushed
the
github-pagination
branch
4 times, most recently
from
a369701 to
db55359
Compare
|
|
||
| Application.put_env(:code_corps, :github, old_mock) | ||
| end | ||
| end |
There was a problem hiding this comment.
This uses the real API to test. I feel fairly confident that we should introduce some acceptance tests rather than letting our production server serve as that fallback. They should be kept to a bare minimum (like in this case testing a sync of a repository should only be done maybe when merging from staging to master).
I'm not comfortable right now, though, with the best way to do this with tags. Could use thoughts.
There was a problem hiding this comment.
Not sure what the perfect approach is, but we could make a command alias, for example mix test.acceptance, which basically calls mix test --include :acceptance
Then we setup mix test to auto-exclude tests with the tag :acceptance.
If we want to use the same test in acceptance and mock API mode, that's probably also doable, but we'll have to be creative. I'm not sure what the approach would be in that case.
|
|
||
| {:ok, issues} = Repository.issues(github_repo) | ||
|
|
||
| assert Enum.count(issues) == 8 |
There was a problem hiding this comment.
This doesn't actually use the with_real_api right now. It should.
| is_after = current_time |> Timex.after?(previous_time) | ||
| is_equal = current_time |> Timex.equal?(previous_time) | ||
| after_or_equal = is_after || is_equal | ||
| case after_or_equal do |
There was a problem hiding this comment.
Nothing is testing for this change yet.
There was a problem hiding this comment.
I'm OK with creating an issue and doing it separately.
|
|
||
| def get_all(url, headers, options) do | ||
| HTTPoison.start | ||
| {:ok, response} = HTTPoison.get(url, headers, options) |
There was a problem hiding this comment.
This should likely be modified to call into a more central place so we can mock the %HTTPoison.Response{} (headers included). Otherwise it will be hard to test pagination.
There was a problem hiding this comment.
I'm thinking we have an API.Gateway which simply has a request method that calls HTTPosion. Response parsing can be done in this module. We then mock the gateway in tests.
|
Coverage of |
begedin
left a comment
begedin
left a comment
There was a problem hiding this comment.
I have some comments here, hope that helps.
|
|
||
| def get_all(url, headers, options) do | ||
| HTTPoison.start | ||
| {:ok, response} = HTTPoison.get(url, headers, options) |
There was a problem hiding this comment.
I'm thinking we have an API.Gateway which simply has a request method that calls HTTPosion. Response parsing can be done in this module. We then mock the gateway in tests.
| 1 -> first_page | ||
| total -> | ||
| first_page ++ get_remaining_pages(total, url, headers, options) |> List.flatten | ||
| end |
There was a problem hiding this comment.
With having a separate gateway, our marshall_response could then
- check if the headers contains a
"Links"key - if it does (and optionally, doable in a separate PR, if an option is set to get all), it calls this code which makes additional requests for further pages
This isn't absolutely needed in this PR, but could be an approach
| is_after = current_time |> Timex.after?(previous_time) | ||
| is_equal = current_time |> Timex.equal?(previous_time) | ||
| after_or_equal = is_after || is_equal | ||
| case after_or_equal do |
There was a problem hiding this comment.
I'm OK with creating an issue and doing it separately.
|
|
||
| Application.put_env(:code_corps, :github, old_mock) | ||
| end | ||
| end |
There was a problem hiding this comment.
Not sure what the perfect approach is, but we could make a command alias, for example mix test.acceptance, which basically calls mix test --include :acceptance
Then we setup mix test to auto-exclude tests with the tag :acceptance.
If we want to use the same test in acceptance and mock API mode, that's probably also doable, but we'll have to be creative. I'm not sure what the approach would be in that case.
joshsmith
force-pushed
the
github-pagination
branch
from
0b728a5 to
1034e9a
Compare
joshsmith
force-pushed
the
github-pagination
branch
from
1034e9a to
e86971d
Compare
Closes #1109