test(parity): add native-engine guards for returnTypeMap and callAssignments#1287
Conversation
…gnments normalize() in parity.test.ts strips returnTypeMap and callAssignments from the structural comparison loop, so divergence between engines for those fields would have gone undetected — exactly the regression pattern from #1279/#1283. Two new explicit tests in the Cross-engine parity suite: - JS — returnTypeMap: verifies WASM produces a Map with `return new X()` inference (confidence 0.85), and that native produces an equivalent raw array (patchReturnTypeMap converts it to a Map in production). - JS — callAssignments: verifies WASM records cross-file receiver-typed method calls in callAssignments when the return type is not locally resolvable. Native is checked in its raw Vec form. Both native assertions soft-skip on binaries older than #1283 that predate the returnTypeMap/callAssignments fields (field absent = undefined). Closes #1286
|
Claude finished @carlos-alm's task —— View job PR Review: test(parity): add native-engine guards for returnTypeMap and callAssignmentsTasks:
Phase 1: Root Cause AnalysisProblem Being Solved: Why This Approach: Correct — adding explicit, isolated tests for fields excluded from Risk Assessment: Low. Pure test addition, no production code changes. Phase 2: Configuration Change DetectionNone. No lint rules disabled, no timeouts changed, no TypeScript flags relaxed. Clean. Phase 3: Test IntegrityThis is where I have substantive concerns: 1. Native assertions soft-skip on older binaries — but the skip mechanism is too permissive ( if (raw?.returnTypeMap === undefined) return;if (raw?.callAssignments === undefined) return;These guards silently pass if the binary predates #1283. That means in CI environments where the native binary hasn't been updated, both native assertion blocks become no-ops. The WASM side is validated; the native side is not. There's no warning or skip marker — the test name still says "populated by both WASM and native engines" even when only one engine was actually tested. The correct pattern for "known binary version gap" used elsewhere in this file is Recommended fix: Replace the silent if (raw?.returnTypeMap === undefined) {
// Binary predates #1283 — skip native assertion
return;
}...is arguably fine, but the comment at line 741 says "a simple existence check suffices" which frames this as intentional, so at minimum the test description should be 2. WASM-only expect(wasm?.callAssignments?.find((ca) => ca.varName === 'user')).toMatchObject({ ... });Using 3. The
This is accurate: 4. No assertion on The test only asserts that Phase 4: Code QualityThe tests are clear, well-commented, and correctly structured. The comment block at lines 729–741 is unusually thorough — it explains why the test exists (normalize() strips these fields), what the format difference is (Map vs array), and the binary-version soft-skip rationale. This is good practice. One minor nit: the native section at lines 769–784 uses an inline type cast: const entries = raw.returnTypeMap as Array<{ name: string; typeName: string; confidence: number; }>;This is fine since this is a test file and the native shape isn't typed through a shared interface. Phase 5: Backlog Compliance
Critical ConcernsMedium: Silent When the installed native binary predates #1283, both tests run and "pass" — but only the WASM half is actually validated. The test names claim "both WASM and native engines" are tested, which is misleading in those environments. In a parity CI job ( This isn't a blocker — the tests are still useful — but it violates the repo principle of "never silently skip verification" from CLAUDE.md. Final Recommendation |
Greptile SummaryThis PR closes a testing gap identified in #1286: the structural parity loop's
Confidence Score: 5/5Test-only change adding two well-scoped parity guards; no production code is touched and no existing tests are modified. The change is confined to the parity test file. Both new tests follow the established pattern of the file (WASM assertions first, guarded native assertions second, soft-skip for old binaries). Named intermediate variables with toBeDefined() messages make failures immediately actionable. No logic under test is altered. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Test: returnTypeMap / callAssignments] --> B{hasNative?}
B -- No --> C[Run WASM assertions only]
B -- Yes --> D[Run WASM assertions]
D --> E[nativeExtract code]
E --> F{field === undefined?}
F -- Yes --> G[Soft-skip: binary predates #1283]
F -- No --> H[Assert native raw array format]
C --> I[PASS / FAIL on WASM]
H --> J[PASS / FAIL on native]
Reviews (3): Last reviewed commit: "test(parity): add toBeDefined guards and..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| expect(entries.find((e) => e.name === 'UserService.getUser')).toMatchObject({ | ||
| name: 'UserService.getUser', | ||
| typeName: 'User', | ||
| confidence: 0.85, | ||
| }); | ||
| expect(entries.find((e) => e.name === 'UserService.buildQuery')).toMatchObject({ | ||
| name: 'UserService.buildQuery', | ||
| typeName: 'QueryBuilder', | ||
| confidence: 0.85, | ||
| }); |
There was a problem hiding this comment.
find() result passed directly to toMatchObject without a prior existence check
When find() returns undefined (entry absent), Vitest's toMatchObject surfaces a generic "expected undefined to match object" failure rather than pointing at the missing entry name. The same pattern recurs in the native callAssignments block (~line 818). Adding an intermediate toBeDefined() with a custom label makes the failure immediately actionable without changing test semantics.
There was a problem hiding this comment.
Fixed in 9b508e4. Split each find() chain into a named intermediate variable and added toBeDefined(message) before toMatchObject() — all 6 affected assertions (2 WASM + 4 native across both tests). Failure messages now name the missing entry (e.g. "WASM returnTypeMap missing UserService.getUser") instead of surfacing a generic "expected undefined to match object".
…) chains - Split find().toMatchObject() into named intermediate variables with toBeDefined(message) before each toMatchObject(), so failures name the missing entry rather than surfacing a generic "expected undefined to match object" error. - Rename both test titles from "populated by both WASM and native engines" to "populated by WASM; native when binary >= #1283" to accurately reflect that native assertions are soft-skipped on pre-#1283 binaries. Addresses Greptile P2 (find result passed directly to toMatchObject without existence check) and Claude review note on misleading test descriptions.
|
Addressed in 9b508e4:
The |
1 participant
Summary
tests/engines/parity.test.tsforreturnTypeMapandcallAssignments, the fields introduced by fix(native): implement return-type + call-assignment extraction in Rust engine #1283normalize()in the structural loop strips these fields, so any future divergence between engines would go undetected — these tests close that gapreturnTypeMaptest — verifies WASM produces aMapwithreturn new Constructor()inference (confidence 0.85); verifies native produces an equivalent raw array (the form beforepatchReturnTypeMapconverts it to a Map in production)callAssignmentstest — verifies WASM records method calls incallAssignmentswhen the callee's return type is not locally resolvable (cross-file propagation case); verifies native produces the sameBoth native assertion blocks soft-skip on binaries older than #1283 (field absent =
undefined).Test plan
npx vitest run tests/engines/parity.test.ts -t "returnTypeMap|callAssignments"— 2 passednpx vitest run tests/parsers/javascript.test.ts— 63 passedCloses #1286