feat(cpp): add functions related to system#3102
feat(cpp): add functions related to system#3102slbotbm wants to merge 2 commits intoapache:masterfrom
Conversation
slbotbm
changed the title
|
This PR is ready for review |
slbotbm
changed the title
| ffi::ClientInfo { | ||
| client_id: client.client_id, | ||
| // TODO(slbotbm): In high-level client, this should be converted to None. | ||
| user_id: client.user_id.unwrap_or(u32::MAX), |
There was a problem hiding this comment.
client.user_id.unwrap_or(u32::MAX) collapses None and a valid id of u32::MAX into the same value. backing type is Option<u32> (core/common/src/types/client/client_info.rs:33). plumb option through ffi - add has_user_id: bool to ffi::ClientInfo or split into a shared cxx option type.
| ffi::ClientInfoDetails { | ||
| client_id: client.client_id, | ||
| // TODO(slbotbm): In high-level client, this should be converted to None. | ||
| user_id: client.user_id.unwrap_or(u32::MAX), |
There was a problem hiding this comment.
same sentinel collision in From<RustClientInfoDetails>. same fix as L40.
|
|
||
| struct ClientInfo { | ||
| client_id: u32, | ||
| user_id: u32, |
There was a problem hiding this comment.
user_id: u32 in ffi::ClientInfo cannot represent an absent value. root cause behind client.rs:40. add has_user_id: bool field.
|
|
||
| struct ClientInfoDetails { | ||
| client_id: u32, | ||
| user_id: u32, |
There was a problem hiding this comment.
same shape problem for ffi::ClientInfoDetails.user_id.
| os_version: String, | ||
| kernel_version: String, | ||
| iggy_server_version: String, | ||
| iggy_server_semver: u32, |
There was a problem hiding this comment.
iggy_server_semver: u32 paired with unwrap_or(u32::MAX) sentinel at client.rs:116. backing field is Option<u32> (core/common/src/types/stats/mod.rs:73). prior commit #3036 already got burned by a zero-sentinel - don't repeat. add has_server_semver: bool.
| ASSERT_NE(client, nullptr); | ||
|
|
||
| const auto heartbeat_interval = client->heartbeat_interval(); | ||
| EXPECT_EQ(heartbeat_interval, 5'000'000u); |
There was a problem hiding this comment.
magic 5'000'000u (and 10'000'000u at L757). define constexpr std::uint64_t kDefaultHeartbeatMicros = 5'000'000ull; or derive from IggyDuration::from("5s").as_micros().
| client = nullptr; | ||
| } | ||
|
|
||
| TEST(LowLevelE2E_Client, SnapshotBeforeLoginThrows) { |
There was a problem hiding this comment.
all four snapshot tests (L763, L782, L797, L811) are negative paths. add a happy-path test with "deflated" + ["test"] asserting the returned Vec<u8> is non-empty.
| ASSERT_NO_THROW(client->connect()); | ||
| ASSERT_THROW(client->get_stats(), std::exception); | ||
|
|
||
| ASSERT_NO_THROW(iggy::ffi::delete_connection(client)); |
There was a problem hiding this comment.
every test manually does delete_connection(client); client = nullptr;. wrap in struct ScopedClient { ~ScopedClient() { iggy::ffi::delete_connection(p); } }; to cut ~30 lines of boilerplate and recover correctness on test-assertion failure - today the connection leaks if an ASSERT_* aborts mid-test.
| ASSERT_NO_THROW({ client = iggy::ffi::new_connection(""); }); | ||
| ASSERT_NE(client, nullptr); | ||
|
|
||
| rust::Vec<rust::String> snapshot_types_before_connect; |
There was a problem hiding this comment.
rust::Vec<rust::String> snapshot_types; snapshot_types.push_back(...) repeats at L774, L787, L802, L816. add a make_snapshot_types({"test"}) helper in tests/common/test_helpers.hpp.
| println!("cargo:rerun-if-changed=src/identifier.rs"); | ||
| println!("cargo:rerun-if-changed=src/lib.rs"); | ||
| println!("cargo:rerun-if-changed=src/stream.rs"); | ||
| println!("cargo:rerun-if-changed=src/system.rs"); |
There was a problem hiding this comment.
cargo:rerun-if-changed=src/system.rs watches a file that doesn't exist (verified ls foreign/cpp/src/system.rs). cargo accepts the entry but it's dead config. delete the line or create the file.
2 participants
Which issue does this PR close?
Works toward completion of #2100
Rationale
This PR adds functions related to the system, and tests for previously untested functions.
What changed?
This PR adds:
The places labelled TODO will be completed after merging of #3046.
Local Execution
AI Usage
If AI tools were used, please answer: