Implement tests for range and partial content responses - #619
Conversation
|
@alamb how do you feel about this PR? |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @vitoordaz -- I took a quick look and it seems like a nice addition to me. I have one test question, otherwise I think this is ready to go
| assert_eq!(b.as_ref(), b"ello"); | ||
|
|
||
| // NOTE: if debug_assertions is true, hyper panics with response content length header | ||
| // value does not match the length of response body. |
There was a problem hiding this comment.
Does this mean hyper would panic in a real world case? If so, is there a ticket tracking it?
Or does this mean that this particular response is not valid (and thus we shouldn't be testing it)?
There was a problem hiding this comment.
hyper panics in test environment which usually have debug_assertions = true. In prod debug_assertions = false and hyper returns regular response.
I'm wondering if we can update CI to run all tests with debug_assertions = false as well?
There was a problem hiding this comment.
I'm wondering if we can update CI to run all tests with debug_assertions = false as well?
We could add a new test run with that enabled (or perhaps we could run the tests in --release mode)?
There was a problem hiding this comment.
yes, I think that this will work. I was using this command to run this test
RUSTFLAGS="-C debug-assertions=false" RUST_BACKTRACE=full cargo test --features http client::get::http_tests::test_stream_retry
There was a problem hiding this comment.
Perhaps you can do so as a follow on PR
|
I think if you can merge up from main the CI tests would also pass cleanly |
|
(please just merge up from main so we can get a clean CI run) |
Added tests for handling partial content responses and retries for incomplete response bodies.
Removed unnecessary empty lines in the get.rs file.
|
Thanks @vitoordaz |
2 participants
Added tests for handling partial content responses and retries for incomplete response bodies.
Which issue does this PR close?
I didn't file an issue, because this PR only adds unit tests.
Rationale for this change
This improves code coverage and effectively documents how client behaves in edge case situation.
What changes are included in this PR?
Only unit tests for GetClient
Are there any user-facing changes?
No