Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTPoison Conversion #16

Merged

Conversation

DavidAntaramian
Copy link
Contributor

  • Switch to HTTPoison dependency
  • Refactor tests to use HTTPoison

This addresses #12, which thankfully was very isolated. The largest factor of the changes was making sure callers of SparkPost.Endpoint.request/3 were updated to call SparkPost.Endpoint.request/5 correctly. request/5 is modeled to be identical to the underlying HTTPoison.request that it calls.

The majority of the changes to tests were handled by updating the mocks, but as I mentioned in #13, I think these tests are very fragile since they are not testing the true functionality of the module. That being said, I've done some manual testing which works. 👌

I did make a small change to the config file layout which uses the typical tiered layout.

@ewandennis @richleland

@princemaple
Copy link

hello?

@ewandennis
Copy link
Contributor

My my. This PR has been open waaay too long for which I apologise.

@DavidAntaramian - would you mind bumping httpoison to v0.9.0 and we'll get this merged down?

I'd also love a chat about test strategy at some point. I'll come find you on Slack...

@DavidAntaramian
Copy link
Contributor Author

@ewandennis I should be able to find some time this week. Will do!

* Switch to HTTPoison dependency
* Refactor tests to use HTTPoison
* Removes unecessary handling of error state
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 287d1a3 on DavidAntaramian:features/12-use-httpoison into 270ff6d on SparkPost:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling b1eb970 on DavidAntaramian:features/12-use-httpoison into 270ff6d on SparkPost:master.

@princemaple
Copy link

@ewandennis could this be merged now?

@ewandennis ewandennis merged commit 5f43828 into SparkPost:master Aug 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants