Repository navigation
Contract test suite (4/5): hardening and CI follow-ups - #98
Open
olivhoenen wants to merge 17 commits into
Open
olivhoenen wants to merge 17 commits into
olivhoenen wants to merge 17 commits into
Conversation
In a plain YAML scalar, ' #' starts a YAML comment even inside shell-looking quotes, so both backend Note steps shipped as an unterminated 'echo "...' and failed with 'unexpected EOF while looking for matching quote', turning the jobs red even when their test steps passed. Switch both Note steps to block scalars so the full command (including the issue references) reaches the shell. Add an actionlint workflow gated on .github/workflows/** changes; actionlint's shellcheck pass catches this truncation class before it lands. Closes #43 (cherry picked from commit b19bd27)
contract_tests opens pulses against the baked model tree, but only depended on al; the mdsplus-contract workflow builds just --target contract_tests, so al-mdsplus-model (an independent ALL custom target) was never built and the whole MDSplus tier failed with %TREE-E-FOPENR while opening the pulse/model tree. Make the dependency part of the build graph: when AL_BACKEND_MDSPLUS and AL_BUILD_MDSPLUS_MODELS are both on, contract_tests depends on al-mdsplus-model (declared at the top level, after both targets exist, since tests/ is added before models/mdsplus/). Also verify the three ids_model.* files exist in the workflow before CTest, covering both the cache-hit and cache-miss paths. Closes #42 (cherry picked from commit 54db010)
docker/uda/run.sh retried the TCP listener but never checked whether the loop succeeded, printed 'server listening' unconditionally, and downgraded a failed uda_cli ping to a warning. Combined with the contract suite's server-unreachable GTEST_SKIP(), a total bootstrap failure could skip every UDA case while CTest exits 0 — a green job that tested nothing. - Track listener success and exit nonzero after the timeout. - Make the independent uda_cli help::help() ping fatal. - Add a dedicated-CI mode (AL_UDA_DEDICATED_CI=1, set by uda-contract.yml): run CTest with --output-junit and fail unless a nonzero number of UDA tests actually executed (guards both zero selected and all skipped). - Developer builds keep the compile-gate/runtime-skip behavior: without the flag nothing changes. Closes #41 (cherry picked from commit 297a946)
The multi-pass TEST_FILTER partition was broken at both ends of the supported CMake range: gtest_discover_tests' PROPERTIES forwarding collapses a multi-element LABELS list to its first token on CMake 3.28 (so ctest -L unit selected nothing), TEST_FILTER does not exist on the declared 3.21 floor (which fell back to a single all-'contract' pass), and even where multi-label forwarding worked (4.3.2) the unit and integration selections overlapped by two tests (ReadbackPlugins.BindReadbackPluginsAutoRegistersAndBindsFromStoredMetadata, UriOwnership.CallerFreesBuiltUri). Replace the four discovery passes with ONE pass (no TEST_FILTER, single-element LABELS — both fine on 3.21) plus post-discovery label assignment: assign_contract_labels.cmake, appended to the directory TEST_INCLUDE_FILES property, runs after the discovery include has registered every test, parses the generated registration file (handling the version-dependent add_test name quoting: bracket, quoted, bare) and classifies each test into exactly one of unit / integration / mdsplus / uda with a plain set_tests_properties list. The partition is disjoint by construction. Add tests/contract/check_label_partition.sh — fails on zero-selection, overlap, or a non-hermetic unit tier — and run it in the blocking functional workflow before the test run, since an empty 'ctest -L unit' exits 0 and is otherwise invisible. Verified on CMake 3.21.4 (Linux container), 3.28.4, and 4.3.2 (macOS): identical selections everywhere — 94 unit, 173 integration, 267 total; unit and integration disjoint; the two previously double-labeled tests now carry only 'unit'; unit tier and full suite pass (87/87, 247/247 executed). Closes #31 (cherry picked from commit c6c4d38)
TRACEABILITY Part 2 marked initBackend's unrecognized-ID throw path covered only 'by inspection'. Add test_backend_factory.cpp (BackendFactory.*, hermetic unit tier) pinning the refusal through the public surface: - an unknown backend name in an otherwise valid data-entry URI returns UNKNOWN_ERR from al_begin_dataentry_action across all four open modes, with the stable 'Unable to identify a backend' diagnostic, no context handed out (sentinel untouched) and no filesystem artifact; - an empty backend name is likewise refused; - an unknown positive numeric ID via al_build_uri_from_legacy_parameters returns LOWLEVEL_ERR and synthesizes no URI. A regression that defaults an unrecognized identifier to a real backend makes these calls succeed and the tests fail. Verified clean under ASan/UBSan (macOS) and ASan+LSan (Linux container, detect_leaks=1). TRACEABILITY Part 2 row A now points at the active tests instead of the inspection-only residual. Closes #47 (cherry picked from commit 21c5570)
PluginTest.RegisterUnloadableSharedLibSwallowsAssert_CurrentBehavior called al_register_plugin in-process; in a Debug build the product assertion in registerPlugin fires on the failed dlopen and aborted the whole test runner (exit 134) instead of producing a GoogleTest verdict. Split the current-behavior pin by build mode (issue #32): - assert-enabled builds: a death test (RegisterUnloadableSharedLibAbortsOnAssert_CurrentBehavior) pins SIGABRT + the assertion text in a child process. The fixture directory is created by the child but removed by the PARENT; since threadsafe-style death tests re-execute the whole test in the child, the parent's path is handed down via an env var (overwrite=0 so the re-executed child keeps the inherited value) — verified no temp directory is left behind on macOS and Linux. - NDEBUG builds: the existing swallowed-assert tripwire is kept unchanged under #ifdef NDEBUG. - The DISABLED_ correct-contract test (nonzero status attributing the actual dlopen error) stays build-mode independent; fixing registerPlugin makes the active pin of that mode fail and the correct-contract test ready to enable. Run both build modes in CI: the blocking functional workflow now builds a {RelWithDebInfo, Debug} matrix, so each branch of the pin executes on every PR. Verified: full contract suite passes 251/251 in Debug and Release on macOS, and in Debug on Linux (container); the previously-crashing gtest_filter repro now returns [PASSED] in Debug. Closes #32 (cherry picked from commit 240330d)
The first dedicated-CI run proved the zero-executed guard works end to end — and that it caught its own parsing defect: CTest writes the <testsuite> attributes one per line, so the single-line sed extraction saw tests=0/skipped=0 and (correctly) failed the tier even though all 71 UDA tests had passed. Flatten the file and extract tests=/skipped= from the suite header only. Verified against a real 'ctest --output-junit' file (98 tests, 1 skipped -> 97 executed). Part of issue #41. (cherry picked from commit 8322cfe)
TRACEABILITY Part 2 row B declined to test the always-on HDF5 stored-version drift check because forcing a mismatch needs out-of-band HDF5 attribute surgery. Add that infrastructure per issue #36's producer/observer split: - al-hdf5-fixture-tool: a small standalone target (the only place the HDF5 C API appears) that rewrites master.h5's HDF5_BACKEND_VERSION attribute; contract_tests only learns its path. - Hdf5VersionDrift.* (integration tier, default HDF5-enabled suite, so it runs on every PR): matching stored version opens; stored 999.0 (major) and 1.999 (the minor comparison boundary) are refused with an exact status/message assertion; contract_tests stays public-C-ABI-only. Empirical characterization the pins record: for HDF5 the generic LOWLEVEL_ERR drift comparison in al_lowlevel.cpp is SHADOWED — HDF5BackendFactory accepts exactly "1.0" and openPulse throws BACKEND_ERR ('No backend writer with version: <stored>') for any other stored value before the generic check runs, so every non-compiled stored version is refused (stricter than the generic rule). Part 2 row B goes from 'gap' to 'covered' and documents the shadowing. Verified locally: 3/3 new tests pass; full suite 254/254; label partition intact (98 unit / 176 integration). Closes #36 (cherry picked from commit 5dbc566)
The old tripwire opened a fresh, matching HDF5 pulse — a future correct drift implementation would also accept it, so it discriminated nothing. Rework per issue #39: the pulse's stored HDF5_BACKEND_VERSION is rewritten to 999.0 out-of-band by hdf5_fixture_tool (issue #36's isolated producer); every asserted behavior stays on the public C ABI. Empirically characterized against the reference stack (which corrects the issue's assumption that the mismatched fixture opens today): the mismatched pulse is REFUSED, but by the SERVER-side HDF5 open — the server plugin's own IMAS-Core hits HDF5BackendFactory's 'No backend writer with version: 999.0' — forwarded to the client as UNKNOWN_ERR. The client-side check stays inert (placeholder {0,0} comparison); its own LOWLEVEL_ERR 'Compatibility …' refusal never appears. The pins encode that split: - baseline MatchingStoredVersionOpensThroughUda: matching fixture opens; - active tripwire VersionDriftCheckCurrentlyDefersToServerSideRefusal: exact UNKNOWN_ERR + server-forwarded message, and asserts the client-side 'Compatibility' diagnostic is absent — implementing real stored-version retrieval makes it fail; - DISABLED_OpenRefusesMismatchedStoredBackendVersion (correct contract): the client's own LOWLEVEL_ERR 'Compatibility' refusal. TRACEABILITY Part 5 documents the actual mismatched fixture and the exact observed status. Verified against the local aarch64 reference stack: full UDA tier 72/72 passed; dedicated-CI check reports 65 executed. Closes #39 (cherry picked from commit b26d7e0)
…step First real run of the issue #42 verify step failed on a healthy build: a baked MDSplus model tree carries all structure in ids_model.{tree,characteristics} and its ids_model.datafile is legitimately 0 bytes, so the -s (nonempty) check rejected it and the tier was skipped (masked by continue-on-error). Check tree and characteristics for nonzero size, datafile for existence only. Verified in the local aarch64 container: a clean '--build --target contract_tests' bakes all three files and 'ctest -L mdsplus' passes 44/44. Part of issue #42. (cherry picked from commit 26c8f9a)
test_mdsplus_version_drift.cpp included <mdsobjects.h> and mutated VERSION:BACK_MAJOR through the raw MDSplus C++ API, linking the MDSplus libraries into contract_tests — a second behavioral seam: a rewrite could satisfy the C ABI while the test still required the legacy API and storage layout. Separate fixture mutation from contract observation (issue #35): - al-mdsplus-fixture-tool (build-gated with the backend) owns ALL direct MDSplus API access: it reproduces the ids_path/setDataEnv dance and overwrites VERSION:BACK_MAJOR. Fixtures are regenerated from the pinned DD/model inputs at test run time; nothing opaque or host-specific is committed. - test_mdsplus_version_drift.cpp now includes only AL/test headers and calls only al_* APIs; it invokes the tool as a subprocess for the mismatch. contract_tests has no MDSplus include or link left (verified: no MDSplus NEEDED entry in the binary; the tool links libMdsObjectsCppShr). - The single discovery pass no longer sets a 'contract' label: ctest MERGES labels across set_tests_properties calls, so a discovery-time label would leak onto the mdsplus/uda tiers and change what 'ctest -L contract' (sanitizers.yml) selects. The labeler assigns each test its complete final list; verified in the container: contract=285 (=unit 98 + integration 187), mdsplus=45, disjoint. Verified in the local aarch64 container (compile-gated tier, reproducible): 'ctest -L mdsplus' 44/44 passed, both drift fixtures (matching + mismatched via the tool) prove the check; always-on suite 254/254 locally. Closes #35 (cherry picked from commit c90b8e3)
The MDSplus (#15) and UDA (#25) real-path breadth suites each defined their own copy of the DD-4.1.1 datatype/rank path table, the terminal-gap DD facts, and the case-name generator. A path correction or a new (type, rank) cell needed coordinated edits in two large files, and the tables could silently diverge -- undermining #12/#21's promise that the two backend verdicts are comparable over the same curated set. Extract the single source of truth into test-only real_dd_path_catalog.{h,cpp}: the validated DD version, the datatype/rank key, the IDS + struct_array chain + leaf, and the terminal-gap reason (all DD facts, identical for both backends). Backend-specific verdicts stay local -- MDSplus's STR_0D dimension divergence and UDA's fixture-backend divergence + dynamic-leaf-inside-AOS known defects are attached per suite by (dtype, rank) key, never duplicated into the catalog. Both matrices now derive their parametrization 1:1 from catalog(), so a cell cannot be dropped or duplicated silently: - RealDdPathCatalog.* (always-on): the catalog has exactly one entry per (dtype, rank) cell, no duplicate, path/terminal-gap mutually exclusive. - {Mdsplus,Uda}RealPathCatalog.CoversEveryCatalogKeyOnce (per gated tier): the matrix key set equals the catalog key set exactly. Case names ('CHAR_r0' ...) and INSTANTIATE_TEST_SUITE_P prefixes are preserved, so existing test names and TRACEABILITY references stay stable. Verified: MDSplus tier 18 pass / 14 skip (unchanged) + meta green; UDA tier 15 pass / 17 skip + 3 known-defect tripwires green + meta green (one Protocol-11 server flake confirmed transient on re-run); always-on catalog tests 2/2; label partition disjoint and hermetic (unit 98->100, mdsplus 45->46, integration unchanged) across MDSplus-enabled and backends-off builds. (cherry picked from commit 8506879)
Use a static DD-4.1.1 AOS leaf with three index-bound values so repeated or reordered traversal fails through the public C ABI. The old dynamic-leaf sentinel tripwire observed identical values and could not distinguish a successful no-op iterator. (cherry picked from commit cd20a87)
(cherry picked from commit a39e680)
(cherry picked from commit 92f8466)
TRACEABILITY.md marked the whole MDSplus DeleteMatrix row a terminal-gap: MDSplus is absent from kDeleteBackends because the always-on fixture's synthetic paths (leaf_a, block, "") have no node in its DD-baked model tree, even though MDSplusBackend::deleteData genuinely implements path-scoped deletion. That is 'not yet done', not unreachable, so it failed #12's zero-unresolved-row bar. Close it with test_mdsplus_delete.cpp, the delete analogue of MdsplusRealPathMatrix (#15): seed real DD-4.1.1 equilibrium/code/* STR_0D leaves plus an unrelated vacuum_toroidal_field/r0 sibling through the public C ABI, al_delete_data at each granularity, reopen, and assert exactly which paths survive. (imas-dd MCP not in this session; paths validated against the baked-from IDSDef.xml, the method real_dd_path_catalog.h documents.) Observed verdicts: - LEAF (covered): al_delete_data("code/name") clears just that leaf; the two other code/* leaves and vacuum_toroidal_field/r0 keep their values. - STRUCTURE and ROOT (xfail): al_delete_data("code") and al_delete_data("") both throw %TREE-W-NNF because deleteData's STRUCTURE branch (mdsplus_backend.cpp:2147) looks up the :static/.timed_aos AOS-storage children, which a plain structure and the DATAOBJECT root have no node for; the delete is a no-op and every seeded path survives. Pinned per D2 with DISABLED_ correct-contract tests (confirmed red today) + active …CurrentlyThrowsNodeNotFound tripwires. Verified in the aarch64 MDSplus reference container: 3 active tests green, 2 DISABLED_ correct-contract tests fail as expected, all 5 land in the mdsplus tier, label partition still disjoint and hermetic. The Part 4 DeleteMatrix row is now covered (leaf) / xfail (structure, root), no longer terminal-gap. (cherry picked from commit cfa8896)
The shared equilibrium fixture modelled profiles_1d/psi as a flat top-level
leaf and constraints/{measured,weight} as a generic top-level AOS -- neither a
valid equilibrium DD-4.1.1 shape. The two DD-coupled backends could only skip
the seed (MDSplus pre-skip; UDA asserted the read failed then skipped), and the
hash covered values only, so a 3x4->4x3 extent transpose or a dropped/reordered
AOS element with unchanged bytes passed silently.
Rewrite the seed against real equilibrium DD-4.1.1 paths (validated against the
baked-from IDSDef.xml; imas-dd MCP not in this session): a static scalar
(vacuum_toroidal_field/r0), a top-level timebase array (time), and the real
time_slice struct_array whose elements carry time, profiles_1d/psi (FLT_1D),
global_quantities/ip, and a real constraints/ip/{measured,weight} subtree --
written with the real time_slice write idiom test_mdsplus_unique_surface.cpp
proves. Replace the values-only FNV hash with a structural hash over a canonical
stream (path identity + datatype + rank + every extent + AOS size/index +
values, lengths interleaved); the read side rebuilds the stream from the actual
observed shapes and the test asserts exact rank/extents field-by-field before
hashing.
Verdicts, all now covered (no skips):
- HDF5, Memory, ASCII: round-trip (verified locally).
- MDSplus: now executes and round-trips the whole composite (verified in the
aarch64 reference container) -- the former divergence/skip is gone.
- UDA: round-trips the whole composite through remote mode, including the
time_slice AOS dynamic leaves (verified stable across repeated runs of the
reference stack). This does NOT contradict UdaAosKnownDefects: that fixture
seeds the AOS with an empty timebase and no per-element time; a well-formed
DD-conformant time_slice (real timebasepath + per-element time) is the case
UDA resolves correctly.
Structural sensitivity is pinned hermetically by EquilibriumSeedStructuralHash.*
(unit tier): swapping two extents with identical bytes, dropping an AOS element,
and reordering two AOS elements each change the hash. No DD path/version is
compiled into product code (paths remain test data). TRACEABILITY Parts 1/4/5
updated: the seed no longer claims parity through skipped DD-invalid shapes.
(cherry picked from commit 23204bc)
olivhoenen
requested review from
SimonPinches and
prasad-sawantdesai
as code owners
October 14, 2026 06:39
This was referenced Oct 14, 2026
This branch has not been deployed
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
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.
Part of the split of #85 (Tests and Benchmarks for regression) into smaller, single-purpose PRs that are easier to review.
Follow-ups from reviewing the suite:
contract_testsand fails the UDA tier if its server does not start;Tests and CI only.
Fork history: issues yohannmarguier#31 – yohannmarguier#47, fork PR yohannmarguier#49. Commits 7ba5c54, 29e5136 and 7274728 from that PR are in #91 and #92 instead.
Stack position: 4/9; depends on #97.
Authorship: these commits are @yohannmarguier's, cherry-picked unchanged from his fork with
git cherry-pick -x, so each one keeps its original author, date and message plus a(cherry picked from commit …)link back to the fork. Only conflicts with currentdevelopand with the new commit order were resolved.🤖 Generated with Claude Code