Improve Storage code coverage #2052
Conversation
antkmsft
added
MQ
|
This repository is protected by Check Enforcer. The check-enforcer check-run will not pass until there is at least one more check-run successfully passing. Check Enforcer supports the following comment commands:
|
| void test_storage_blobs_init_nonnull_options(void** state); | ||
| void test_storage_blobs_init_nonnull_options(void** state) |
There was a problem hiding this comment.
We do we need to define the function like this first before implementing it?
Doesn't look like we do this elsewhere (in other places like core/json tests or iot hub), unless I am mistaken. And the tests are just static.
There was a problem hiding this comment.
We do. Could be in other places, but we do. This is to avoid warning on one of the compilers that we define a non-static function without prior declaration.
| #define URL_START "x://" | ||
| #define URL_START_LEN (sizeof(URL_START) - 1) |
There was a problem hiding this comment.
nit: rather than defining these macros and then undefining them after, why not use locals within the test function itself?
There was a problem hiding this comment.
I'd love to do that, - I really only use macros and non-local stuff if I have to, - but you see the size of the url_buf is greater than the size of "x://", so there is no good way of getting the value of URL_START_LEN without repetition, and if we do repetition, then there is always a possibility that you change one thing, but don't update another. So, macros are the most elegant and reliable way to do it.
| (void)state; | ||
| SETUP_PRECONDITION_CHECK_TESTS(); | ||
|
|
||
| _az_http_client_set_callback(no_op_transport); |
There was a problem hiding this comment.
Unrelated question: Is this how we expect customers to set the transport?
There was a problem hiding this comment.
To pick curl, you specify TRANSPORT_CURL=ON when building the SDK. There isn't any other transport at the moment, but in general the transport should implement the az_http_client_send_request() function. If there was WinHTTP implementation, for example, I'd think there would be some switch like TRANSPORT_WINHTTP=ON, and then in CMakeLists.txt for core we would replace az_curl.c with az_winhttp.c (can't easily have both, although it is possible).
In this specific case, when storage tests are built, they do implement the az_http_client_send_request(), and _az_http_client_set_callback() is really a part of storage tests, not the core API: see https://github.com/Azure/azure-sdk-for-c/blob/feature/storage-blobs/sdk/tests/storage/blobs/_az_test_http_client.h and https://github.com/Azure/azure-sdk-for-c/blob/feature/storage-blobs/sdk/tests/storage/blobs/_az_test_http_client.c . It allows you to provide callback, but our core does not directly offer you that feature, although as you see, if test code can do that, core could add that in the future if needed, or even customers can add that feature themselves if they need to switch between transport (although they'll have to do some linker magic or code patching to rename az_curl's az_http_client_send_request().
| _az_http_client_set_callback(NULL); | ||
| } | ||
|
|
||
| #endif |
There was a problem hiding this comment.
nit: Add a comment // AZ_NO_PRECONDITION_CHECKING
There was a problem hiding this comment.
Let's make clangformat to do that, like it does for C++. With manual comment, it may go out of sync (for instance, if you change the ifdef).
There was a problem hiding this comment.
It looks like there isn't an option in clang-format to do that. I had an impression it does that, but that's for C++ namespaces, not the preprocessor directives.
Closes #2052