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

feat: adds bt http client #74

Merged
merged 2 commits into from
Jun 15, 2023
Merged

feat: adds bt http client #74

merged 2 commits into from
Jun 15, 2023

Conversation

kevinperaza
Copy link
Contributor

@kevinperaza kevinperaza commented Jun 11, 2023

Description

  • feat: adds bt http client

Testing required outside of automated testing?

  • Not Applicable

Screenshots (if appropriate):

image
  • Not Applicable

Rollback / Rollforward Procedure

  • Roll Forward
  • Roll Back

Reviewer Checklist

  • Description of Change
  • Description of outside testing if applicable.
  • Description of Roll Forward / Backward Procedure
  • Documentation updated for Change

@kevinperaza kevinperaza force-pushed the adds-http-client branch 4 times, most recently from 8953130 to 5964f1b Compare June 12, 2023 22:23
@kevinperaza kevinperaza marked this pull request as ready for review June 12, 2023 22:57
@kevinperaza kevinperaza requested a review from a team as a code owner June 12, 2023 22:57
send(url, HttpMethod.DELETE, null, headers)
}

private fun requestBuilder(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename this to buildRequest since it is building it already

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed ✅

</LinearLayout>


<TextView
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference between payment_method and result

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed payment_method

val foo = "bar"
}

server.enqueue(MockResponse().setBody("Everything is broken").setResponseCode(500))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it really returns 500? it feels like we should return 400

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just MockedWebServer; it could return anything you want and in this test we're just making sure we throw when using unsupported content-types in the request; we don't have control over the responses from the 3rd-parties anyways

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if they provide a content type that the client doesn't support does it return 500?

val request = server.takeRequest()
assertEquals("POST", request.method)
assertEquals(endpoint, request.path)
assertEquals(expectedRequestBody, request.body.readUtf8())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you assert on the response of all of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can. Do you see any value tho? Or is there anything in particular you'd like to see? Because it's pretty much the same as above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in which one are you asserting the response?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to make sure nothing is updating the response and we get what we expect

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll leave it up to you

// RequestBody appends the charset by default
assertEquals(expectedRequestBody.contentType(), "$contentType; charset=utf-8".toMediaType())
assertEquals(expectedRequestBody.contentLength(), requestBody?.contentLength())

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove these extra lines

import java.io.IOException

class DualWriteUtilsTests {
private val faker = Faker()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you using this anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not; removed.

@kevinperaza kevinperaza requested a review from jleon15 June 14, 2023 23:28
@kevinperaza kevinperaza merged commit d8d071c into master Jun 15, 2023
@kevinperaza kevinperaza deleted the adds-http-client branch June 15, 2023 23:56
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.

2 participants