Fix ConfigTests tests
#85
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lorak-mmk
requested changes
Oct 17, 2022
| cluster: *mut CassCluster, | ||
| contact_points: *mut *const c_char, | ||
| contact_points_length: *mut size_t, | ||
| contact_points_boxed: *mut *const String, |
There was a problem hiding this comment.
It probably doesn't matter, but maybe this should be *mut *mut? We use it as *mut String when freeing.
There was a problem hiding this comment.
I guess that does not have an effect on the generated code and rather is a question of documenting intent.
scylla-rust-wrapper/build.rs
Outdated
Comment on lines
36
to
49
| fn prepare_testing_bindings(out_path: &Path) { | ||
| let testing_bindings = bindgen::Builder::default() | ||
| .header("extern/testing_utils.h") | ||
| .parse_callbacks(Box::new(bindgen::CargoCallbacks)) | ||
| .layout_tests(true) | ||
| .generate_comments(false) | ||
| .generate() | ||
| .expect("Unable to generate bindings"); | ||
|
|
||
| testing_bindings | ||
| .write_to_file(out_path.join("testing_bindings.rs")) | ||
| .expect("Couldn't write bindings!"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are you generating those bindings? It doesn't look like your'e using them anywhere
Comment on lines
+1
to
+26
| #include "testing_utils.h" | ||
| #include <stdexcept> | ||
|
|
||
| CASS_EXPORT CassConsistency testing_get_consistency(const CassStatement* statement) { | ||
| throw std::runtime_error("UNIMPLEMENTED testing_get_consistency\n"); | ||
| } | ||
|
|
||
| CASS_EXPORT CassConsistency testing_get_serial_consistency(const CassStatement* statement) { | ||
| throw std::runtime_error("UNIMPLEMENTED testing_get_serial_consistency\n"); | ||
| } | ||
|
|
||
| CASS_EXPORT cass_uint64_t testing_get_request_timeout_ms(const CassStatement* statement) { | ||
| throw std::runtime_error("UNIMPLEMENTED testing_get_request_timeout_ms\n"); | ||
| } | ||
|
|
||
| CASS_EXPORT const CassRetryPolicy* testing_get_retry_policy(const CassStatement* statement) { | ||
| throw std::runtime_error("UNIMPLEMENTED testing_get_retry_policy\n"); | ||
| } | ||
|
|
||
| CASS_EXPORT const char* testing_get_server_name(CassFuture* future) { | ||
| throw std::runtime_error("UNIMPLEMENTED testing_get_server_name\n"); | ||
| } | ||
|
|
||
| CASS_EXPORT void testing_set_record_attempted_hosts(CassStatement* statement, cass_bool_t enable) { | ||
| throw std::runtime_error("UNIMPLEMENTED testing_set_record_attempted_hosts\n"); | ||
| } |
There was a problem hiding this comment.
Why are those implementations required? Can't you just remove them from testing_utils.h?
There was a problem hiding this comment.
We can safely remove them, but I guess this way it will be much easier to find which testing functions are yet to be implemented.
Comment on lines
+1
to
+34
| #ifndef CPPDRIVERV2_TESTING_UTILS_H | ||
| #define CPPDRIVERV2_TESTING_UTILS_H | ||
|
|
||
| #include "cassandra.h" | ||
|
|
||
| extern "C" { | ||
| CASS_EXPORT cass_uint16_t testing_get_connect_timeout_from_cluster(CassCluster* cluster); | ||
|
|
||
| CASS_EXPORT cass_int32_t testing_get_port_from_cluster(CassCluster* cluster); | ||
|
|
||
| CASS_EXPORT void | ||
| testing_get_contact_points_from_cluster( | ||
| CassCluster* cluster, | ||
| const char** contact_points, | ||
| size_t* contact_points_length, | ||
| void** contact_points_boxed | ||
| ); | ||
|
|
||
| CASS_EXPORT void testing_free_contact_points_string(void* box); | ||
|
|
||
| CASS_EXPORT CassConsistency testing_get_consistency(const CassStatement* statement); | ||
|
|
||
| CASS_EXPORT CassConsistency testing_get_serial_consistency(const CassStatement* statement); | ||
|
|
||
| CASS_EXPORT cass_uint64_t testing_get_request_timeout_ms(const CassStatement* statement); | ||
|
|
||
| CASS_EXPORT const CassRetryPolicy* testing_get_retry_policy(const CassStatement* statement); | ||
|
|
||
| CASS_EXPORT const char* testing_get_server_name(CassFuture* future); | ||
|
|
||
| CASS_EXPORT void testing_set_record_attempted_hosts(CassStatement* statement, cass_bool_t enable); | ||
| } | ||
|
|
||
| #endif //CPPDRIVERV2_TESTING_UTILS_H |
There was a problem hiding this comment.
This file is present twice for some reason, with a slight modification. Do we need it twice in our repository?
Also, if I'm understanding this correctly, those are signatures for Rust functions that are used in implementation of testing.hpp. If that's the case then:
- Name should be different than
testing_utils.h- something liketesting_rust_impls.hmaybe? - This file should not be written by hand, but rather generated - cbindgen is a tool to do that
Added implementation for functions in `testing.cpp` used in `ConfigTests` tests: * get_connect_timeout_from_cluster * get_port_from_cluster * get_contact_points_from_cluster To avoid generating bindings for functions that are directly under namespaces and may return objects of custom types, `testing_utils` header is added as median between the Rust implementation of those functions and the `testing.hpp` header. Added `ConfigTests` tests to GitHub Actions for Cassandra and ScyllaDB.
|
Closing in favor of #163 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added implementation for functions in
testing.cppused inConfigTeststests:To avoid generating bindings for functions that are directly under namespaces and may return objects of custom types, the
testing_utilsheader is added as a median between the Rust implementation of those functions and thetesting.hppheader.Added
ConfigTeststests to GitHub Actions for Cassandra and ScyllaDB.Pre-review checklist
.github/workflows/build.ymlingtest_filter..github/workflows/cassandra.ymlingtest_filter.