From be60b7bab6cb6acfbdd40c2a072db2960d9c582f Mon Sep 17 00:00:00 2001 From: Arkadiusz Jedrzejewski Date: Wed, 28 Oct 2026 17:16:01 +0100 Subject: [PATCH] kvs: fix small C++ issues - Replace Yoda comparisons in kvs.cpp. - Drop stray this-> in the KvsBuilder setters. - Stop copy-assigning KvsBuilder to itself in the test scenarios. - Replace the C++20 designated initializer in KvsParameters. - Move using namespace out of kvs_general_test.h. - Drop redundant static in anonymous namespaces. - Remove a debug directory listing from kvs_test.cpp. - Rename shadowing locals in kvs_test.cpp and multiple_kvs.cpp. - Rename the snake_case helpers in kvs_test.cpp and kvs_builder_test.cpp to PascalCase. - Brace-initialize two declarations that reached main after the idiom pass. Co-Authored-By: Claude Opus 5.5 --- score/kvs/kvs.cpp | 4 +- score/kvs/kvs_builder.cpp | 10 +- score/kvs/kvs_builder_test.cpp | 8 +- score/kvs/kvs_deprecated_api_test.cpp | 2 + score/kvs/kvs_error_test.cpp | 2 + score/kvs/kvs_general_test.cpp | 2 + score/kvs/kvs_general_test.h | 5 +- score/kvs/kvs_helper_test.cpp | 2 + score/kvs/kvs_test.cpp | 137 +++++++++--------- .../test_scenarios/cit/default_values.cpp | 10 +- .../tests/test_scenarios/cit/multiple_kvs.cpp | 26 ++-- .../test_scenarios/helpers/kvs_instance.cpp | 8 +- .../test_scenarios/helpers/kvs_parameters.cpp | 2 +- score/kvs/tests/test_scenarios/test_basic.cpp | 4 +- 14 files changed, 116 insertions(+), 106 deletions(-) diff --git a/score/kvs/kvs.cpp b/score/kvs/kvs.cpp index b27def17e..b8b599bce 100644 --- a/score/kvs/kvs.cpp +++ b/score/kvs/kvs.cpp @@ -432,7 +432,7 @@ score::Result Kvs::SetValueToDefault(const std::string_view key) noexcept score::Result Kvs::IsValueDefault(const std::string_view key) const noexcept { - std::unique_lock lock(kvs_mutex_, std::try_to_lock); + std::unique_lock lock{kvs_mutex_, std::try_to_lock}; if (!lock.owns_lock()) { GetLogger().LogWarn() << "Failed to lock KVS mutex"; @@ -599,7 +599,7 @@ score::Result Kvs::SnapshotRestore(const SnapshotId& snapshot_id) noexcept } // Fail if the snapshot ID is the current KVS or does not exist. - if ((0 == snapshot_id.id) || (*snapshot_count_result < snapshot_id.id)) + if ((snapshot_id.id == 0U) || (*snapshot_count_result < snapshot_id.id)) { GetLogger().LogError() << "Invalid snapshot ID" << snapshot_id.id; return score::MakeUnexpected(ErrorCode::kInvalidSnapshotId); diff --git a/score/kvs/kvs_builder.cpp b/score/kvs/kvs_builder.cpp index 5073b641e..00475600a 100644 --- a/score/kvs/kvs_builder.cpp +++ b/score/kvs/kvs_builder.cpp @@ -29,7 +29,7 @@ KvsBuilder::KvsBuilder(const InstanceId& instance_id) noexcept KvsBuilder& KvsBuilder::Snapshot(const SnapshotId& snapshot_id) noexcept { - this->snapshot_id_ = snapshot_id; + snapshot_id_ = snapshot_id; return *this; } @@ -47,26 +47,26 @@ KvsBuilder& KvsBuilder::KvsLoad(const score::kvs::KvsLoad& mode) noexcept KvsBuilder& KvsBuilder::WorkingDir(const std::string& working_dir) noexcept { - this->working_dir_ = working_dir; + working_dir_ = working_dir; return *this; } KvsBuilder& KvsBuilder::MaxStorageBytes(const std::optional& max_storage_bytes) noexcept { - this->maximum_storage_bytes_ = max_storage_bytes; + maximum_storage_bytes_ = max_storage_bytes; return *this; } KvsBuilder& KvsBuilder::SnapshotMaxCount(const std::size_t max_count) noexcept { - this->max_snapshots_ = max_count; + max_snapshots_ = max_count; return *this; } score::Result KvsBuilder::Build() const noexcept { // Use current directory if empty. - const std::string working_dir = working_dir_.empty() ? std::string{"./"} : working_dir_; + const std::string working_dir{working_dir_.empty() ? std::string{"./"} : working_dir_}; return Kvs::Open( instance_id_, snapshot_id_, defaults_, kvs_load_, working_dir, maximum_storage_bytes_, max_snapshots_); diff --git a/score/kvs/kvs_builder_test.cpp b/score/kvs/kvs_builder_test.cpp index b7f44a9fc..dfd49fa12 100644 --- a/score/kvs/kvs_builder_test.cpp +++ b/score/kvs/kvs_builder_test.cpp @@ -13,9 +13,11 @@ #include "score/kvs/kvs_general_test.h" +using namespace score::kvs; + namespace { -void remove_file(const std::string& path) +void RemoveFile(const std::string& path) { score::cpp::ignore = score::filesystem::FilesystemFactory{}.CreateInstance().standard->Remove(path); } @@ -121,8 +123,8 @@ TEST(kvs_kvsbuilder, kvsbuilder_directory_check) ASSERT_TRUE(kvs.Flush()); auto filename{kvs.KvsFilePath(SnapshotId{0ul})}; - remove_file(expected_prefix + ".json"); - remove_file(expected_prefix + ".hash"); + RemoveFile(expected_prefix + ".json"); + RemoveFile(expected_prefix + ".hash"); ASSERT_TRUE(filename); EXPECT_EQ(filename.value().CStr(), expected_prefix + ".json"); } diff --git a/score/kvs/kvs_deprecated_api_test.cpp b/score/kvs/kvs_deprecated_api_test.cpp index 45b357d45..4bc059096 100644 --- a/score/kvs/kvs_deprecated_api_test.cpp +++ b/score/kvs/kvs_deprecated_api_test.cpp @@ -27,6 +27,8 @@ DISABLE_WARNING(-Wdeprecated-declarations) #include "score/kvs/kvsvalue.hpp" DISABLE_WARNING_POP +using namespace score::kvs; + namespace { diff --git a/score/kvs/kvs_error_test.cpp b/score/kvs/kvs_error_test.cpp index e9c5f6cd5..2b699c626 100644 --- a/score/kvs/kvs_error_test.cpp +++ b/score/kvs/kvs_error_test.cpp @@ -13,6 +13,8 @@ #include "score/kvs/kvs_general_test.h" +using namespace score::kvs; + namespace { constexpr score::kvs::KvsErrorDomain kKvsErrorDomain; diff --git a/score/kvs/kvs_general_test.cpp b/score/kvs/kvs_general_test.cpp index 761746af2..e87ca7736 100644 --- a/score/kvs/kvs_general_test.cpp +++ b/score/kvs/kvs_general_test.cpp @@ -13,6 +13,8 @@ #include "score/kvs/kvs_general_test.h" +using namespace score::kvs; + /// @brief Independent Adler-32 implementation to check the KVS hash against. uint32_t Adler32(const std::string& data) { diff --git a/score/kvs/kvs_general_test.h b/score/kvs/kvs_general_test.h index c43ed3908..9363dcf47 100644 --- a/score/kvs/kvs_general_test.h +++ b/score/kvs/kvs_general_test.h @@ -30,7 +30,6 @@ #include "score/json/i_json_writer_mock.h" #include "score/kvs/internal/kvs_helper.h" #include "score/kvs/kvs_builder.h" -using namespace score::kvs; uint32_t Adler32(const std::string& data); void PrepareEnvironment(); @@ -84,8 +83,8 @@ inline void PrepareEnvironment(const std::string& dir, // Default data used in unit tests. const std::uint32_t kInstance{123U}; -const InstanceId kInstanceId{kInstance}; -const SnapshotId kSnapshotId{0ul}; +const score::kvs::InstanceId kInstanceId{kInstance}; +const score::kvs::SnapshotId kSnapshotId{0ul}; // score::filesystem::Path could be constructed implicitly from std::string. // For readability, the test code constructs it explicitly from these strings. diff --git a/score/kvs/kvs_helper_test.cpp b/score/kvs/kvs_helper_test.cpp index 8d744f1bb..d092aa250 100644 --- a/score/kvs/kvs_helper_test.cpp +++ b/score/kvs/kvs_helper_test.cpp @@ -13,6 +13,8 @@ #include "score/kvs/kvs_general_test.h" +using namespace score::kvs; + TEST(kvs_calculate_hash_adler32, calculate_hash_adler32) { // Test the Adler-32 hash calculation. diff --git a/score/kvs/kvs_test.cpp b/score/kvs/kvs_test.cpp index 6f3dee1b6..72f24f33c 100644 --- a/score/kvs/kvs_test.cpp +++ b/score/kvs/kvs_test.cpp @@ -30,6 +30,8 @@ #include "score/os/stat.h" #include "score/utility.hpp" +using namespace score::kvs; + namespace { using score::filesystem::Perms; @@ -68,10 +70,10 @@ std::string FileContent(const score::filesystem::Path& path) } /// @brief Writes the defaults file and the snapshot 0 file with valid hashes, then opens the KVS. -score::Result open_crafted(const std::string& defaults, - const std::string& kvs_data, - KvsDefaults need_defaults = KvsDefaults::kRequired, - KvsLoad need_kvs = KvsLoad::kRequired) +score::Result OpenCrafted(const std::string& defaults, + const std::string& kvs_data, + KvsDefaults need_defaults = KvsDefaults::kRequired, + KvsLoad need_kvs = KvsLoad::kRequired) { PrepareEnvironment(); Store(kDefaultPrefix + ".json", kDefaultPrefix + ".hash", defaults); @@ -82,20 +84,20 @@ score::Result open_crafted(const std::string& defaults, /// @brief Writes the defaults file with a valid hash. /// Call it before Kvs::Open(), because Open() reads the defaults once. -void seed_defaults(const std::string& json) +void SeedDefaults(const std::string& json) { Store(kDefaultPrefix + ".json", kDefaultPrefix + ".hash", json); } /// @brief Returns one i32 entry of a KVS file. /// The key is JSON text without the quotes, so escape sequences in it stay as they are. -std::string entry(const std::string& json_escaped_key, int value) +std::string Entry(const std::string& json_escaped_key, int value) { return "\"" + json_escaped_key + "\":{\"t\":\"i32\",\"v\":" + std::to_string(value) + "}"; } /// @brief Returns the content of a KVS file: the type-tagged root object with the given entries. -std::string kvs_file(const std::string& entries) +std::string KvsFile(const std::string& entries) { return "{\"t\":\"obj\",\"v\":{" + entries + "}}"; } @@ -141,7 +143,7 @@ Kvs OpenTestKvs() } /// @brief Opens the KVS like OpenTestKvs() and replaces its data with "new_key". -Kvs open_test_kvs_with_new_key() +Kvs OpenTestKvsWithNewKey() { auto kvs{OpenTestKvs()}; EXPECT_TRUE(kvs.RemoveAllKeys()); @@ -151,7 +153,7 @@ Kvs open_test_kvs_with_new_key() /// @brief Checks that the stored KVS holds only "new_key". /// A second KVS loads the files, so a wrong hash fails here with kValidationFailed. -void expect_only_new_key() +void ExpectOnlyNewKey() { auto kvs{Kvs::Open(InstanceId(kInstanceId), kSnapshotId, @@ -170,26 +172,26 @@ void expect_only_new_key() EXPECT_EQ(std::get(value.value().GetValue()), 1); } -/// @brief Returns the JSON content that plant_generation() writes for snapshot `index`. -std::string generation_tag(std::size_t index) +/// @brief Returns the JSON content that PlantGeneration() writes for snapshot `index`. +std::string GenerationTag(std::size_t index) { return "{\"gen\":" + std::to_string(index) + "}"; } /// @brief Writes the JSON and hash files of snapshot `index`, tagged with the index. /// The hash is never read, so a tag is enough. -void plant_generation(std::size_t index) +void PlantGeneration(std::size_t index) { - std::ofstream(kFilenamePrefix + "_" + std::to_string(index) + ".json") << generation_tag(index); + std::ofstream(kFilenamePrefix + "_" + std::to_string(index) + ".json") << GenerationTag(index); std::ofstream(kFilenamePrefix + "_" + std::to_string(index) + ".hash") << "h" << index; } -std::string generation_json(std::size_t index) +std::string GenerationJson(std::size_t index) { return FileContent(score::filesystem::Path{kFilenamePrefix + "_" + std::to_string(index) + ".json"}); } -std::string generation_hash(std::size_t index) +std::string GenerationHash(std::size_t index) { return FileContent(score::filesystem::Path{kFilenamePrefix + "_" + std::to_string(index) + ".hash"}); } @@ -198,7 +200,7 @@ std::string generation_hash(std::size_t index) TEST(kvs_constructor, move_constructor) { PrepareEnvironment(); - seed_defaults(R"({"t": "obj", "v": {"test_default": {"t": "bool", "v": true}}})"); + SeedDefaults(R"({"t": "obj", "v": {"test_default": {"t": "bool", "v": true}}})"); const std::uint32_t instance_b{5U}; // Create object A. @@ -280,10 +282,10 @@ TEST(kvs_TEST, parse_json_data_success) { // The real parser reads the file through Open(). // This also covers the Optional mode with an existing file. - auto kvs{open_crafted(kDefaultJson, - R"({"t": "obj", "v": {"kvs": {"t": "i32", "v": 42}}})", - KvsDefaults::kOptional, - KvsLoad::kOptional)}; + auto kvs{OpenCrafted(kDefaultJson, + R"({"t": "obj", "v": {"kvs": {"t": "i32", "v": 42}}})", + KvsDefaults::kOptional, + KvsLoad::kOptional)}; ASSERT_TRUE(kvs); auto value{kvs->GetValue("kvs")}; @@ -297,23 +299,23 @@ TEST(kvs_TEST, parse_json_data_success) TEST(kvs_TEST, parse_json_data_failure) { // JSON parser failure. - auto kvs{open_crafted(kDefaultJson, "{ invalid json }")}; + auto kvs{OpenCrafted(kDefaultJson, "{ invalid json }")}; ASSERT_FALSE(kvs); EXPECT_EQ(static_cast(*kvs.error()), ErrorCode::kJsonParserError); // The root is valid JSON, but not a type-tagged value. - kvs = open_crafted(kDefaultJson, "42"); + kvs = OpenCrafted(kDefaultJson, "42"); ASSERT_FALSE(kvs); EXPECT_EQ(static_cast(*kvs.error()), ErrorCode::kInvalidValueType); // AnyToKvsValue error. - kvs = open_crafted(kDefaultJson, R"({"t": "obj", "v": {"kvs": {"t": "invalid", "v": 42}}})"); + kvs = OpenCrafted(kDefaultJson, R"({"t": "obj", "v": {"kvs": {"t": "invalid", "v": 42}}})"); ASSERT_FALSE(kvs); // Error is passed from AnyToKvsValue. EXPECT_EQ(static_cast(*kvs.error()), ErrorCode::kInvalidValueType); // Tagged root that is not an object. - kvs = open_crafted(kDefaultJson, R"({"t": "i32", "v": 5})"); + kvs = OpenCrafted(kDefaultJson, R"({"t": "i32", "v": 5})"); ASSERT_FALSE(kvs); EXPECT_EQ(static_cast(*kvs.error()), ErrorCode::kJsonParserError); @@ -572,13 +574,13 @@ TEST(kvs_TEST, parse_json_data_rejects_over_length_key) const std::string too_long_key(kMaxKeyLengthBytes + 1U, 'b'); // A key of the maximum length loads. - auto kvs{open_crafted(kDefaultJson, kvs_file(entry(valid_key, 42)))}; + auto kvs{OpenCrafted(kDefaultJson, KvsFile(Entry(valid_key, 42)))}; ASSERT_TRUE(kvs); auto exists{kvs->KeyExists(valid_key)}; ASSERT_TRUE(exists); EXPECT_TRUE(exists.value()); - kvs = open_crafted(kDefaultJson, kvs_file(entry(valid_key, 42) + "," + entry(too_long_key, 7))); + kvs = OpenCrafted(kDefaultJson, KvsFile(Entry(valid_key, 42) + "," + Entry(too_long_key, 7))); ASSERT_FALSE(kvs); EXPECT_EQ(static_cast(*kvs.error()), ErrorCode::kInvalidKey); @@ -590,8 +592,8 @@ TEST(kvs_TEST, parse_json_data_rejects_empty_key) // comp_req__kvs__key_length: an empty key found while loading means the file is invalid or corrupted. // Loading must halt with an error. // The JSON parser already rejects an empty key, before the key check of the KVS. - auto kvs{open_crafted(kDefaultJson, - R"({"t": "obj", "v": {"valid_key": {"t": "i32", "v": 42}, "": {"t": "i32", "v": 7}}})")}; + auto kvs{OpenCrafted(kDefaultJson, + R"({"t": "obj", "v": {"valid_key": {"t": "i32", "v": 42}, "": {"t": "i32", "v": 7}}})")}; ASSERT_FALSE(kvs); EXPECT_EQ(static_cast(*kvs.error()), ErrorCode::kJsonParserError); @@ -608,7 +610,7 @@ TEST(kvs_TEST, parse_json_data_rejects_invalid_characters_key) for (const auto& invalid_key : invalid_keys) { SCOPED_TRACE(invalid_key); - auto kvs{open_crafted(kDefaultJson, kvs_file(entry("valid_key", 42) + "," + entry(invalid_key, 7)))}; + auto kvs{OpenCrafted(kDefaultJson, KvsFile(Entry("valid_key", 42) + "," + Entry(invalid_key, 7)))}; ASSERT_FALSE(kvs); EXPECT_EQ(static_cast(*kvs.error()), ErrorCode::kInvalidKey); } @@ -621,7 +623,7 @@ TEST(kvs_TEST, parse_json_data_accepts_over_length_nested_key) // comp_req__kvs__key_length applies to root keys only. // Keys of nested objects are value data. const std::string long_key(kMaxKeyLengthBytes + 1U, 'b'); - auto kvs{open_crafted(kDefaultJson, kvs_file(R"("cfg":{"t":"obj","v":{)" + entry(long_key, 7) + "}}"))}; + auto kvs{OpenCrafted(kDefaultJson, KvsFile(R"("cfg":{"t":"obj","v":{)" + Entry(long_key, 7) + "}}"))}; ASSERT_TRUE(kvs); auto cfg{kvs->GetValue("cfg")}; @@ -639,7 +641,7 @@ TEST(kvs_TEST, parse_json_data_accepts_over_length_nested_key) TEST(kvs_open_json, open_json_json_invalid) { // An invalid defaults file fails in ParseJsonData(). - auto kvs{open_crafted("{ invalid json }", kKvsJson)}; + auto kvs{OpenCrafted("{ invalid json }", kKvsJson)}; ASSERT_FALSE(kvs); // Error code passed by ParseJsonData(). EXPECT_EQ(static_cast(*kvs.error()), ErrorCode::kJsonParserError); @@ -810,7 +812,7 @@ TEST(kvs_key_exists, key_exists_failure) TEST(kvs_get_value, get_value_success) { PrepareEnvironment(); - seed_defaults(R"({"t": "obj", "v": {"kvs": {"t": "i32", "v": 42}}})"); + SeedDefaults(R"({"t": "obj", "v": {"kvs": {"t": "i32", "v": 42}}})"); auto result{Kvs::Open(kInstanceId, kSnapshotId, @@ -877,7 +879,7 @@ TEST(kvs_get_value, get_value_failure) TEST(kvs_get_default_value, get_default_value_success) { PrepareEnvironment(); - seed_defaults(R"({"t": "obj", "v": {"kvs": {"t": "i32", "v": 42}}})"); + SeedDefaults(R"({"t": "obj", "v": {"kvs": {"t": "i32", "v": 42}}})"); auto result{Kvs::Open(kInstanceId, kSnapshotId, @@ -926,7 +928,7 @@ TEST(kvs_get_default_value, get_default_value_failure) TEST(kvs_set_value_to_default, set_value_to_default_success) { PrepareEnvironment(); - seed_defaults(R"({"t": "obj", "v": {"default": {"t": "i32", "v": 5}, "kvs": {"t": "f64", "v": 42.0}}})"); + SeedDefaults(R"({"t": "obj", "v": {"default": {"t": "i32", "v": 5}, "kvs": {"t": "f64", "v": 42.0}}})"); auto result{Kvs::Open(kInstanceId, kSnapshotId, @@ -1056,7 +1058,7 @@ TEST(kvs_set_value_to_default, set_value_to_default_failure_invalid_key) TEST(kvs_is_value_default, is_value_default) { PrepareEnvironment(); - seed_defaults(R"({"t": "obj", "v": {"default": {"t": "f64", "v": 42.0}}})"); + SeedDefaults(R"({"t": "obj", "v": {"default": {"t": "f64", "v": 42.0}}})"); auto result{Kvs::Open(kInstanceId, kSnapshotId, @@ -1079,23 +1081,23 @@ TEST(kvs_is_value_default, is_value_default) // Check non-default value. { - auto result{kvs.IsValueDefault("not-default")}; - ASSERT_TRUE(result); - ASSERT_FALSE(result.value()); + auto not_default_result{kvs.IsValueDefault("not-default")}; + ASSERT_TRUE(not_default_result); + ASSERT_FALSE(not_default_result.value()); } // Check default value. { - auto result{kvs.IsValueDefault("default")}; - ASSERT_TRUE(result); - ASSERT_TRUE(result.value()); + auto default_result{kvs.IsValueDefault("default")}; + ASSERT_TRUE(default_result); + ASSERT_TRUE(default_result.value()); } // Check key not found. { - auto result{kvs.IsValueDefault("non_existing_key")}; - ASSERT_FALSE(result); - ASSERT_EQ(result.error(), ErrorCode::kKeyNotFound); + auto missing_result{kvs.IsValueDefault("non_existing_key")}; + ASSERT_FALSE(missing_result); + ASSERT_EQ(missing_result.error(), ErrorCode::kKeyNotFound); } CleanupEnvironment(); @@ -1670,7 +1672,7 @@ TEST(kvs_write_json_data, write_json_data_open_failure) const score::filesystem::Path json_path{kKvsPrefix + ".json"}; const score::filesystem::Path hash_path{kKvsPrefix + ".hash"}; const std::string previous_hash{FileContent(hash_path)}; - auto kvs{open_test_kvs_with_new_key()}; + auto kvs{OpenTestKvsWithNewKey()}; { score::os::MockGuard<::testing::NiceMock> fcntl_mock; @@ -1695,7 +1697,7 @@ TEST(kvs_write_json_data, write_json_data_set_permissions_failure) const score::filesystem::Path json_path{kKvsPrefix + ".json"}; const score::filesystem::Path hash_path{kKvsPrefix + ".hash"}; const std::string previous_hash{FileContent(hash_path)}; - auto kvs{open_test_kvs_with_new_key()}; + auto kvs{OpenTestKvsWithNewKey()}; { score::os::MockGuard<::testing::NiceMock> stat_mock; @@ -1727,7 +1729,7 @@ TEST(kvs_write_json_data, write_json_data_staging_failure_keeps_previous_file) const auto fs{RealFilesystem()}; ASSERT_TRUE(fs.standard->CreateDirectory(json_tmp_path)); std::ofstream{json_tmp_path.Native() + "/file"} << "data"; - auto kvs{open_test_kvs_with_new_key()}; + auto kvs{OpenTestKvsWithNewKey()}; auto result{kvs.Flush()}; ASSERT_FALSE(result); @@ -1750,11 +1752,11 @@ TEST(kvs_write_json_data, write_json_data_replaces_stale_temp_file) constexpr Perms all_permissions{Perms::kReadWriteExecUser | Perms::kReadWriteExecGroup | Perms::kReadWriteExecOthers}; ASSERT_TRUE(fs.standard->Permissions(json_tmp_path, all_permissions)); - auto kvs{open_test_kvs_with_new_key()}; + auto kvs{OpenTestKvsWithNewKey()}; ASSERT_TRUE(kvs.Flush()); EXPECT_FALSE(FileExists(json_tmp_path)); - expect_only_new_key(); + ExpectOnlyNewKey(); EXPECT_EQ(FilePermissions(json_path), static_cast(kExpectedFilePermissions)); CleanupEnvironment(); @@ -1764,10 +1766,10 @@ TEST(kvs_write_json_data, write_json_data_leaves_no_temp_files) { PrepareEnvironment(); // Test temporary files are renamed to the JSON and hash files. - auto kvs{open_test_kvs_with_new_key()}; + auto kvs{OpenTestKvsWithNewKey()}; ASSERT_TRUE(kvs.Flush()); - expect_only_new_key(); + ExpectOnlyNewKey(); EXPECT_FALSE(FileExists(score::filesystem::Path{kKvsPrefix + ".json.tmp"})); EXPECT_FALSE(FileExists(score::filesystem::Path{kKvsPrefix + ".hash.tmp"})); @@ -1813,7 +1815,7 @@ TEST(kvs_write_json_data, write_json_data_overrides_existing_file_permissions) ASSERT_EQ(FilePermissions(hash_path), static_cast(all_permissions)); // With a maximum of 0 snapshots, Flush() replaces these files instead of rotating them to snapshot 1. - auto kvs{open_test_kvs_with_new_key()}; + auto kvs{OpenTestKvsWithNewKey()}; ASSERT_TRUE(kvs.Flush()); EXPECT_EQ(FilePermissions(json_path), static_cast(kExpectedFilePermissions)); EXPECT_EQ(FilePermissions(hash_path), static_cast(kExpectedFilePermissions)); @@ -1837,7 +1839,7 @@ TEST(kvs_snapshot_rotate, snapshot_rotate_success) // Create test snapshot files, tagged with their index. for (size_t i{0U}; i < kDefaultMaxSnapshots; i++) { - plant_generation(i); + PlantGeneration(i); EXPECT_EQ(result.value().SnapshotCount().value(), i + 1); } ASSERT_FALSE(std::filesystem::exists(kFilenamePrefix + "_" + std::to_string(kDefaultMaxSnapshots) + ".json")); @@ -1850,12 +1852,12 @@ TEST(kvs_snapshot_rotate, snapshot_rotate_success) for (size_t i{0U}; i < kDefaultMaxSnapshots; i++) { SCOPED_TRACE(i); - EXPECT_EQ(generation_json(i + 1), generation_tag(i)); - EXPECT_EQ(generation_hash(i + 1), "h" + std::to_string(i)); + EXPECT_EQ(GenerationJson(i + 1), GenerationTag(i)); + EXPECT_EQ(GenerationHash(i + 1), "h" + std::to_string(i)); } // Snapshot 0 holds the new data with a valid hash. - EXPECT_NE(generation_json(0), generation_tag(0)); + EXPECT_NE(GenerationJson(0), GenerationTag(0)); auto reopened{Kvs::Open(kInstanceId, kSnapshotId, KvsDefaults::kOptional, @@ -1884,15 +1886,15 @@ TEST(kvs_snapshot_rotate, snapshot_rotate_max_snapshots) // Create test snapshot files, tagged with their index. for (size_t i{0U}; i < kDefaultMaxSnapshots; i++) { - plant_generation(i); + PlantGeneration(i); EXPECT_EQ(result.value().SnapshotCount().value(), i + 1); } // Also fill the last ID, so that a rotation one step too far has a file to move. - plant_generation(kDefaultMaxSnapshots); + PlantGeneration(kDefaultMaxSnapshots); // Check if no ID higher than kDefaultMaxSnapshots exists. ASSERT_TRUE(result.value().Flush()); - EXPECT_EQ(generation_json(kDefaultMaxSnapshots), generation_tag(kDefaultMaxSnapshots - 1U)); + EXPECT_EQ(GenerationJson(kDefaultMaxSnapshots), GenerationTag(kDefaultMaxSnapshots - 1U)); EXPECT_FALSE(std::filesystem::exists(kFilenamePrefix + "_" + std::to_string(kDefaultMaxSnapshots + 1) + ".json")); EXPECT_FALSE(std::filesystem::exists(kFilenamePrefix + "_" + std::to_string(kDefaultMaxSnapshots + 1) + ".hash")); @@ -1915,7 +1917,7 @@ TEST(kvs_snapshot_rotate, snapshot_rotate_failure_renaming_json) // Create test snapshot files, tagged with their index. for (size_t i{0U}; i < kDefaultMaxSnapshots; i++) { - plant_generation(i); + PlantGeneration(i); EXPECT_EQ(result.value().SnapshotCount().value(), i + 1); } @@ -1926,7 +1928,7 @@ TEST(kvs_snapshot_rotate, snapshot_rotate_failure_renaming_json) ASSERT_FALSE(flush_result); EXPECT_EQ(static_cast(*flush_result.error()), ErrorCode::kPhysicalStorageFailure); // The rotation failed, so Flush() did not write a new snapshot 0. - EXPECT_EQ(generation_json(0), generation_tag(0)); + EXPECT_EQ(GenerationJson(0), GenerationTag(0)); CleanupEnvironment(); } @@ -1947,7 +1949,7 @@ TEST(kvs_snapshot_rotate, snapshot_rotate_failure_renaming_hash) // Create test snapshot files, tagged with their index. for (size_t i{0U}; i < kDefaultMaxSnapshots; i++) { - plant_generation(i); + PlantGeneration(i); EXPECT_EQ(result.value().SnapshotCount().value(), i + 1); } @@ -1958,7 +1960,7 @@ TEST(kvs_snapshot_rotate, snapshot_rotate_failure_renaming_hash) ASSERT_FALSE(flush_result); EXPECT_EQ(static_cast(*flush_result.error()), ErrorCode::kPhysicalStorageFailure); // The rotation failed, so Flush() did not write a new snapshot 0. - EXPECT_EQ(generation_json(0), generation_tag(0)); + EXPECT_EQ(GenerationJson(0), GenerationTag(0)); CleanupEnvironment(); } @@ -2492,7 +2494,6 @@ TEST(kvs_get_filename, get_hashname_success) for (size_t i{0U}; i < kDefaultMaxSnapshots; i++) { auto hashname{result.value().HashFilePath(SnapshotId(i))}; - system("ls -l ./data_folder/"); EXPECT_TRUE(hashname); EXPECT_EQ(hashname.value().CStr(), kFilenamePrefix + "_" + std::to_string(i) + ".hash"); } @@ -2598,7 +2599,7 @@ TEST(kvs_remove_all_keys, remove_all_keys_empty_kvs) TEST(kvs_remove_all_keys, remove_all_keys_keeps_defaults) { PrepareEnvironment(); - seed_defaults(R"({"t": "obj", "v": {"defaulted": {"t": "f64", "v": 42.0}}})"); + SeedDefaults(R"({"t": "obj", "v": {"defaulted": {"t": "f64", "v": 42.0}}})"); auto kvs{Kvs::Open(kInstanceId, kSnapshotId, @@ -3161,7 +3162,7 @@ TEST(kvs_snapshot_rotate, snapshot_rotate_respects_configured_max) // Tag them, so that the rotation can be followed by content and not only by existence. for (std::size_t i{0U}; i <= configured_max; i++) { - plant_generation(i); + PlantGeneration(i); } ASSERT_TRUE(result.value().Flush()); @@ -3169,9 +3170,9 @@ TEST(kvs_snapshot_rotate, snapshot_rotate_respects_configured_max) // Rotation happened: the previous snapshot 0 moved into the first snapshot slot. // Flush() then wrote a new snapshot 0. // Without this the test would also pass if nothing rotated. - EXPECT_EQ(generation_json(1), generation_tag(0)); + EXPECT_EQ(GenerationJson(1), GenerationTag(0)); EXPECT_TRUE(FileExists(score::filesystem::Path{kFilenamePrefix + "_0.json"})); - EXPECT_NE(generation_json(0), generation_tag(0)); + EXPECT_NE(GenerationJson(0), GenerationTag(0)); // No snapshot beyond the configured maximum may be created. // If rotation used the compile-time default instead of the configured value, _1 would have been rotated into _2. diff --git a/score/kvs/tests/test_scenarios/cit/default_values.cpp b/score/kvs/tests/test_scenarios/cit/default_values.cpp index 86e117b9a..8c1a9a6ea 100644 --- a/score/kvs/tests/test_scenarios/cit/default_values.cpp +++ b/score/kvs/tests/test_scenarios/cit/default_values.cpp @@ -33,10 +33,10 @@ const std::string kTargetName{"cpp_test_scenarios::cit::default_values"}; /// @param value_is_default Whether the current value matches the default ("Ok(true)", "Ok(false)" or error string). /// @param default_value String encoding the default value for the key (e.g. "Ok(F64(...))" or error string). /// @param current_value String encoding the current value for the key (e.g. "Ok(F64(...))" or error string). -static void InfoLog(const std::string& key, - const std::string& value_is_default, - const std::string& default_value, - const std::string& current_value) +void InfoLog(const std::string& key, + const std::string& value_is_default, + const std::string& default_value, + const std::string& current_value) { TRACING_INFO(kTargetName, std::pair{std::string{"key"}, key}, @@ -52,7 +52,7 @@ static void InfoLog(const std::string& key, /// @param key The key being queried or modified in the KVS. /// @param value_is_default String encoding whether the current value matches the default. /// @param current_value Current value for the key (e.g. "Ok(F64(...))" or an error such as "Err(KeyNotFound)"). -static void InfoLog(const std::string& key, const std::string& value_is_default, const std::string& current_value) +void InfoLog(const std::string& key, const std::string& value_is_default, const std::string& current_value) { TRACING_INFO(kTargetName, std::pair{std::string{"key"}, key}, diff --git a/score/kvs/tests/test_scenarios/cit/multiple_kvs.cpp b/score/kvs/tests/test_scenarios/cit/multiple_kvs.cpp index cb6ce9716..250f66ab6 100644 --- a/score/kvs/tests/test_scenarios/cit/multiple_kvs.cpp +++ b/score/kvs/tests/test_scenarios/cit/multiple_kvs.cpp @@ -25,7 +25,7 @@ namespace { const std::string kTargetName{"cpp_test_scenarios::multiple_kvs"}; -static void InfoLog(const std::string& instance, const std::string& keyname, double value) +void InfoLog(const std::string& instance, const std::string& keyname, double value) { TRACING_INFO(kTargetName, std::pair{std::string{"instance"}, instance}, @@ -99,19 +99,19 @@ class MultipleInstanceIds final : public Scenario auto kvs1{KvsInstance(params1)}; auto kvs2{KvsInstance(params2)}; - auto value1{kvs1.GetValue(keyname)}; - if (!value1) + auto kvs1_value{kvs1.GetValue(keyname)}; + if (!kvs1_value) { throw std::runtime_error{"Failed to read value"}; } - InfoLog("kvs1", keyname, std::get(value1->GetValue())); + InfoLog("kvs1", keyname, std::get(kvs1_value->GetValue())); - auto value2{kvs2.GetValue(keyname)}; - if (!value2) + auto kvs2_value{kvs2.GetValue(keyname)}; + if (!kvs2_value) { throw std::runtime_error{"Failed to read value"}; } - InfoLog("kvs2", keyname, std::get(value2->GetValue())); + InfoLog("kvs2", keyname, std::get(kvs2_value->GetValue())); } } }; @@ -244,19 +244,19 @@ class SameInstanceIdDifferentValue final : public Scenario auto kvs1{KvsInstance(params)}; auto kvs2{KvsInstance(params)}; - auto value1{kvs1.GetValue(keyname)}; - if (!value1) + auto kvs1_value{kvs1.GetValue(keyname)}; + if (!kvs1_value) { throw std::runtime_error{"Failed to read value"}; } - InfoLog("kvs1", keyname, std::get(value1->GetValue())); + InfoLog("kvs1", keyname, std::get(kvs1_value->GetValue())); - auto value2{kvs2.GetValue(keyname)}; - if (!value2) + auto kvs2_value{kvs2.GetValue(keyname)}; + if (!kvs2_value) { throw std::runtime_error{"Failed to read value"}; } - InfoLog("kvs2", keyname, std::get(value2->GetValue())); + InfoLog("kvs2", keyname, std::get(kvs2_value->GetValue())); } } }; diff --git a/score/kvs/tests/test_scenarios/helpers/kvs_instance.cpp b/score/kvs/tests/test_scenarios/helpers/kvs_instance.cpp index df845e7bf..b180198ce 100644 --- a/score/kvs/tests/test_scenarios/helpers/kvs_instance.cpp +++ b/score/kvs/tests/test_scenarios/helpers/kvs_instance.cpp @@ -25,19 +25,19 @@ Kvs KvsInstance(const KvsParameters& params) // Set parameters. if (params.defaults.has_value()) { - builder = builder.Defaults(*params.defaults); + builder.Defaults(*params.defaults); } if (params.kvs_load.has_value()) { - builder = builder.KvsLoad(*params.kvs_load); + builder.KvsLoad(*params.kvs_load); } if (params.dir.has_value()) { - builder = builder.WorkingDir(std::string(*params.dir)); + builder.WorkingDir(std::string(*params.dir)); } if (params.snapshot_max_count.has_value()) { - builder = builder.SnapshotMaxCount(*params.snapshot_max_count); + builder.SnapshotMaxCount(*params.snapshot_max_count); } // Build KVS instance. diff --git a/score/kvs/tests/test_scenarios/helpers/kvs_parameters.cpp b/score/kvs/tests/test_scenarios/helpers/kvs_parameters.cpp index 238ceddba..da2d833a2 100644 --- a/score/kvs/tests/test_scenarios/helpers/kvs_parameters.cpp +++ b/score/kvs/tests/test_scenarios/helpers/kvs_parameters.cpp @@ -70,7 +70,7 @@ KvsParameters KvsParameters::FromValue(const Object& object) const auto& map_root{object.at("kvs_parameters")}; const auto& obj_root{map_root.As().value().get()}; - KvsParameters params{.instance_id = obj_root.at("instance_id").As().value()}; + KvsParameters params{obj_root.at("instance_id").As().value()}; // Deserialize "defaults". params.defaults = DeserializeLoadParam(obj_root, "defaults"); diff --git a/score/kvs/tests/test_scenarios/test_basic.cpp b/score/kvs/tests/test_scenarios/test_basic.cpp index 07f32c654..58387b2b1 100644 --- a/score/kvs/tests/test_scenarios/test_basic.cpp +++ b/score/kvs/tests/test_scenarios/test_basic.cpp @@ -46,11 +46,11 @@ void BasicScenario::run(const std::string& input) const KvsBuilder builder{instance_id}; if (params.defaults.has_value()) { - builder = builder.Defaults(*params.defaults); + builder.Defaults(*params.defaults); } if (params.kvs_load.has_value()) { - builder = builder.KvsLoad(*params.kvs_load); + builder.KvsLoad(*params.kvs_load); } // Create KVS.