testing: adding default values test cases for cpp #189

Saumya-R · 2025-12-11T07:55:32Z

This PR adds C++ test cases for default values scenarios to achieve parity with existing Rust tests. The implementation includes a temporary workaround to create hash files for C++ KVS, which requires them for validation (unlike Rust). The changes enable parameterized testing across both Rust and C++ versions.

Key changes:
Added C++ test scenarios for default values testing (default_values, remove_key, reset operations, checksum)

Created the issue at : #170

Saumya-R · 2025-12-11T07:56:11Z

#171
All the review comments from the PR tagged here are addressed here.

github-actions · 2025-12-11T07:56:36Z

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: e6de08c6-c573-4519-9d51-b5362fd1c9c2
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: 
WARNING: For repository 'rules_rust', the root module requires module version rules_rust@0.56.0, but got rules_rust@0.61.0 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 rules_cc@0.1.1, but got rules_cc@0.1.2 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 platforms@0.0.11, but got platforms@1.0.0 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: 5 packages loaded
Loading: 5 packages loaded
    currently loading: 
Analyzing: target //:license-check (6 packages loaded, 0 targets configured)
Analyzing: target //:license-check (6 packages loaded, 0 targets configured)

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

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

Analyzing: target //:license-check (157 packages loaded, 2244 targets configured)

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

Analyzing: target //:license-check (166 packages loaded, 9001 targets configured)

INFO: Analyzed target //:license-check (166 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: 22.983s, Critical Path: 0.44s
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>]

github-actions · 2025-12-11T07:58:51Z

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

Copilot

Pull request overview

This PR adds C++ test scenarios for default values functionality to achieve parity with existing Rust tests, enabling parameterized testing across both languages. The implementation includes a workaround to generate hash files for C++ KVS validation requirements.

Key changes:

Added C++ test scenarios for default values (default_values, remove_key, reset operations, checksum)
Implemented helper utilities for KVS parameter parsing and instance creation
Modified Python test assertions to accommodate both Rust and C++ execution modes with known bug workarounds

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_default_values.py	Extended parametrization to include "cpp", added FAILURE result code handling, and inserted xfail markers for known C++ bugs
tests/python_test_cases/tests/common.py	Added FAILURE result code constant
tests/cpp_test_scenarios/src/main.cpp	Integrated default values test group into scenario hierarchy
tests/cpp_test_scenarios/src/helpers/kvs_parameters.hpp	Created parameter parsing helper with JSON deserialization and defaults file validation
tests/cpp_test_scenarios/src/helpers/kvs_instance.hpp	Created KVS instance builder helper with parameter-based configuration
tests/cpp_test_scenarios/src/cit/test_default_values.hpp	Declared five scenario classes for default values testing
tests/cpp_test_scenarios/src/cit/test_default_values.cpp	Implemented all default values test scenarios with Python-compatible logging
tests/cpp_test_scenarios/BUILD	Changed to glob pattern for source files and added includes directory

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

tests/python_test_cases/tests/test_cit_default_values.py

tests/cpp_test_scenarios/src/cit/test_default_values.hpp

tests/cpp_test_scenarios/src/cit/test_default_values.cpp

tests/python_test_cases/tests/test_cit_default_values.py

arkjedrz · 2025-12-12T10:10:43Z

tests/cpp_test_scenarios/BUILD

    copts = [
        "-g",
    ],
+    includes = ["src"],


I don't think You need to define includes when .hpp are defined by srcs.

We have sub-folders for helpers function.
We either need to give relative path using "../sub-folder/header" or add includes to let all the files find the header.
Giving relation path vs adding includes: the cleaner method would be adding includes.

tests/cpp_test_scenarios/src/helpers/kvs_instance.hpp

arkjedrz · 2025-12-12T10:15:42Z

tests/cpp_test_scenarios/src/helpers/kvs_parameters.hpp

+
+#include <kvs.hpp>
+
+namespace


Why make an anonymous namespace in a .hpp file?

The changes of this helper functions repeating in name is very low, so namespaces here are not really necessary. Removed them.

arkjedrz · 2025-12-12T10:17:18Z

tests/cpp_test_scenarios/src/cit/test_default_values.hpp

+
+#include "scenario.hpp"
+
+namespace test_default_values


Not sure if You really need namespace?

I asked for that to keep the namespace clean. Main will be aggregating all the tests and some clashes in the future could happen.

tests/cpp_test_scenarios/src/main.cpp

arkjedrz · 2025-12-12T10:20:29Z

tests/cpp_test_scenarios/src/cit/test_default_values.cpp

+namespace test_default_values
+{
+
+    constexpr double kFloatEpsilon = 1e-6;


Those definitions here are a good match for anonymous namespace.

Main will be aggregating all the tests and some clashes in the future could happen so giving them a name would make it cleaner , i feel.

tests/cpp_test_scenarios/src/cit/test_default_values.cpp

tests/cpp_test_scenarios/src/main.cpp

tests/cpp_test_scenarios/src/cit/test_default_values.cpp

igorostrowskiq

Before the merge, should be squashed to a single commit testing: <description>

- Added C++ test scenarios for default values testing - default_values, remove_key, reset operations, checksum

Saumya-R requested review from PiotrKorkus, Copilot and igorostrowskiq December 11, 2025 08:03

Copilot AI reviewed Dec 11, 2025

View reviewed changes

Saumya-R mentioned this pull request Dec 11, 2025

Test Cases for default values in case of CPP added #171

Closed

Saumya-R force-pushed the saumya_default_values_test_cases branch from 914957a to 32679c8 Compare December 12, 2025 07:36

arkjedrz suggested changes Dec 12, 2025

View reviewed changes

igorostrowskiq reviewed Dec 12, 2025

View reviewed changes

tests/cpp_test_scenarios/src/cit/test_default_values.cpp Outdated Show resolved Hide resolved

tests/cpp_test_scenarios/src/cit/test_default_values.cpp Outdated Show resolved Hide resolved

tests/cpp_test_scenarios/src/cit/test_default_values.cpp Outdated Show resolved Hide resolved

igorostrowskiq reviewed Dec 17, 2025

View reviewed changes

tests/cpp_test_scenarios/src/cit/test_default_values.cpp Outdated Show resolved Hide resolved

igorostrowskiq approved these changes Dec 17, 2025

View reviewed changes

Saumya-R force-pushed the saumya_default_values_test_cases branch from 9b9e5cd to 6ba9b41 Compare December 17, 2025 10:42

testing: Adding default values test cases for KVS cpp

77bb280

- Added C++ test scenarios for default values testing - default_values, remove_key, reset operations, checksum

Saumya-R force-pushed the saumya_default_values_test_cases branch from 6ba9b41 to 77bb280 Compare December 17, 2025 10:49

Saumya-R changed the title ~~adding default values test cases for cpp~~ testing: adding default values test cases for cpp Dec 17, 2025

testing: adding default values test cases for cpp #189

Are you sure you want to change the base?

testing: adding default values test cases for cpp #189

Uh oh!

Conversation

Saumya-R commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Saumya-R commented Dec 11, 2025

Uh oh!

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

Uh oh!

License Check Results

Uh oh!

github-actions bot commented Dec 11, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igorostrowskiq left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Saumya-R commented Dec 11, 2025 •

edited

Loading

github-actions bot commented Dec 11, 2025 •

edited

Loading