test: audit cpp against the protocol conformance matrix, wire its tests into CI - #585
Open
bhuvan-somisetty wants to merge 1 commit into
Open
bhuvan-somisetty wants to merge 1 commit into
bhuvan-somisetty wants to merge 1 commit into
Conversation
…follow-up) Extends the cross-runtime matrix from ControlCore-Project#440/ControlCore-Project#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 (ControlCore-Project#389, ControlCore-Project#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 ControlCore-Project#440 ControlCore-Project#574
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi @pradeeban, this follows on from #467 (Phase 2 of #440). cpp was still not_audited for all 8 cases in cross_runtime_matrix.phase2.json, same as matlab/octave/verilog, so I went through concore.hpp and concore_base.hpp by hand against the phase-1 fixtures to see where it actually stands.
5 of the 8 line up fine with Python, empty-input initval, both write_zmq cases, both read_file cases. The other 3 turned up actual bugs though, not just gaps.
Both parse_params cases fail. load_params() converts semicolon-style params into a dict literal by globally swapping every comma for ," and every = for ":. Works fine for something plain like a=1;b=2, but the second a value has its own comma or equals sign, an array literal like coeffs=[1,2,3], or a URL with query params, the substitution mangles it into invalid syntax, parsing throws, and load_params() just quietly returns an empty map. No error, nothing logged, params just vanish. Traced it by hand running the exact regex steps against the fixture inputs, the existing test_load_params_semicolon_format doesn't catch it since it only uses comma-free values.
The mixed-type initval case fails too. initval() flattens through flatten_numeric(), and the STRING branch there is literally just break, so any non-numeric element gets silently dropped instead of kept. For [12.5, "a", 3] simtime comes out right but the returned vector is [3.0] instead of ["a", 3], the string's just gone and everything after it shifts over by one. Underlying cause is initval()'s return type is vector, so it can't hold a string anyway, the parser itself (parse_literal) handles strings fine, it's just this one call site that loses them.
Logged both as observed_fail in the matrix with the reasoning inline, kept the classification as required since that's what was already there, happy to change it if you'd rather call these implementation_defined. This is report-only, nothing in the runtime changed.
Also, kind of related, TestLiteralEvalCpp.cpp (#389) and TestConcoreHpp.cpp (#484) already exist and are built to be CI-friendly, plain pass/fail with real exit codes, but the cpp job in ci.yml only runs g++ -fsyntax-only against a different file. Neither of these has ever actually run in CI. Wired both in. Fixes #584.
Happy to open separate issues for the two bugs, or just send the fixes directly since they're both pretty contained, whichever you'd prefer.