More S3 client test #255
More S3 client test #255
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #255 +/- ##
==========================================
+ Coverage 86.41% 87.02% +0.60%
==========================================
Files 16 16
Lines 4055 4037 -18
==========================================
+ Hits 3504 3513 +9
+ Misses 551 524 -27
|
source/s3_client.c
Outdated
| int error = aws_http_headers_get(message_headers, g_host_header_name, &host_value); | ||
| AWS_ASSERT(!error); | ||
| (void)error; |
There was a problem hiding this comment.
| int error = aws_http_headers_get(message_headers, g_host_header_name, &host_value); | |
| AWS_ASSERT(!error); | |
| (void)error; | |
| AWS_ASSERT(!aws_http_headers_get(message_headers, g_host_header_name, &host_value)); |
There was a problem hiding this comment.
AWS_ASSERT will be optimized out for release build. So, you cannot use AWS_ASSERT on code that really needs to run
There was a problem hiding this comment.
What about precondition if we are asserting that this will never fail? Or just not checking the error at all.
Edit: Nevermind, precondition is also just an AWS_ASSERT.
There was a problem hiding this comment.
If anything went wrong, we can catch the error during our tests and CI, which basically are debug build. But, anything went wrong from there, will be a programming bug, instead of something can really happen.
But, yeah, in theory, that should never fail. Just doing this to respect the return code.
There was a problem hiding this comment.
A fatal assert might be better. We do the same in Swift. If something slips through our tests, this will be a hard to debug bug because of ignoring the error in production which will cause failure somewhere else.
There was a problem hiding this comment.
The logic here is host header has already been set by the code above.
It should never ever error out.
There was a problem hiding this comment.
It doesn't really matter. I just prefer this code style to just AWS_ASSERT on this kind of things. So, it won't affect the production and we did check the possible error code for most of the case
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.