Skip to content

Commit effcf28

Browse files
Saumya-RSaumya-R
committed
review fixes added
- moving code to namespace - add empty lines - remove default value vector - move comparison to function - added enum for FAILURE result code adding spaces to comments
1 parent 3207275 commit effcf28

File tree

7 files changed

+72
-76
lines changed

7 files changed

+72
-76
lines changed

‎tests/cpp_test_scenarios/src/cit/test_default_values.cpp‎

Lines changed: 55 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -18,33 +18,15 @@
1818
#include <sstream>
1919

2020
using score::mw::per::kvs::KvsValue;
21+
2122
namespace test_default_values {
2223

2324
constexpr double kFloatEpsilon = 1e-6;
2425
const std::string kTargetName{"cpp_test_scenarios::cit::default_values"};
2526

26-
std::vector<std::shared_ptr<const Scenario>> get_default_value_scenarios() {
27-
std::vector<std::shared_ptr<const Scenario>> scenarios;
28-
scenarios.emplace_back(std::make_shared<DefaultValuesScenario>());
29-
scenarios.emplace_back(std::make_shared<RemoveKeyScenario>());
30-
scenarios.emplace_back(std::make_shared<ResetAllKeysScenario>());
31-
scenarios.emplace_back(std::make_shared<ResetSingleKeyScenario>());
32-
scenarios.emplace_back(std::make_shared<ChecksumScenario>());
33-
return scenarios;
34-
}
35-
36-
// Default values group
37-
ScenarioGroup::Ptr create_default_values_group() {
38-
return ScenarioGroup::Ptr{
39-
new ScenarioGroupImpl{"default_values",
40-
{std::make_shared<DefaultValuesScenario>(),
41-
std::make_shared<RemoveKeyScenario>(),
42-
std::make_shared<ResetAllKeysScenario>(),
43-
std::make_shared<ResetSingleKeyScenario>(),
44-
std::make_shared<ChecksumScenario>()},
45-
{}}};
27+
inline bool float_equal(double a, double b, double epsilon = kFloatEpsilon) {
28+
return std::abs(a - b) < epsilon;
4629
}
47-
} // namespace test_default_values
4830

4931
/**
5032
* Helper to log key/value state in a format parsable by Python tests.
@@ -64,8 +46,7 @@ static void info_log(const std::string& key,
6446
const std::string& value_is_default,
6547
const std::string& default_value,
6648
const std::string& current_value) {
67-
TRACING_INFO(test_default_values::kTargetName,
68-
std::pair{std::string{"key"}, key},
49+
TRACING_INFO(kTargetName, std::pair{std::string{"key"}, key},
6950
std::pair{std::string{"value_is_default"}, value_is_default},
7051
std::pair{std::string{"default_value"}, default_value},
7152
std::pair{std::string{"current_value"}, current_value});
@@ -88,14 +69,14 @@ static void info_log(const std::string& key,
8869
template <typename T>
8970
static void info_log(const std::string& key, const bool value_is_default,
9071
T current_value) {
91-
TRACING_INFO(test_default_values::kTargetName,
92-
std::pair{std::string{"key"}, key},
72+
TRACING_INFO(kTargetName, std::pair{std::string{"key"}, key},
9373
std::pair{std::string{"value_is_default"}, value_is_default},
9474
std::pair{std::string{"current_value"}, current_value});
9575
}
9676

9777
std::string DefaultValuesScenario::name() const { return "default_values"; }
9878
void DefaultValuesScenario::run(const std::string& input) const {
79+
9980
using namespace score::mw::per::kvs;
10081
std::string key = "test_number";
10182
auto params = map_to_params(input);
@@ -108,6 +89,7 @@ void DefaultValuesScenario::run(const std::string& input) const {
10889
std::string value_is_default;
10990
std::string default_value;
11091
std::string current_value;
92+
11193
// Default value
11294
if (!get_default_result.has_value() ||
11395
get_default_result.value().getType() != KvsValue::Type::f64 ||
@@ -121,6 +103,7 @@ void DefaultValuesScenario::run(const std::string& input) const {
121103
<< std::get<double>(get_default_result.value().getValue());
122104
default_value = "Ok(F64(" + oss.str() + "))";
123105
}
106+
124107
// Current value
125108
if (!get_value_result.has_value() ||
126109
get_value_result.value().getType() != KvsValue::Type::f64 ||
@@ -134,24 +117,27 @@ void DefaultValuesScenario::run(const std::string& input) const {
134117
<< std::get<double>(get_value_result.value().getValue());
135118
current_value = "Ok(F64(" + oss.str() + "))";
136119
}
120+
137121
// value_is_default
138122
if (default_value == "Err(KeyNotFound)" ||
139123
current_value == "Err(KeyNotFound)") {
140124
value_is_default = "Err(KeyNotFound)";
141-
} else if (std::abs(
142-
std::get<double>(get_default_result.value().getValue()) -
143-
std::get<double>(get_value_result.value().getValue())) <
144-
test_default_values::kFloatEpsilon) {
125+
} else if (float_equal(
126+
std::get<double>(get_default_result.value().getValue()),
127+
std::get<double>(get_value_result.value().getValue()))) {
145128
value_is_default = "Ok(true)";
146129
} else {
147130
value_is_default = "Ok(false)";
148131
}
132+
149133
info_log(key, value_is_default, default_value, current_value);
134+
150135
auto set_result = kvs.set_value(key, KvsValue{432.1});
151136
if (!set_result)
152137
throw std::runtime_error("Failed to set value");
153138
kvs.flush();
154139
}
140+
155141
{
156142
// Second check: log after set_value and flush
157143
// - value_is_default: Ok(true) if value == default, Ok(false) if not,
@@ -177,15 +163,15 @@ void DefaultValuesScenario::run(const std::string& input) const {
177163
cur_val->getType() == KvsValue::Type::f64;
178164
if (both_f64) {
179165
try {
180-
double v = std::get<double>(cur_val->getValue());
181-
double d = std::get<double>(def_val->getValue());
182-
if (std::abs(v - d) < test_default_values::kFloatEpsilon)
166+
if (float_equal(std::get<double>(cur_val->getValue()),
167+
std::get<double>(def_val->getValue())))
183168
value_is_default = "Ok(true)";
184169
} catch (const std::bad_variant_access& e) {
185170
throw;
186171
}
187172
}
188173
}
174+
189175
// Format default_value for log
190176
if (get_default_ok && def_val->getType() == KvsValue::Type::f64) {
191177
try {
@@ -203,6 +189,7 @@ void DefaultValuesScenario::run(const std::string& input) const {
203189
} else {
204190
default_value = "Err(KeyNotFound)";
205191
}
192+
206193
// Format current_value for log
207194
if (get_value_ok && cur_val->getType() == KvsValue::Type::f64) {
208195
try {
@@ -225,6 +212,7 @@ void DefaultValuesScenario::run(const std::string& input) const {
225212
current_value); // Log after set/flush
226213
}
227214
}
215+
228216
std::string RemoveKeyScenario::name() const { return "remove_key"; }
229217
void RemoveKeyScenario::run(const std::string& input) const {
230218
using namespace score::mw::per::kvs;
@@ -237,6 +225,7 @@ void RemoveKeyScenario::run(const std::string& input) const {
237225
std::string value_is_default;
238226
std::string default_value;
239227
std::string current_value;
228+
240229
// Default value
241230
if (!get_default || get_default->getType() != KvsValue::Type::f64 ||
242231
!std::holds_alternative<double>(get_default->getValue())) {
@@ -247,6 +236,7 @@ void RemoveKeyScenario::run(const std::string& input) const {
247236
oss << std::fixed << std::get<double>(get_default->getValue());
248237
default_value = "Ok(F64(" + oss.str() + "))";
249238
}
239+
250240
// Current value
251241
if (!get_value || get_value->getType() != KvsValue::Type::f64 ||
252242
!std::holds_alternative<double>(get_value->getValue())) {
@@ -257,33 +247,36 @@ void RemoveKeyScenario::run(const std::string& input) const {
257247
oss << std::fixed << std::get<double>(get_value->getValue());
258248
current_value = "Ok(F64(" + oss.str() + "))";
259249
}
250+
260251
// value_is_default
261252
if (default_value == "Err(KeyNotFound)" ||
262253
current_value == "Err(KeyNotFound)") {
263254
value_is_default = "Err(KeyNotFound)";
264-
} else if (std::abs(std::get<double>(get_default->getValue()) -
265-
std::get<double>(get_value->getValue())) <
266-
test_default_values::kFloatEpsilon) {
255+
} else if (float_equal(std::get<double>(get_default->getValue()),
256+
std::get<double>(get_value->getValue()))) {
267257
value_is_default = "Ok(true)";
268258
} else {
269259
value_is_default = "Ok(false)";
270260
}
261+
271262
info_log(key, value_is_default, default_value, current_value);
263+
272264
auto set_result = kvs.set_value(key, KvsValue{432.1});
273265
if (!set_result)
274266
throw std::runtime_error("Failed to set value");
275267
get_value = kvs.get_value(key);
268+
276269
// Second check: log after set_value
277270
// - value_is_default: Ok(true) if value == default, Ok(false) if not
278271
value_is_default = "Ok(false)";
279272
if (get_value && get_default &&
280273
get_value->getType() == get_default->getType() &&
281274
get_value->getType() == KvsValue::Type::f64) {
282-
double v = std::get<double>(get_value->getValue());
283-
double d = std::get<double>(get_default->getValue());
284-
if (std::abs(v - d) < test_default_values::kFloatEpsilon)
275+
if (float_equal(std::get<double>(get_value->getValue()),
276+
std::get<double>(get_default->getValue())))
285277
value_is_default = "Ok(true)";
286278
}
279+
287280
// Format current_value for log
288281
if (get_value && get_value->getType() == KvsValue::Type::f64) {
289282
std::ostringstream oss;
@@ -293,12 +286,15 @@ void RemoveKeyScenario::run(const std::string& input) const {
293286
} else {
294287
current_value = "Err(KeyNotFound)";
295288
}
289+
296290
info_log(key, value_is_default, default_value,
297291
current_value); // Log after set
292+
298293
auto remove_result = kvs.remove_key(key);
299294
if (!remove_result)
300295
throw std::runtime_error("Failed to remove key");
301296
get_value = kvs.get_value(key);
297+
302298
// Third check: log after remove_key
303299
// - value_is_default: Err(KeyNotFound) if default missing, Ok(true) if
304300
// value
@@ -311,12 +307,12 @@ void RemoveKeyScenario::run(const std::string& input) const {
311307
if (get_value && get_default &&
312308
get_value->getType() == get_default->getType() &&
313309
get_value->getType() == KvsValue::Type::f64) {
314-
double v = std::get<double>(get_value->getValue());
315-
double d = std::get<double>(get_default->getValue());
316-
if (std::abs(v - d) < test_default_values::kFloatEpsilon)
310+
if (float_equal(std::get<double>(get_value->getValue()),
311+
std::get<double>(get_default->getValue())))
317312
value_is_default = "Ok(true)";
318313
}
319314
}
315+
320316
// Format current_value for log
321317
if (get_value && get_value->getType() == KvsValue::Type::f64) {
322318
std::ostringstream oss;
@@ -332,7 +328,6 @@ void RemoveKeyScenario::run(const std::string& input) const {
332328
}
333329

334330
std::string ResetAllKeysScenario::name() const { return "reset_all_keys"; }
335-
336331
void ResetAllKeysScenario::run(const std::string& input) const {
337332
using namespace score::mw::per::kvs;
338333
const int num_values = 5;
@@ -430,13 +425,15 @@ void ChecksumScenario::run(const std::string& input) const {
430425
try {
431426
auto kvs = kvs_instance(params);
432427
kvs.flush();
428+
433429
// Get kvs_path
434430
auto kvs_path_res = kvs.get_kvs_filename(0);
435431
if (kvs_path_res.has_value()) {
436432
kvs_path = static_cast<std::string>(kvs_path_res.value());
437433
} else {
438434
kvs_path = "";
439435
}
436+
440437
// Get hash_path
441438
auto hash_path_res = kvs.get_hash_filename(0);
442439
if (hash_path_res.has_value()) {
@@ -448,8 +445,21 @@ void ChecksumScenario::run(const std::string& input) const {
448445
kvs_path = "";
449446
hash_path = "";
450447
}
448+
451449
// Log using Rust-compatible field names for Python test parsing
452-
TRACING_INFO(test_default_values::kTargetName,
453-
std::pair{std::string{"kvs_path"}, kvs_path},
450+
TRACING_INFO(kTargetName, std::pair{std::string{"kvs_path"}, kvs_path},
454451
std::pair{std::string{"hash_path"}, hash_path});
455-
}
452+
}
453+
454+
// Default values group
455+
ScenarioGroup::Ptr create_default_values_group() {
456+
return ScenarioGroup::Ptr{
457+
new ScenarioGroupImpl{"default_values",
458+
{std::make_shared<DefaultValuesScenario>(),
459+
std::make_shared<RemoveKeyScenario>(),
460+
std::make_shared<ResetAllKeysScenario>(),
461+
std::make_shared<ResetSingleKeyScenario>(),
462+
std::make_shared<ChecksumScenario>()},
463+
{}}};
464+
}
465+
} // namespace test_default_values

‎tests/cpp_test_scenarios/src/cit/test_default_values.hpp‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
#include "scenario.hpp"
1616

17+
namespace test_default_values {
18+
1719
class DefaultValuesScenario final : public Scenario {
1820
public:
1921
~DefaultValuesScenario() final = default;
@@ -49,9 +51,6 @@ class ChecksumScenario final : public Scenario {
4951
void run(const std::string& input) const final;
5052
};
5153

52-
namespace test_default_values {
53-
// Helper to get all scenarios
54-
std::vector<std::shared_ptr<const Scenario>> get_default_value_scenarios();
5554
// Default values group
5655
ScenarioGroup::Ptr create_default_values_group();
5756
} // namespace test_default_values

‎tests/cpp_test_scenarios/src/helpers/kvs_instance.hpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ static score::mw::per::kvs::Kvs kvs_instance(const KvsParameters& params) {
1919
using namespace score::mw::per::kvs;
2020
InstanceId instance_id{params.instance_id};
2121
KvsBuilder builder{instance_id};
22+
2223
if (params.need_defaults.has_value()) {
2324
builder = builder.need_defaults_flag(*params.need_defaults);
2425
}
@@ -28,12 +29,14 @@ static score::mw::per::kvs::Kvs kvs_instance(const KvsParameters& params) {
2829
if (params.dir.has_value()) {
2930
builder = builder.dir(std::string(*params.dir));
3031
}
32+
3133
auto kvs_ptr = builder.build();
3234
if (!kvs_ptr) {
3335
throw ScenarioError(
3436
score::mw::per::kvs::ErrorCode::JsonParserError,
3537
"KVS creation failed: build() returned null (possible "
3638
"file not found, JSON parse error, or corruption)");
3739
}
40+
3841
return std::move(*kvs_ptr);
39-
}
42+
}

‎tests/cpp_test_scenarios/src/helpers/kvs_parameters.hpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ KvsParameters map_to_params(const std::string& data) {
4646

4747
KvsParameters params;
4848
params.instance_id = obj_root.at("instance_id").As<double>().value();
49+
4950
// Precedence: direct 'need_defaults' field overrides inference from
5051
// 'defaults'.
5152
if (obj_root.find("need_defaults") != obj_root.end()) {
@@ -63,9 +64,11 @@ KvsParameters map_to_params(const std::string& data) {
6364
}
6465
}
6566
}
67+
6668
if (obj_root.find("need_kvs") != obj_root.end()) {
6769
params.need_kvs = obj_root.at("need_kvs").As<bool>().value();
6870
}
71+
6972
if (obj_root.find("dir") != obj_root.end()) {
7073
params.dir = obj_root.at("dir").As<std::string>().value();
7174
}
@@ -88,4 +91,4 @@ KvsParameters map_to_params(const std::string& data) {
8891
return params;
8992
}
9093

91-
} // namespace
94+
} // namespace

‎tests/cpp_test_scenarios/src/main.cpp‎

Lines changed: 2 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -36,31 +36,9 @@ int main(int argc, char** argv) {
3636
TestContext test_context{root_group};
3737

3838
run_cli_app(raw_arguments, test_context);
39-
} catch (const ScenarioError& ex) {
40-
using score::mw::per::kvs::ErrorCode;
41-
switch (ex.code) {
42-
case ErrorCode::KvsFileReadError:
43-
case ErrorCode::KvsHashFileReadError:
44-
case ErrorCode::JsonParserError:
45-
case ErrorCode::ValidationFailed:
46-
std::cerr << "[EXCEPTION] Critical error: " << ex.what()
47-
<< std::endl;
48-
return 101;
49-
default:
50-
std::cerr << "[EXCEPTION] Non-critical runtime error: " << ex.what()
51-
<< std::endl;
52-
return 202;
53-
}
54-
} catch (const std::runtime_error& ex) {
55-
std::cerr << "[EXCEPTION] std::runtime_error: " << ex.what()
56-
<< std::endl;
57-
return 102;
5839
} catch (const std::exception& ex) {
59-
std::cerr << "[EXCEPTION] std::exception: " << ex.what() << std::endl;
60-
return 103;
61-
} catch (...) {
62-
std::cerr << "[EXCEPTION] Unknown exception" << std::endl;
63-
return 104;
40+
std::cerr << ex.what() << std::endl;
41+
return 1;
6442
}
6543
return 0;
6644
}

0 commit comments

Comments
 (0)