From d11e58293cf8f2be89c0f95befcf3f20ed51c2d0 Mon Sep 17 00:00:00 2001 From: bhuvan-somisetty Date: Tue, 15 Sep 2026 20:28:44 +0530 Subject: [PATCH] test: audit cpp runtime against protocol conformance matrix (Phase 2 follow-up) Extends the cross-runtime matrix from #440/#467 to cover cpp, which was still all not_audited. Traced concore.hpp/concore_base.hpp against the 8 phase-1 fixtures by hand: - both parse_params cases fail: load_params() globally replaces every ',' and '=' before building a dict literal, so any comma- or equals-containing value (an array literal, a URL) corrupts the conversion and load_params() silently returns an empty map - the mixed-type initval case fails: flatten_numeric() silently drops non-numeric elements, so a string in the payload disappears instead of being preserved, shifting the remaining values - the other 5 cases (empty-input initval, both write_zmq cases, both read_file cases) match Python's semantics Also wires TestLiteralEvalCpp.cpp and TestConcoreHpp.cpp into CI. Both files already existed (#389, #484) with proper pass/fail exit codes, but the only cpp CI job only ran -fsyntax-only on a different file, so these never actually executed and a regression in either would have stayed green. Report-only, no runtime behavior changed. Refs #440 #574 --- .github/workflows/ci.yml | 10 ++++++ .../cross_runtime_matrix.phase2.json | 32 +++++++++---------- tests/test_protocol_conformance_phase2.py | 27 ++++++++++++++++ 3 files changed, 53 insertions(+), 16 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a226a6b..618fe82 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -191,3 +191,13 @@ jobs: # segments to succeed. This catches header-level regressions on # the SHM transport (issue #195) without needing g++ at runtime. run: g++ -std=c++17 -Wall -Wextra -I. -fsyntax-only tests/test_shm_cpp_smoke.cpp + + - name: Build and run C++ parser tests + run: | + g++ -std=c++11 -Wall -Wextra -I. -o TestLiteralEvalCpp TestLiteralEvalCpp.cpp + ./TestLiteralEvalCpp + + - name: Build and run C++ Concore API tests + run: | + g++ -std=c++11 -Wall -Wextra -I. -o TestConcoreHpp TestConcoreHpp.cpp + ./TestConcoreHpp diff --git a/tests/protocol_fixtures/cross_runtime_matrix.phase2.json b/tests/protocol_fixtures/cross_runtime_matrix.phase2.json index 712ba96..0ffda31 100644 --- a/tests/protocol_fixtures/cross_runtime_matrix.phase2.json +++ b/tests/protocol_fixtures/cross_runtime_matrix.phase2.json @@ -32,9 +32,9 @@ "note": "Phase-1 baseline execution." }, "cpp": { - "status": "not_audited", + "status": "observed_fail", "classification": "required", - "note": "Audit planned in phase 2." + "note": "concore_base.hpp load_params() converts \"a=1;b=2\" style params to a JSON-like dict literal by globally replacing every ',' with ',\"' before handing the string to parsedict(). Any param value containing a comma (e.g. an array literal like coeffs=[1,2,3]) gets its commas mangled into invalid syntax, parsedict() throws, and load_params() silently returns an empty map instead of {delay:5, coeffs:[1,2,3], label:\"hello world\"}. Verified by tracing the exact regex pipeline (semicolon->comma, comma->,\", =->\":, space strip) against this fixture's input; not caught by TestConcoreHpp.cpp's test_load_params_semicolon_format, which only exercises comma-free values." }, "java": { "status": "observed_pass", @@ -68,9 +68,9 @@ "note": "Phase-1 baseline execution." }, "cpp": { - "status": "not_audited", + "status": "observed_fail", "classification": "required", - "note": "Audit planned in phase 2." + "note": "Same load_params() conversion in concore_base.hpp also does a global '=' -> '\":' replacement, so every '=' in the value (not just the key/value separator) gets treated as a delimiter. For this fixture's URL value, that produces multiple stray \":\" tokens and parsedict() throws, so load_params() returns an empty map instead of {url: \"https://example.com?a=1&b=2\"}. Same root cause as parse_params/simple_types_and_whitespace." }, "java": { "status": "observed_pass", @@ -104,9 +104,9 @@ "note": "Phase-1 baseline execution." }, "cpp": { - "status": "not_audited", + "status": "observed_fail", "classification": "required", - "note": "Audit planned in phase 2." + "note": "Concore::initval() (concore.hpp) parses through concore_base::flatten_numeric, whose STRING case is a no-op ('case ConcoreValueType::STRING: break;' in concore_base.hpp), so non-numeric elements are silently dropped rather than preserved. For \"[12.5, \\\"a\\\", 3]\", flatten_numeric produces {12.5, 3.0}, simtime is correctly set to 12.5, but initval() returns [3.0] instead of [\"a\", 3] -- the string is lost with no error and the remaining values shift position. Root cause is that the public initval() API returns vector, which cannot represent a string element at all; concore_base.hpp's own parser (parse_literal) does support strings, only the numeric-flattening call site loses them. Not covered by TestConcoreHpp.cpp's test_initval_parses_simtime_and_data, which only uses an all-numeric payload. Verified by tracing flatten_numeric_impl and initval() directly." }, "java": { "status": "observed_pass", @@ -140,9 +140,9 @@ "note": "Phase-1 baseline execution." }, "cpp": { - "status": "not_audited", + "status": "observed_pass", "classification": "required", - "note": "Audit planned in phase 2." + "note": "For non-list-shaped literal input that parse_literal() itself fails to parse (e.g. bare text with no enclosing '[' or '('), concore_base::parselist_double() falls into its catch-all fallback and returns an empty vector; Concore::initval() returns that empty vector immediately, before ever touching simtime. Matches this fixture. Verified by tracing parselist_double()'s exception fallback and initval()'s early-return-on-empty; TestConcoreHpp.cpp's test_initval_empty_input_returns_empty covers the same early-return path with a different input (\"[]\"), not this exact non-bracketed string." }, "java": { "status": "observed_pass", @@ -176,9 +176,9 @@ "note": "Phase-1 baseline execution." }, "cpp": { - "status": "not_audited", + "status": "observed_pass", "classification": "required", - "note": "Audit planned in phase 2." + "note": "write_ZMQ(vector) in concore.hpp inserts simtime+delta at the front of the outgoing payload and sends it, without writing back to the Concore instance's simtime member (the code even carries an explicit comment, 'simtime must not be mutated here (issue #385)'). Matches this fixture's semantics. Gated behind the CONCORE_USE_ZMQ build macro; TestConcoreHpp.cpp has no ZMQ coverage, so this was verified by reading concore.hpp directly rather than by an existing test." }, "java": { "status": "observed_pass", @@ -212,9 +212,9 @@ "note": "Phase-1 baseline execution." }, "cpp": { - "status": "not_audited", + "status": "observed_pass", "classification": "required", - "note": "Audit planned in phase 2." + "note": "write_ZMQ(string) in concore.hpp sends the string as-is with no timestamp prepended and no simtime mutation, matching this fixture. Same CONCORE_USE_ZMQ caveat as the list-payload case; verified by reading concore.hpp, no existing ZMQ test exercises it." }, "java": { "status": "observed_pass", @@ -248,9 +248,9 @@ "note": "Phase-1 baseline execution." }, "cpp": { - "status": "not_audited", + "status": "observed_pass", "classification": "required", - "note": "Audit planned in phase 2." + "note": "read_FM() in concore.hpp falls back to parsing initstr and sets ReadStatus::FILE_NOT_FOUND when the port file doesn't exist, returning the init default with simtime left unchanged. Same value/monotonic semantics as this fixture, though C++ exposes an enum status (last_read_status / ReadResult.status) rather than a boolean 'ok' flag. Directly covered by TestConcoreHpp.cpp's test_read_FM_missing_file_uses_initstr and test_read_result_missing_file_status." }, "java": { "status": "observed_pass", @@ -284,9 +284,9 @@ "note": "Phase-1 baseline execution." }, "cpp": { - "status": "not_audited", + "status": "observed_pass", "classification": "required", - "note": "Audit planned in phase 2." + "note": "read_FM() takes 'simtime = simtime > inval[0] ? simtime : inval[0]' (concore.hpp), i.e. a plain max(), so an older incoming timestamp never decreases simtime. Matches this fixture. Verified by reading concore.hpp directly; no existing TestConcoreHpp.cpp test exercises an older-than-current timestamp specifically (test_read_FM_file only covers the initial 0.0 -> 3.0 case)." }, "java": { "status": "observed_pass", diff --git a/tests/test_protocol_conformance_phase2.py b/tests/test_protocol_conformance_phase2.py index 787e918..4c13f52 100644 --- a/tests/test_protocol_conformance_phase2.py +++ b/tests/test_protocol_conformance_phase2.py @@ -66,3 +66,30 @@ def test_phase2_matrix_java_status_is_recorded_for_each_case(): assert java_result["classification"] in EXPECTED_CLASSIFICATIONS assert isinstance(java_result["note"], str) and java_result["note"].strip() assert java_result["status"] == "observed_pass" + + +# cpp is audited (no longer not_audited) for every case; three cases are a +# genuine observed_fail -- the two parse_params cases (load_params() mangles +# comma- and equals-containing values) and the mixed-type initval case +# (flatten_numeric silently drops non-numeric elements) -- everything else +# is observed_pass. This pins the audit result so a future edit can't +# silently regress it back to not_audited or paper over the known bugs. +CPP_KNOWN_FAILING_CASES = { + "parse_params/simple_types_and_whitespace", + "parse_params/embedded_equals_not_split", + "initval/valid_list_sets_simtime", +} + + +def test_phase2_matrix_cpp_status_is_recorded_for_each_case(): + for row in _phase2_matrix()["cases"]: + cpp_result = row["runtime_results"]["cpp"] + assert cpp_result["status"] in EXPECTED_STATUSES + assert cpp_result["classification"] in EXPECTED_CLASSIFICATIONS + assert isinstance(cpp_result["note"], str) and cpp_result["note"].strip() + assert cpp_result["status"] != "not_audited" + + if row["id"] in CPP_KNOWN_FAILING_CASES: + assert cpp_result["status"] == "observed_fail" + else: + assert cpp_result["status"] == "observed_pass"