-
Notifications
You must be signed in to change notification settings - Fork 21
testing: adding test cases for multiple kvs in cpp #205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
testing: adding test cases for multiple kvs in cpp #205
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds C++ test cases for multiple KVS (Key-Value Store) instances, extending test coverage beyond the existing Rust implementation.
Key Changes:
- Extended Python test parametrization to include "cpp" alongside "rust"
- Implemented three C++ test scenarios for multiple KVS instances with different configurations
- Added helper utilities for KVS parameter parsing and instance creation
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/python_test_cases/tests/test_cit_multiple_kvs.py | Added "cpp" to test parametrization and modified assertions to use rounding for floating-point comparison |
| tests/cpp_test_scenarios/src/main.cpp | Added CIT test group registration and reformatted code structure |
| tests/cpp_test_scenarios/src/helpers/kvs_parameters.hpp | New header defining KvsParameters struct for test configuration |
| tests/cpp_test_scenarios/src/helpers/kvs_parameters.cpp | Implements JSON parsing logic to extract KVS parameters from test input |
| tests/cpp_test_scenarios/src/helpers/kvs_instance.hpp | New header declaring kvs_instance factory function |
| tests/cpp_test_scenarios/src/helpers/kvs_instance.cpp | Implements KVS instance builder using parsed parameters |
| tests/cpp_test_scenarios/src/cit/test_cit_multiple_kvs.hpp | Declares three test scenario classes and group creation function |
| tests/cpp_test_scenarios/src/cit/test_cit_multiple_kvs.cpp | Implements test scenarios for multiple KVS instances with value setting/retrieval |
| tests/cpp_test_scenarios/BUILD | Updated build configuration to use glob pattern for source files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The created documentation from the pull request is available at: docu-html |
3c7d065 to
d350af5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| ********************************************************************************/ | ||
| #include <exception> |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The <exception> include is unnecessary since std::exception is already available through other includes. Consider removing this redundant include.
| #include <exception> |
| auto it2 = obj.find("kvs_parameters_2"); | ||
| if (it1 == obj.end() || it2 == obj.end()) | ||
| { | ||
| std::cout << "[DEBUG] FINDING Error: " << any_res.error().Message() << std::endl; |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message accesses any_res.error() even though any_res was successfully parsed at this point (line 149). This will likely cause a runtime error or undefined behavior. The error handling should use a different error message or create a proper error object.
| std::cout << "[DEBUG] FINDING Error: " << any_res.error().Message() << std::endl; | |
| std::cout << "[DEBUG] FINDING Error: Missing kvs_parameters_1 or kvs_parameters_2 in parsed JSON object" << std::endl; |
| } | ||
| catch (const std::exception &e) | ||
| { | ||
| throw e; |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-throwing the caught exception with throw e; causes object slicing and loses the original exception type. Use throw; instead to preserve the exception type.
| throw e; | |
| throw; |
| catch (const std::exception &e) | ||
| { | ||
|
|
||
| throw e; |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-throwing the caught exception with throw e; causes object slicing and loses the original exception type. Use throw; instead to preserve the exception type.
| throw e; | |
| throw; |
This PR adds C++ test cases for multiple KVS (Key-Value Store) instances, extending test coverage beyond the existing Rust implementation.
Key Changes: