adding test cases for snapshot #184

Saumya-R · 2025-12-05T05:47:23Z

Test cases for cpp kvs added for snapshots.

Copilot

Pull request overview

This PR adds comprehensive C++ test coverage for KVS (Key-Value Store) snapshot functionality, enabling the existing Python test suite to run against both Rust and C++ implementations. The changes introduce new C++ test scenarios that mirror the Python snapshot tests, along with necessary helper utilities for KVS parameter parsing and instance creation.

Key Changes:

Extended Python test parametrization to include C++ version alongside Rust
Added C++ implementations for four snapshot test scenarios (count, max_count, restore, paths)
Created helper utilities for KVS parameter parsing and instance management

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tests/python_test_cases/tests/test_cit_snapshots.py	Extended test parametrization to run snapshot tests against both "rust" and "cpp" versions; added xfail markers for known C++ bugs
tests/cpp_test_scenarios/src/test_objects/test_object_snapshot.hpp	Defined snapshot test scenario objects and grouped them into a scenario group
tests/cpp_test_scenarios/src/main.cpp	Integrated snapshot test group into the main test runner hierarchy
tests/cpp_test_scenarios/src/helpers/kvs_parameters.hpp	Implemented JSON parsing and validation for KVS configuration parameters
tests/cpp_test_scenarios/src/helpers/kvs_instance.hpp	Created utility function to instantiate KVS objects from parsed parameters
tests/cpp_test_scenarios/src/cit/test_snapshots.hpp	Declared four snapshot test scenario classes (SnapshotCount, SnapshotMaxCount, SnapshotRestore, SnapshotPaths)
tests/cpp_test_scenarios/src/cit/test_snapshots.cpp	Implemented snapshot test scenarios with detailed comments documenting known C++ implementation issues
tests/cpp_test_scenarios/BUILD	Updated build configuration to use glob patterns for source files and added include paths for new directory structure

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/python_test_cases/tests/test_cit_snapshots.py

tests/cpp_test_scenarios/src/test_objects/test_object_snapshot.hpp

tests/cpp_test_scenarios/src/cit/test_snapshots.cpp

github-actions · 2025-12-05T05:50:33Z

The created documentation from the pull request is available at: docu-html

github-actions · 2025-12-05T05:50:35Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.3.0) and connecting to it...
INFO: Invocation ID: 7aa75b5b-5e1f-4860-9796-3e84d011915f
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
DEBUG: Rule 'rust_qnx8_toolchain+' indicated that a canonical reproducible form can be obtained by modifying arguments integrity = "sha256-oEubHgeZDdT0svMmBKJx7c3/2TdSI/vfwRUyDn+TPGA="
DEBUG: Repository rust_qnx8_toolchain+ instantiated at:
  <builtin>: in <toplevel>
Repository rule http_archive defined at:
  /home/runner/.bazel/external/bazel_tools/tools/build_defs/repo/http.bzl:394:31: in <toplevel>
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
WARNING: For repository 'rules_rust', the root module requires module version [email protected], but got [email protected] in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'rules_cc', the root module requires module version [email protected], but got [email protected] in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'platforms', the root module requires module version [email protected], but got [email protected] in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
Computing main repo mapping: 
Loading: 
Loading: 4 packages loaded
Loading: 4 packages loaded
    currently loading: 
Analyzing: target //:license-check (5 packages loaded, 0 targets configured)
Analyzing: target //:license-check (5 packages loaded, 0 targets configured)

Analyzing: target //:license-check (84 packages loaded, 9 targets configured)

Analyzing: target //:license-check (92 packages loaded, 9 targets configured)

Analyzing: target //:license-check (155 packages loaded, 1211 targets configured)

Analyzing: target //:license-check (162 packages loaded, 6985 targets configured)

Analyzing: target //:license-check (165 packages loaded, 8995 targets configured)

INFO: Analyzed target //:license-check (165 packages loaded, 9001 targets configured).
INFO: From Generating Dash formatted dependency file ...:
INFO: Successfully converted 65 packages from Cargo.lock to bazel-out/k8-fastbuild/bin/formatted.txt
INFO: Found 1 target...
Target //:license.check.license_check up-to-date:
  bazel-bin/license.check.license_check
  bazel-bin/license.check.license_check.jar
INFO: Elapsed time: 24.601s, Critical Path: 0.41s
INFO: 14 processes: 5 disk cache hit, 9 internal.
INFO: Build completed successfully, 14 total actions
INFO: Running command line: bazel-bin/license.check.license_check ./formatted.txt <args omitted>
usage: org.eclipse.dash.licenses.cli.Main [-batch <int>] [-cd <url>]
       [-confidence <int>] [-ef <url>] [-excludeSources <sources>] [-help] [-lic
       <url>] [-project <shortname>] [-repo <url>] [-review] [-summary <file>]
       [-timeout <seconds>] [-token <token>]

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T06:07:01Z

tests/python_test_cases/tests/test_cit_snapshots.py


+    @pytest.mark.xfail(
+        condition=lambda version: version == "cpp",
+        reason="Known bug in CPP code : ?",


The xfail reason is incomplete with '?'. Based on the detailed comments in test_snapshots.cpp (lines 48-58), this should reference the snapshot count issues after first flush where C++ emits snapshot_count: 0 instead of the expected min(count, snapshot_max_count).

Suggested change

reason="Known bug in CPP code : ?",

reason="Known bug in C++: after first flush, C++ emits snapshot_count: 0 instead of expected min(count, snapshot_max_count) (see test_snapshots.cpp lines 48-58)",

tests/cpp_test_scenarios/src/helpers/kvs_parameters.hpp

tests/cpp_test_scenarios/src/cit/test_snapshots.cpp

tests/cpp_test_scenarios/BUILD

tests/python_test_cases/tests/test_cit_snapshots.py

PiotrKorkus · 2025-12-08T13:22:09Z

tests/python_test_cases/tests/test_cit_snapshots.py


+    @pytest.mark.xfail(
+        condition=lambda version: version == "cpp",
+        reason="Known bug in CPP code : ?",


fill link to the BT

PiotrKorkus · 2025-12-08T13:28:57Z

tests/cpp_test_scenarios/src/test_basic.cpp

this file shouldnt be modified. Please check your IDE settings and use 4 spaces for tab

tests/cpp_test_scenarios/src/main.cpp

PiotrKorkus · 2025-12-08T13:31:39Z

tests/cpp_test_scenarios/src/helpers/kvs_parameters.hpp

+#include <fstream>
+#include <kvs.hpp>
+#include <kvsbuilder.hpp>
+#include <nlohmann/json.hpp>


do we need it, as we have baselibs json parser?

tests/cpp_test_scenarios/src/helpers/kvs_parameters.hpp

PiotrKorkus · 2025-12-08T13:48:28Z

tests/cpp_test_scenarios/src/cit/test_snapshots.cpp

+  KvsParameters params = map_to_params(input);
+
+  for (uint8_t i = 0; i < count; ++i) {
+    auto kvs = kvs_instance(params);


do we need to define an instance in a loop? Cant we do it before loop and just flush in loop?

PiotrKorkus · 2025-12-08T14:04:22Z

tests/cpp_test_scenarios/src/cit/test_snapshots.cpp

+  {
+    auto kvs = kvs_instance(params);
+    auto restore_res = kvs.snapshot_restore(snapshot_id);
+    // Emit expected error messages to stderr for test compatibility


im not sure if we should do it like this or the impl should be changed

adding test cases for snapshot

fa39281

Saumya-R requested a review from Copilot December 5, 2025 05:47

Copilot AI reviewed Dec 5, 2025

View reviewed changes

adding format fixes and copyrights.

88d82e8

Saumya-R requested a review from Copilot December 5, 2025 06:05

Copilot AI reviewed Dec 5, 2025

View reviewed changes

Saumya-R requested review from PiotrKorkus and igorostrowskiq December 5, 2025 06:07

Saumya-R added 2 commits December 5, 2025 11:41

removing the redundant includes in build

8c5af08

cleaning the main of test and clang-format applied

ed53085

PiotrKorkus reviewed Dec 8, 2025

View reviewed changes

removing redundant header inclusion

462bcda

Saumya-R marked this pull request as draft December 11, 2025 08:27

Saumya-R removed the request for review from igorostrowskiq December 11, 2025 08:27

Saumya-R closed this Dec 11, 2025

Saumya-R deleted the saumya_snapshot_cpp_test_cases branch December 11, 2025 08:28

	reason="Known bug in CPP code : ?",
	reason="Known bug in C++: after first flush, C++ emits snapshot_count: 0 instead of expected min(count, snapshot_max_count) (see test_snapshots.cpp lines 48-58)",

adding test cases for snapshot #184

adding test cases for snapshot #184

Uh oh!

Conversation

Saumya-R commented Dec 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 5, 2025

Uh oh!

github-actions bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Check Results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PiotrKorkus Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

PiotrKorkus Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PiotrKorkus Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PiotrKorkus Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

PiotrKorkus Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Dec 5, 2025 •

edited

Loading