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

Add tests for HttpPostContext content-type #140

Merged
merged 3 commits into from Jul 25, 2019

Conversation

doyaaaaaken
Copy link
Contributor

Hello, I try to resolve #62 , but I can't do to the last.
Because, this comment's ( #62 (comment) ) 1st way ( header { type } ) seems not to work.

So, at first I try to implement 2nd and 3rd way's test.

@doyaaaaaken
Copy link
Contributor Author

header { type } seems not to work.

I try to run below test code, but failed.

@Test
    fun `application json content type when content type is set on 'header' method`() {
        val expected = "application/json; charset=utf-8"
        val context = HttpPostContext()
        context.header { "Content-Type" to "application/json; charset=utf-8" }
        context.body { string("content") }

        assertEquals(expected, context.makeBody().contentType().toString())
    }

Can I fix production code (below /src code) in other PR? (After this PR is merged)

@rybalkinsd
Copy link
Owner

Hi @doyaaaaaken ! Thanks for your contribution. I’ll be able to handle this PR in 10 days.
@DeviantBadge would you have time to check earlier?


1. Form or Json in body : ```kotlin ... body() { json { ... } } ...```
2. Custom body type : ```kotlin ... body(myContentType) { ... } ...```

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also you should mention about kotlin header{ "Content-type" to type } way.
About it I wrote some info in pull request comments.

@DeviantBadge
Copy link
Collaborator

Hello, @doyaaaaaken !
Many thanks for your effort!
I am ready to accept your pull request if you add information about last way to set Content-type (through headers directly), also we need to get approve from @rybalkinsd .

Question about setting Content-type with kotlin header{ "Content-type" to type } is little bit deeper.
You know that we are using okhttp under the hood, so part of logic is transferred to okhttp library.
If we dig deeper, we can find, that okhttp sets 'Content-type' header if body present and Content-type in it is not 'null' (Ref. 'BridgeInterceptor' line 54 in okhttp version '3.14.2')

So, if body was set and Content-type is not null, request will use Content-type from body and will ignore header
If body was set and Content-type is null, request will use header
if body was not set, request will also use header

You can also check it through postman-echo public api:

httpPost {
    url("https://postman-echo.com/post")
    header { "Content-Type" to "application/json; charset=utf-8" }
    body { form("content") }
}.use {
    // will print json with form Content-Type in it 
    print(it.body()?.string())
}

So, its very good that you tested HttpPostContext for setting contentType in body.
But also it would be also great if you will add Integration tests with (for example) postman-echo, that will check request headers after all processing steps.

@codecov
Copy link

codecov bot commented Jul 21, 2019

Codecov Report

Merging #140 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #140   +/-   ##
=========================================
  Coverage     90.59%   90.59%           
  Complexity      120      120           
=========================================
  Files            40       40           
  Lines           372      372           
  Branches         45       45           
=========================================
  Hits            337      337           
  Misses            9        9           
  Partials         26       26

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f217bd2...23e263b. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jul 21, 2019

Codecov Report

Merging #140 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #140   +/-   ##
=========================================
  Coverage     90.59%   90.59%           
  Complexity      120      120           
=========================================
  Files            40       40           
  Lines           372      372           
  Branches         45       45           
=========================================
  Hits            337      337           
  Misses            9        9           
  Partials         26       26

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f217bd2...23e263b. Read the comment docs.

@doyaaaaaken
Copy link
Contributor Author

@DeviantBadge
Many thanks for your detail review! I improved my code by your review.

@DeviantBadge
Copy link
Collaborator

Great!
Now we will merge your changes

@DeviantBadge DeviantBadge merged commit c6dfcba into rybalkinsd:master Jul 25, 2019
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.

Test body content type
3 participants