fix(test): restore strict parity assertions and add CI parity job#916
fix(test): restore strict parity assertions and add CI parity job#916carlos-alm merged 3 commits intomainfrom
Conversation
Remove all transitional filters from build-parity.test.ts that were masking engine divergences (filterCall on ast_nodes, new_expression edge filter, Calculator role normalization). Restore strict toEqual assertions for nodes, edges, roles, and ast_nodes. Add a dedicated "Engine parity" CI job that verifies the native addon is loaded before running parity tests — the previous setup silently skipped the entire suite via describeOrSkip when native was unavailable.
|
Claude finished @carlos-alm's task —— View job PR Review: Engine Parity Test Restoration ✅Reviewing PR #916 for codegraph - a local code dependency graph CLI tool. Todo List
Phase 1: Root Cause Analysis ✅Problem Being Solved: The parity test was giving false confidence - it was either completely skipped (when native addon unavailable) or had multiple transitional filters that masked real engine divergences between the native Rust engine and WASM engine. Why This Approach:
Risk Assessment: 🟢 LOW RISK - This is strengthening tests, not weakening them. The approach follows the documented principle that engine divergences are bugs to be fixed, not acceptable differences. Phase 2: Configuration Change Detection ✅CI Workflow Analysis:
Assessment: The CI changes are justified and strengthen testing. This addresses a real gap where parity tests were being skipped in CI. Phase 3: Test Integrity Analysis ✅Removed Filters (Previously Masking Real Bugs):
Current Test Approach:
Assessment: This is the correct approach. The old filters were anti-patterns that documented bugs as acceptable instead of fixing them. This PR follows CLAUDE.md principle: "Never document bugs as expected behavior." Phase 4: Code Quality Review ✅Code Changes Analysis:
Technical Quality: High. The implementation is straightforward and follows existing patterns. Root Cause AnalysisProblem Being Solved: Parity tests were giving false confidence due to being silently skipped (native addon unavailable) or having transitional filters that masked real engine divergences. Backlog Compliance
Critical ConcernsNone. This PR follows best practices and removes anti-patterns. Final Recommendation
The PR author correctly states that tests will fail until a new native binary is published — this is the intended behavior and demonstrates the fix is working. |
Greptile SummaryThis PR removes three transitional workaround filters from
Confidence Score: 5/5Safe to merge — all three prior CI gate concerns are resolved and the core parity assertions are correctly restored to strict equality. All findings are P2. The transitional filter removal is clean and the CI job is well-structured. The it.skipIf inconsistency in ast-parity.test.ts is an edge case already guarded by the upstream verification step and does not affect normal CI operation. tests/engines/ast-parity.test.ts — the it.skipIf guards at individual test level create a false-pass edge case when requireParity=true but native is unavailable Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CI: parity job triggered] --> B[npm install with retry]
B --> C[Verify native addon step]
C -->|"pkg not found or load fails"| D[Job FAILS ✗]
C -->|"loaded OK"| E[Run parity tests CODEGRAPH_PARITY=1]
E --> F{describeOrSkip requireParity OR hasNative}
F -->|"true → describe"| G[Suite runs]
G --> H{build-parity / parity / dataflow-parity}
G --> I{ast-parity.test.ts}
H -->|native unavailable| J["native call throws → FAILS ✗ ✓"]
H -->|native available| K["toEqual assertions → pass or FAIL"]
I --> L{it.skipIf !isNativeAvailable}
L -->|native unavailable| M["All tests SKIP → passes silently ⚠"]
L -->|native available| N["Tests run → pass or FAIL"]
Reviews (3): Last reviewed commit: "fix(test): apply CODEGRAPH_PARITY guard ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ci-pipeline: | ||
| if: always() | ||
| needs: [lint, test, typecheck, audit, verify-imports, rust-check] | ||
| needs: [lint, test, typecheck, audit, verify-imports, rust-check, parity] |
There was a problem hiding this comment.
The PR description explicitly states: "The parity job will fail until a new native binary is published from current source." Adding this job to ci-pipeline's needs array means the required gate will fail until the binary ships, blocking every subsequent PR from showing green CI.
Either coordinate the binary publish to happen before this PR merges, or use continue-on-error: true on the parity job temporarily (and remove it once the binary is live):
parity:
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
continue-on-error: true # remove once updated binary is published
runs-on: ${{ matrix.os }}There was a problem hiding this comment.
The parity job is now passing on all three platforms (ubuntu, macos, windows) — the native binary was published before CI ran. The continue-on-error workaround is not needed since the gate is green.
That said, if this becomes an issue in the future (e.g. binary lags behind a source change), the coordinated approach is to publish the binary first. Adding continue-on-error would defeat the purpose of making parity a hard gate.
| const pkgs = { | ||
| 'linux-x64': '@optave/codegraph-linux-x64-gnu', | ||
| 'darwin-arm64': '@optave/codegraph-darwin-arm64', | ||
| 'darwin-x64': '@optave/codegraph-darwin-x64', | ||
| 'win32-x64': '@optave/codegraph-win32-x64-msvc', | ||
| }; | ||
| const pkg = pkgs[plat + '-' + arch]; | ||
| if (!pkg) { console.error('No native package for ' + plat + '-' + arch); process.exit(1); } | ||
| try { r(pkg); console.log('Native addon loaded: ' + pkg); } | ||
| catch (e) { console.error('Failed to load ' + pkg + ': ' + e.message); process.exit(1); } |
There was a problem hiding this comment.
isNativeAvailable()
The verification step hardcodes linux-x64 → @optave/codegraph-linux-x64-gnu, but native.ts's resolvePlatformPackage() calls detectLibc() and resolves to linux-x64-musl on a musl host. On such a runner the step would succeed (gnu package loads), then isNativeAvailable() would try the musl package (not installed), return false, and the tests would silently skip — giving false confidence.
The fix is to mirror the musl detection in the verification script, or simply invoke isNativeAvailable() directly via a small Node script that imports src/infrastructure/native.js:
// Verify by calling the same path the test will use
node --experimental-strip-types -e "
import { isNativeAvailable } from './src/infrastructure/native.js';
if (!isNativeAvailable()) { process.stderr.write('Native addon unavailable\n'); process.exit(1); }
console.log('Native addon verified via isNativeAvailable()');
"(Adjust the strip-types flag the same way verify-imports does.)
There was a problem hiding this comment.
Fixed in 2f9f450 — the verification script now mirrors the detectLibc() logic from native.ts: on Linux, it reads /lib for ld-musl-* entries and resolves to the correct -gnu or -musl package. Also added linux-arm64-gnu and linux-arm64-musl entries for completeness.
The key-construction now uses plat-arch-libc on Linux (matching resolvePlatformPackage()) and plat-arch on other platforms.
…silent parity skips (#916) - Mirror detectLibc() logic in CI verification script so musl hosts resolve the correct platform package instead of always using gnu. - Set CODEGRAPH_PARITY=1 env var in the parity job and use it in the test to force unconditional describe (no silent skip when native addon is expected to be present).
|
Addressed the outside-diff comment about The parity job now sets |
|
Claude finished @carlos-alm's task —— View job Review: Parity Test Silent Skip Fix ✅Reviewing commit 2f9f450 that addresses the Greptile feedback about Todo List
Analysis of Changes✅ Problem Solved CorrectlyThe solution elegantly addresses Greptile's P2 concern about silent skips in the parity CI job: Before (Problem):
After (Solution):
✅ Implementation QualityStrengths:
Edge Case Handling:
✅ CI IntegrationThe musl detection fix (also in 2f9f450) properly addresses the platform package resolution: // Lines 179-194 in ci.yml
if (plat === 'linux') {
try {
const files = fs.readdirSync('/lib');
libc = files.some(f => f.startsWith('ld-musl-') && f.endsWith('.so.1')) ? 'musl' : 'gnu';
} catch { libc = 'gnu'; }
}
// ... maps to correct @optave/codegraph-linux-x64-{gnu|musl} packageThis mirrors the logic in ✅ Testing StrategyThe approach correctly distinguishes between two scenarios:
This maintains usability for contributors while ensuring the dedicated parity gate actually enforces what it claims to test. Final AssessmentRating: ⭐⭐⭐⭐⭐ (5/5) This is an excellent fix that:
The solution demonstrates good engineering judgment by making the parity job fail hard when its core assumption (native addon availability) is violated, while preserving developer ergonomics in regular test runs. Recommendation: APPROVE - This properly addresses the review feedback and strengthens the test infrastructure without any downsides. |
#916) Ensures the dedicated parity CI job fails hard instead of silently skipping when the native addon is unavailable, matching the pattern already used in build-parity.test.ts.
|
Addressed the remaining Greptile finding about |
1 participant
Summary
build-parity.test.tsthat masked engine divergences:filterCallon ast_nodes,new_expressionedge filter, and Calculator role normalizationtoEqualassertions for nodes, edges, roles, and ast_nodesdescribeOrSkipwhen native was unavailableContext
The parity test was "passing" in CI for two reasons:
isNativeAvailable()returnedfalseand the entire suite was silently skippedThis PR fixes both: strict assertions catch real bugs, and the dedicated CI job ensures the tests actually execute. The native binaries are already published at 3.9.2 — if any assertion fails, it's a real engine bug that needs fixing.
Test plan