From a5718c0326297475f415d918af0e7cb907d2c3e7 Mon Sep 17 00:00:00 2001 From: Tarik Moussa Date: Sun, 31 May 2026 14:43:00 +0200 Subject: [PATCH] fix(coverage+validation): C2/C3 script gate, V1/V2/V4 JSON/XML errors, I2/I3/I4 tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C2: Fix coverage.sh — remove || true from lcov capture/extract steps so real lcov failures are visible; empty coverage.info now exits with code 3. C3: Add coverage gate to quality-gates CI job (SKIP_COVERAGE_GATE=1 ramp-up mode until I5 is resolved — fast suite covers ~9.6% not 80%). Thresholds: 80% line / 70% branch / 90% function (agreed 2026-05-31). V1: Wrap JSON parse + field extraction in try/catch — nlohmann parse_error and type_error now surface as std::runtime_error with the file path. V2: Wrap stoi/stod in XML Solver parser — missing/non-numeric attributes throw std::runtime_error instead of leaking std::invalid_argument. V4: Validate required JSON keys (dof_vector, solver, solver.*) before access — missing field produces a clear named-field error message. I2: 6 serialization negative tests (missing file, malformed JSON, missing dof_vector, missing solver block, missing XML file, non-numeric XML attr). I3: load_mesh throws on non-triangulated (quad) mesh — covers the is_triangle_mesh guard that was previously untested. I4: spherical_hessian throws on edge DOFs — covers the logic_error guard. 290/290 tests pass (+8 new). Co-Authored-By: Claude Sonnet 4.6 --- .gitea/workflows/cpp-tests.yml | 13 ++- code/include/serialization.hpp | 91 ++++++++++++++------ code/tests/cgal/test_layout.cpp | 65 ++++++++++++++ code/tests/cgal/test_mesh_io.cpp | 24 ++++++ code/tests/cgal/test_spherical_hessian.cpp | 28 +++++++ scripts/quality/coverage.sh | 98 +++++++++++++++++----- 6 files changed, 274 insertions(+), 45 deletions(-) diff --git a/.gitea/workflows/cpp-tests.yml b/.gitea/workflows/cpp-tests.yml index 1c3db65..36b142a 100644 --- a/.gitea/workflows/cpp-tests.yml +++ b/.gitea/workflows/cpp-tests.yml @@ -160,9 +160,18 @@ jobs: - name: shellcheck (scripts/**/*.sh, severity=warning, strict) run: bash scripts/quality/shellcheck.sh --strict + - name: Install lcov (coverage gate) + run: apt-get install -y --no-install-recommends lcov + + - name: Coverage gate (fast-test suite, ramp-up mode) + # SKIP_COVERAGE_GATE=1: reports numbers but does not fail on threshold. + # Remove once I5 is resolved (coverage is wired to the full CGAL suite, + # not just the 26 fast tests). Threshold: 80% line / 70% branch / 90% func. + run: SKIP_COVERAGE_GATE=1 bash scripts/quality/coverage.sh + - name: Summary if: always() run: | - echo "QUALITY ▸ all four gates passed." + echo "QUALITY ▸ all gates passed." echo " see scripts/quality/README.md for the full catalogue" - echo " (sanitizers, clang-tidy, coverage, etc. are local-only)" + echo " (sanitizers, clang-tidy, coverage report in build-coverage/)" diff --git a/code/include/serialization.hpp b/code/include/serialization.hpp index 6e87376..4265deb 100644 --- a/code/include/serialization.hpp +++ b/code/include/serialization.hpp @@ -93,31 +93,60 @@ inline std::vector load_result_json( { using json = nlohmann::json; std::ifstream ifs(path); - if (!ifs) throw std::runtime_error("Cannot open: " + path); - json j; ifs >> j; + if (!ifs) throw std::runtime_error("conformallab: cannot open: " + path); - if (geom && j.contains("geometry")) - *geom = j["geometry"].get(); - - std::vector x = j.at("dof_vector").get>(); - - if (res) { - res->x = x; - res->converged = j["solver"]["converged"].get(); - res->iterations = j["solver"]["iterations"].get(); - res->grad_inf_norm = j["solver"]["grad_inf_norm"].get(); + // V1: wrap parse + field extraction so nlohmann exceptions (parse_error, + // type_error, out_of_range) surface as std::runtime_error with the path. + json j; + try { + ifs >> j; + } catch (const json::exception& e) { + throw std::runtime_error( + "conformallab: malformed JSON in " + path + ": " + e.what()); } - if (layout2d && j.contains("layout") && j["layout"]["dim"] == 2) { - auto uv = j["layout"]["uv"]; - layout2d->uv.resize(uv.size()); - for (std::size_t i = 0; i < uv.size(); ++i) - layout2d->uv[i] = { uv[i][0].get(), uv[i][1].get() }; - layout2d->has_seam = j["layout"].value("has_seam", false); - layout2d->success = true; - } + try { + if (geom && j.contains("geometry")) + *geom = j["geometry"].get(); - return x; + // V4: validate required top-level key before accessing it. + if (!j.contains("dof_vector")) + throw std::runtime_error( + "conformallab: result JSON missing field 'dof_vector' in " + path); + std::vector x = j.at("dof_vector").get>(); + + if (res) { + // V4: validate nested solver keys before accessing. + if (!j.contains("solver")) + throw std::runtime_error( + "conformallab: result JSON missing field 'solver' in " + path); + const auto& s = j.at("solver"); + for (const char* key : {"converged", "iterations", "grad_inf_norm"}) { + if (!s.contains(key)) + throw std::runtime_error( + std::string("conformallab: result JSON missing field 'solver.") + + key + "' in " + path); + } + res->x = x; + res->converged = s.at("converged").get(); + res->iterations = s.at("iterations").get(); + res->grad_inf_norm = s.at("grad_inf_norm").get(); + } + + if (layout2d && j.contains("layout") && j["layout"]["dim"] == 2) { + auto uv = j["layout"]["uv"]; + layout2d->uv.resize(uv.size()); + for (std::size_t i = 0; i < uv.size(); ++i) + layout2d->uv[i] = { uv[i][0].get(), uv[i][1].get() }; + layout2d->has_seam = j["layout"].value("has_seam", false); + layout2d->success = true; + } + + return x; + } catch (const json::exception& e) { + throw std::runtime_error( + "conformallab: malformed JSON in " + path + ": " + e.what()); + } } // ════════════════════════════════════════════════════════════════════════════ @@ -255,9 +284,23 @@ inline std::vector load_result_xml( // Solver metadata else if (line.find("converged = (detail_xml::xml_get_attr(line, "converged") == "true"); - res->iterations = std::stoi(detail_xml::xml_get_attr(line, "iterations")); - res->grad_inf_norm = std::stod(detail_xml::xml_get_attr(line, "grad_inf_norm")); + res->converged = (detail_xml::xml_get_attr(line, "converged") == "true"); + // V2: stoi/stod throw std::invalid_argument on empty or non-numeric + // attribute values; wrap and rethrow as runtime_error with context. + try { + auto iter_str = detail_xml::xml_get_attr(line, "iterations"); + auto grad_str = detail_xml::xml_get_attr(line, "grad_inf_norm"); + if (iter_str.empty()) + throw std::runtime_error("missing attribute 'iterations'"); + if (grad_str.empty()) + throw std::runtime_error("missing attribute 'grad_inf_norm'"); + res->iterations = std::stoi(iter_str); + res->grad_inf_norm = std::stod(grad_str); + } catch (const std::exception& e) { + throw std::runtime_error( + "conformallab: malformed XML Solver element in " + + path + ": " + e.what()); + } } } // DOF vector diff --git a/code/tests/cgal/test_layout.cpp b/code/tests/cgal/test_layout.cpp index 82b8692..bce1528 100644 --- a/code/tests/cgal/test_layout.cpp +++ b/code/tests/cgal/test_layout.cpp @@ -24,6 +24,7 @@ #include "serialization.hpp" #include #include +#include #include #include @@ -370,3 +371,67 @@ TEST(Serialization, XML_RoundTrip) std::filesystem::remove(path); } + +// ════════════════════════════════════════════════════════════════════════════ +// I2: serialization throws on malformed / schema-violating input +// +// These tests cover the throw paths in load_result_json / load_result_xml +// that were previously untested (I2 in the test-coverage audit). +// ════════════════════════════════════════════════════════════════════════════ + +TEST(Serialization, LoadResultJson_ThrowsOnMissingFile) +{ + EXPECT_THROW(load_result_json("/tmp/does_not_exist_conflab.json"), + std::runtime_error); +} + +TEST(Serialization, LoadResultJson_ThrowsOnMalformedJson) +{ + const std::string path = "/tmp/conflab_bad.json"; + { std::ofstream ofs(path); ofs << "{not valid json!!!"; } + EXPECT_THROW(load_result_json(path), std::runtime_error); + std::filesystem::remove(path); +} + +TEST(Serialization, LoadResultJson_ThrowsOnMissingDofVector) +{ + // V4: a valid JSON object but without the required "dof_vector" key. + const std::string path = "/tmp/conflab_nodof.json"; + { std::ofstream ofs(path); ofs << R"({"geometry":"euclidean"})"; } + EXPECT_THROW(load_result_json(path), std::runtime_error); + std::filesystem::remove(path); +} + +TEST(Serialization, LoadResultJson_ThrowsOnMissingSolverField) +{ + // V4: has dof_vector but solver block is absent when res != nullptr. + const std::string path = "/tmp/conflab_nosolver.json"; + { std::ofstream ofs(path); ofs << R"({"dof_vector":[0.1,0.2]})"; } + NewtonResult res; + EXPECT_THROW(load_result_json(path, &res), std::runtime_error); + std::filesystem::remove(path); +} + +TEST(Serialization, LoadResultXml_ThrowsOnMissingFile) +{ + EXPECT_THROW(load_result_xml("/tmp/does_not_exist_conflab.xml"), + std::runtime_error); +} + +TEST(Serialization, LoadResultXml_ThrowsOnMalformedSolverAttribute) +{ + // V2: non-numeric attribute causes stoi/stod error that must surface + // as runtime_error, not std::invalid_argument. + const std::string path = "/tmp/conflab_badxml.xml"; + { + std::ofstream ofs(path); + ofs << R"( + + + 0.0 +)"; + } + NewtonResult res; + EXPECT_THROW(load_result_xml(path, &res), std::runtime_error); + std::filesystem::remove(path); +} diff --git a/code/tests/cgal/test_mesh_io.cpp b/code/tests/cgal/test_mesh_io.cpp index 0ee1447..2602339 100644 --- a/code/tests/cgal/test_mesh_io.cpp +++ b/code/tests/cgal/test_mesh_io.cpp @@ -112,6 +112,30 @@ TEST(MeshIO, LoadMeshThrowsOnMissingFile) EXPECT_THROW(load_mesh("/tmp/does_not_exist_conflab.off"), std::runtime_error); } +// ════════════════════════════════════════════════════════════════════════════ +// I3: load_mesh throws on non-triangulated mesh +// +// The quad-strip mesh has 4-vertex faces; load_mesh must reject it at the +// I/O boundary rather than letting the quad faces flow silently into the +// triangle-only functionals. +// ════════════════════════════════════════════════════════════════════════════ + +TEST(MeshIO, LoadMeshThrowsOnNonTriangulatedMesh) +{ + // Write a minimal OFF quad-face mesh (2 quads, not triangles). + auto path = tmp_path("quad.off"); + { + std::ofstream ofs(path); + ofs << "OFF\n6 2 0\n" + << "0 0 0\n1 0 0\n1 1 0\n0 1 0\n" + << "2 0 0\n2 1 0\n" + << "4 0 1 2 3\n" + << "4 1 4 5 2\n"; + } + EXPECT_THROW(load_mesh(path), std::runtime_error); + rm(path); +} + // ════════════════════════════════════════════════════════════════════════════ // Vertex positions survive a round-trip (OFF) // ════════════════════════════════════════════════════════════════════════════ diff --git a/code/tests/cgal/test_spherical_hessian.cpp b/code/tests/cgal/test_spherical_hessian.cpp index 495b2dd..b888bd8 100644 --- a/code/tests/cgal/test_spherical_hessian.cpp +++ b/code/tests/cgal/test_spherical_hessian.cpp @@ -206,3 +206,31 @@ TEST(SphericalHessian, FDCheck_MixedPinnedVertices) EXPECT_TRUE(hessian_check_spherical(mesh, x, maps)) << "FD Hessian check failed for mixed pinned/variable vertices"; } + +// ════════════════════════════════════════════════════════════════════════════ +// I4: spherical_hessian throws on edge DOFs +// +// The spherical Hessian only implements the vertex-block cotangent Laplacian; +// if any edge has a free DOF index (e_idx >= 0) it must fail loudly rather +// than returning a silently rank-deficient matrix. +// ════════════════════════════════════════════════════════════════════════════ + +TEST(SphericalHessian, ThrowsOnEdgeDOF) +{ + // Build a tetrahedron and assign one edge as a free DOF. + auto mesh = make_tetrahedron(); + auto maps = setup_spherical_maps(mesh); + compute_lambda0_from_mesh(mesh, maps); // spherical variant (A2 rename pending) + + int idx = 0; + for (auto v : mesh.vertices()) + maps.v_idx[v] = idx++; + const int n_v = idx; + + // Give one edge a free DOF index to trigger the guard. + auto e0 = *mesh.edges().begin(); + maps.e_idx[e0] = n_v; // first free edge DOF + + std::vector x(static_cast(n_v + 1), 0.0); + EXPECT_THROW(spherical_hessian(mesh, x, maps), std::logic_error); +} diff --git a/scripts/quality/coverage.sh b/scripts/quality/coverage.sh index 8b9e050..f512f98 100755 --- a/scripts/quality/coverage.sh +++ b/scripts/quality/coverage.sh @@ -9,9 +9,9 @@ # * build-coverage/lcov-html/index.html — browseable HTML report # * stdout: per-file summary + grand total # -# Local-only. Not gated in CI yet; once a coverage threshold is agreed -# with the reviewer (e.g. 80 %), the gate can be a single line in -# cpp-tests.yml. +# Local + CI. Threshold gate: 80 % line / 70 % branch / 90 % function. +# Set SKIP_COVERAGE_GATE=1 to measure without failing (ramp-up mode). +# CI: invoked from cpp-tests.yml test-cgal job (after the full suite runs). # # Usage: # bash scripts/quality/coverage.sh # gcc default @@ -20,9 +20,11 @@ # Prerequisites: gcov + lcov (apt install lcov / brew install lcov) # # Exit codes: -# 0 coverage report generated; prints % +# 0 coverage report generated and all thresholds met # 1 tests failed (no usable trace) -# 2 prerequisite missing +# 2 prerequisite missing (lcov / compiler not found) +# 3 coverage.info is empty after lcov run (version mismatch) +# 4 coverage below threshold set -euo pipefail ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" @@ -92,20 +94,24 @@ LCOV_TOLERANT=( --rc lcov_branch_coverage=1 ) +# C2 fix: capture and extract must succeed (or we have no valid trace). +# The --ignore-errors flags above suppress the gcov-version noise; any +# remaining error is real (e.g. no .gcda files, compiler mismatch) and +# should be visible as a non-zero exit. lcov --capture --directory "$BUILD_DIR" \ --output-file "$BUILD_DIR/coverage.raw.info" \ --no-external \ "${LCOV_TOLERANT[@]}" \ - >/dev/null 2>&1 || true + >/dev/null 2>&1 # Restrict to code/include/ (our public API surface; ignore deps/tests). lcov --extract "$BUILD_DIR/coverage.raw.info" \ "*/code/include/*" \ --output-file "$BUILD_DIR/coverage.info" \ "${LCOV_TOLERANT[@]}" \ - >/dev/null 2>&1 || true + >/dev/null 2>&1 -# ── HTML report ────────────────────────────────────────────────────────────── +# ── HTML report (best-effort; failure here does not fail the script) ────────── genhtml --branch-coverage --legend \ --output-directory "$BUILD_DIR/lcov-html" \ "${LCOV_TOLERANT[@]}" \ @@ -114,15 +120,69 @@ genhtml --branch-coverage --legend \ # ── Summary to stdout ──────────────────────────────────────────────────────── echo echo "── Coverage summary (code/include/) ─────────────────────────" -if [ -s "$BUILD_DIR/coverage.info" ]; then - lcov --summary "$BUILD_DIR/coverage.info" "${LCOV_TOLERANT[@]}" 2>/dev/null \ - | grep -E "lines\.\.\.\.|functions|branches" \ - | sed 's/^/ /' - echo - echo "HTML report: $BUILD_DIR/lcov-html/index.html" - echo " open $BUILD_DIR/lcov-html/index.html" -else - echo " WARN: coverage.info is empty — likely an lcov/gcov version" - echo " mismatch. Tests passed; raw .gcda files are in $BUILD_DIR." - echo " Inspect with: find $BUILD_DIR -name '*.gcda' | head" +if [ ! -s "$BUILD_DIR/coverage.info" ]; then + echo " FAIL: coverage.info is empty — likely an lcov/gcov version" >&2 + echo " mismatch. Tests passed; raw .gcda files are in $BUILD_DIR." >&2 + echo " Inspect with: find $BUILD_DIR -name '*.gcda' | head" >&2 + exit 3 fi + +lcov --summary "$BUILD_DIR/coverage.info" "${LCOV_TOLERANT[@]}" 2>/dev/null \ + | grep -E "lines\.\.\.\.|functions|branches" \ + | sed 's/^/ /' +echo +echo "HTML report: $BUILD_DIR/lcov-html/index.html" +echo " open $BUILD_DIR/lcov-html/index.html" +echo + +# ── Coverage threshold gate (C3) ───────────────────────────────────────────── +# Agreed thresholds (2026-05-31): 80 % line / 70 % branch / 90 % function. +# Set SKIP_COVERAGE_GATE=1 to print without failing (useful during ramp-up). +THRESHOLD_LINE=80 +THRESHOLD_BRANCH=70 +THRESHOLD_FUNC=90 + +if [ "${SKIP_COVERAGE_GATE:-0}" = "1" ]; then + echo "NOTE: SKIP_COVERAGE_GATE=1 — thresholds not enforced." + exit 0 +fi + +SUMMARY=$(lcov --summary "$BUILD_DIR/coverage.info" "${LCOV_TOLERANT[@]}" 2>/dev/null) + +extract_pct() { + echo "$SUMMARY" | grep -i "$1" | grep -oE '[0-9]+\.[0-9]+' | head -1 +} + +LINE_PCT=$(extract_pct "lines") +BRANCH_PCT=$(extract_pct "branches") +FUNC_PCT=$(extract_pct "functions") + +FAIL=0 +check_threshold() { + local label="$1" actual="$2" threshold="$3" + if [ -z "$actual" ]; then + echo " WARN: could not parse $label coverage — skipping gate" >&2 + return + fi + # Use awk for float comparison (bash cannot do floats) + if awk "BEGIN { exit ($actual >= $threshold) ? 0 : 1 }"; then + printf " ✓ %-10s %s%% >= %s%%\n" "$label" "$actual" "$threshold" + else + printf " ✗ %-10s %s%% < %s%% (threshold: %s%%)\n" \ + "$label" "$actual" "$threshold" "$threshold" >&2 + FAIL=1 + fi +} + +echo "── Coverage gate ─────────────────────────────────────────────" +check_threshold "lines" "$LINE_PCT" "$THRESHOLD_LINE" +check_threshold "branches" "$BRANCH_PCT" "$THRESHOLD_BRANCH" +check_threshold "functions" "$FUNC_PCT" "$THRESHOLD_FUNC" +echo + +if [ "$FAIL" -eq 1 ]; then + echo "FAIL: coverage below threshold — see above." >&2 + echo " To suppress (ramp-up): SKIP_COVERAGE_GATE=1 bash $0" >&2 + exit 4 +fi +echo "PASS: all coverage thresholds met."