|
| 1 | +# FIX-CICD: CI fixture tests failing on Ubuntu (jq missing) |
| 2 | + |
| 3 | +**Created:** 2026-01-09 |
| 4 | +**Status:** Not Started |
| 5 | +**Priority:** High |
| 6 | + |
| 7 | +## Problem/Request |
| 8 | +GitHub Actions CI was failing (9/10 fixture tests failing on Ubuntu) while passing locally on macOS. |
| 9 | + |
| 10 | +## Root Cause (confirmed) |
| 11 | +The fixture test runner [dist/tests/run-fixture-tests.sh](../../dist/tests/run-fixture-tests.sh) parses scanner output as JSON using `jq`. |
| 12 | + |
| 13 | +- In GitHub Actions Ubuntu runners, `jq` is not guaranteed to be present. |
| 14 | +- When `jq` is missing, the script’s JSON-parse branch fails and it falls back to *text* parsing. |
| 15 | +- Because [dist/bin/check-performance.sh](../../dist/bin/check-performance.sh) defaults to JSON output (`OUTPUT_FORMAT="json"`), the text parsing fallback fails too. |
| 16 | + |
| 17 | +## Code Review Findings |
| 18 | + |
| 19 | +### ✅ What’s good |
| 20 | +- **Correct fix direction:** Installing `jq` in CI aligns with a JSON-first architecture and also supports Slack/report tooling in [ .github/workflows/ci.yml](../../.github/workflows/ci.yml). |
| 21 | +- **Avoids weakening tests:** Not forcing `--format text` keeps parsing stable and avoids brittle greps for human output. |
| 22 | +- **Script already has some resilience:** The fixture runner strips ANSI codes and captures output to temp files, which helps keep parsing deterministic. |
| 23 | + |
| 24 | +### ⚠️ Correctness / Robustness gaps |
| 25 | +1. **`jq` absence triggers the wrong fallback path** |
| 26 | + - In [dist/tests/run-fixture-tests.sh](../../dist/tests/run-fixture-tests.sh), the decision boundary is “can I run `jq empty`?” rather than “is the output JSON?”. |
| 27 | + - Result: if output *is* JSON but `jq` is missing, the script attempts text parsing, which is structurally incapable of working. |
| 28 | + |
| 29 | +2. **Implicit reliance on default output format** |
| 30 | + - `run_test()` calls `check-performance.sh` without `--format json`, relying on its default. |
| 31 | + - That’s currently stable (default is documented as JSON), but making it explicit would strengthen the contract between the test runner and the scanner. |
| 32 | + |
| 33 | +3. **CHANGELOG inconsistency / mixed narrative** |
| 34 | + - In [CHANGELOG.md](../../CHANGELOG.md) under **Unreleased → Fixed → Test Suite**, it claims: |
| 35 | + - “Fixed JSON parsing in test script to use grep-based parsing (no jq dependency)” |
| 36 | + - But the current script is `jq`-primary and CI explicitly installs `jq`. |
| 37 | + - The entry also says both “All 10 fixture tests now pass” and later “(9/10 tests passing)”, which reads as contradictory. |
| 38 | + |
| 39 | +4. **Duplication in CI dependency installation** |
| 40 | + - [ .github/workflows/ci.yml](../../.github/workflows/ci.yml) installs `jq` in both jobs separately. |
| 41 | + - This is fine, but it’s repeated maintenance surface. |
| 42 | + |
| 43 | +## Recommendations (no code changes requested) |
| 44 | + |
| 45 | +### 1) Make jq a declared prerequisite *or* make JSON parsing dependency-free |
| 46 | +Pick one and make it consistent across CI + docs: |
| 47 | + |
| 48 | +- **Option A (declare jq required):** |
| 49 | + - Treat `jq` as a hard dependency of the fixture runner. |
| 50 | + - In CI, keep installing it. |
| 51 | + - In local/dev, add a clear early check like `command -v jq` and fail with an actionable error message. |
| 52 | + |
| 53 | +- **Option B (remove jq dependency):** |
| 54 | + - Replace the `jq` parsing path in `run_test()` with a dependency-free JSON extraction (e.g., minimal grep extraction, or `python3 -c` JSON parsing). |
| 55 | + - This matches the existing “no jq dependency” statements in the changelog. |
| 56 | + |
| 57 | +### 2) Don’t use “text parsing” as a fallback for “jq missing” |
| 58 | +If you keep a fallback: |
| 59 | +- First detect whether output is JSON (e.g., begins with `{` after stripping ANSI). |
| 60 | +- If output is JSON but `jq` is missing, either: |
| 61 | + - fail with a clear message, or |
| 62 | + - use a dependency-free JSON parser fallback. |
| 63 | + |
| 64 | +### 3) Make format explicit in tests |
| 65 | +Even if the scanner default remains JSON: |
| 66 | +- Have the fixture tests call `check-performance.sh --format json` consistently. |
| 67 | +- This prevents future surprises if the scanner’s default changes. |
| 68 | + |
| 69 | +### 4) Clarify and reconcile CHANGELOG statements |
| 70 | +Update the Unreleased entry so it matches reality: |
| 71 | +- If CI installs `jq` and tests rely on it, remove/adjust the “no jq dependency” claim. |
| 72 | +- Fix the “All 10 pass” vs “9/10 pass” inconsistency. |
| 73 | + |
| 74 | +### 5) CI hardening (optional) |
| 75 | +- Print `jq --version` after install for easier diagnosis. |
| 76 | +- Consider using `sudo apt-get install -y jq` (with update) as you already do; it’s fine. |
| 77 | +- If apt install is a concern, failing the job is acceptable because tests can’t run correctly without `jq` under the current design. |
| 78 | + |
| 79 | +## Edge Cases / Risks to watch |
| 80 | +- **Runner image changes:** `ubuntu-latest` can change; explicit installation avoids surprises. |
| 81 | +- **JSON schema changes:** Tests assume `.summary.total_errors` and `.summary.total_warnings` exist. |
| 82 | + - If the JSON schema changes, the tests should fail loudly (ideally with a clear schema mismatch message). |
| 83 | +- **Non-JSON noise:** Any stderr logging mixed into JSON output will break parsing. |
| 84 | + - Scanner already has safeguards to avoid corrupting JSON; ensure future debug logging stays format-aware. |
| 85 | + |
| 86 | +## Acceptance Criteria |
| 87 | +- [ ] CI passes fixture validation on `ubuntu-latest` reliably. |
| 88 | +- [ ] Fixture tests either (A) explicitly require `jq` with a clear error, or (B) remain dependency-free. |
| 89 | +- [ ] CHANGELOG entry accurately describes the final architecture and outcome (10/10 passing). |
0 commit comments