-
Notifications
You must be signed in to change notification settings - Fork 22
Saumya cit testing add missing cases #223
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?
Saumya cit testing add missing cases #223
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
bbbe842 to
6b59c28
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
This PR adds comprehensive test coverage for key-value store (KVS) component requirements across Rust, C++, and Python test suites. The changes validate key format constraints, UTF-8 encoding, uniqueness guarantees, length limits, supported data types, and JSON serialization.
Changes:
- Added test cases for key validation rules (alphanumeric/underscore/dash only, UTF-8 encoding, uniqueness, 32-byte limit)
- Added test cases for value constraints (supported data types, JSON serialization, 1024-byte limit)
- Enhanced test scenarios to verify boundary conditions for maximum key and value lengths
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| tests/rust_test_scenarios/src/cit/supported_datatypes.rs | Added key validation tests, UTF-8 key tests, uniqueness checks, and boundary tests for 1024/1025 byte string values |
| tests/python_test_cases/tests/test_cit_supported_datatypes.py | Updated test assertions to verify valid/invalid keys, UTF-8 support, uniqueness, and added test classes for 1024/1025 byte strings |
| tests/cpp_test_scenarios/src/cit/supported_datatypes.cpp | Added key validation logic, UTF-8 key tests, uniqueness checks, and test scenarios for maximum length string values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2798638 to
e71670a
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 14 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e4f92df to
0b035f9
Compare
PiotrKorkus
left a comment
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.
Could you check if you are using correct clang rules? All files should be formatted already after #219
It is hard to distinguish between relevant changes and formatting
0b035f9 to
32f7dcc
Compare
ran clang format for cpp adding xfail for uniqueness and length of keys adding xfail for the special char for keys made the comments cleaner format fix alinging to new folders
6edb2c1 to
032f307
Compare
I have rebased again . Should make the changes easy to see. |
arkjedrz
left a comment
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.
Three of requirements tested here should be discussed during forthcoming CFT meeting.
| ) | ||
| @pytest.mark.FullyVerifies([]) | ||
| @pytest.mark.Description("Verifies that KVS supports UTF-8 string keys for storing and retrieving values.") | ||
| @pytest.mark.Description( |
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.
Why requirements are placed in description instead of PartiallyVerifies and FullyVerifies?
| @pytest.mark.xfail( | ||
| reason="KVS does not implement requirement #1: key character set (alphanumeric, underscore, dash) enforcement. It accepts space,special char,invalid keys." | ||
| ) | ||
| def test_ok(self, results: ScenarioResult, logs_info_level: LogContainer) -> None: |
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.
Instead of checking 4 things at one function - separate it to multiple tests.
For example: test_ok to test_valid_chars, test_utf8, test_max_key_size..
| 1. The component shall accept keys that consist solely of alphanumeric characters, underscores, or dashes. | ||
| 2. The component shall encode each key as valid UTF-8. | ||
| 3. The component shall guarantee that each key is unique. | ||
| 4. The component shall limit the maximum length of a key to 32 bytes. |
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.
This requirement should be discussed during CFT meeting - seems unnecessary restrictive.
| @pytest.mark.FullyVerifies([]) | ||
| @pytest.mark.Description("Verifies that KVS supports UTF-8 string keys for storing and retrieving values.") | ||
| @pytest.mark.Description( | ||
| """ |
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.
Again - not sure if that's the right place.
| Test cases for value requirements: | ||
| 5. The component shall accept only values of the following data types: Number, String, Null, Array[Value], or Dictionary{Key:Value}. | ||
| 6. The component shall serialize and deserialize all values to and from JSON. | ||
| 7. The component shall limit the maximum length of a value to 1024 bytes. |
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.
This requirement should be discussed during CFT meeting. Non-string types seem to not be taken into consideration here.
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.
Unnecessary reformat.
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.
Unnecessary reformat.
| @pytest.mark.Description( | ||
| """ | ||
| Test cases for key requirements: | ||
| 1. The component shall accept keys that consist solely of alphanumeric characters, underscores, or dashes. |
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.
This requirement should be discussed during CFT meeting - seems unnecessary restrictive.
Checked and added test cases in RUST and CPP for the following requirements:
3 . The component shall guarantee that each key is unique.