Skip to content

Commit 91a6518

Browse files
fix(res-to-affine): readiness guard must fail, not skip (#742)
Implements the owner ruling of 2026-09-07: **no environment may silently run zero tests.** ## The fake green `test_walker.ml:65 skip_unless_ready()` called `Alcotest.skip()` when the tree-sitter CLI or the generated grammar was absent. `tools/vendor/tree-sitter-rescript/src/parser.c` is gitignored (`.gitignore:112`), so **every fresh checkout** took that path: all 32 walker cases skipped and alcotest printed ``` Test Successful in 0.018s. 0 test run. ``` with exit 0. The suite was green precisely when it was testing nothing. This is not hypothetical. It is why the deletion of this suite's own fixtures in `f766dcb` went unnoticed for three weeks (restored in #741): in any environment lacking the grammar, a missing test corpus produced a pass. ## The change Renames the guard to `require_ready()` and replaces both `Alcotest.skip()` calls with `Alcotest.failf` carrying the remedy (`cargo install tree-sitter-cli` / `just install-grammar`). One file, 32 call sites renamed mechanically. ## Verified, both directions | Grammar | Result | |---|---| | absent | `rc=1` — `32 failures! in 0.020s. 32 tests run.` | | present | `rc=0` — `Test Successful in 1.257s. 32 tests run.` | The important column is the **count**: previously the absent case reported `0 test run` and passed. `ci.yml:70-79` installs the CLI and builds the grammar before `dune runtest`, so CI lands in the passing row. ## Accepted cost A fresh clone can no longer run the walker suite without installing the tree-sitter CLI and building the grammar first. That is the deliberate trade, chosen over the milder "fail under CI, skip locally". ## Note `ocamlformat` is not available in my environment, so I could not pre-check `dune build @fmt`. If the formatting gate objects, say so and I will correct it. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 209c96e commit 91a6518

2 files changed

Lines changed: 73 additions & 51 deletions

File tree

.github/workflows/ci.yml

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,11 @@ jobs:
7171
# Same rationale as the migration-assistant job (see below):
7272
# npm distribution is the fast CI install (~5 s). The walker
7373
# end-to-end tests in tools/res-to-affine/test/test_walker.ml
74-
# auto-skip if the CLI / generated grammar aren't present, so
75-
# this step is only required to *exercise* the walker — the
76-
# build itself does not depend on it.
74+
# now FAIL rather than skip when the CLI or the generated
75+
# grammar is absent, so this step is a hard prerequisite for
76+
# `dune runtest` — not an optional extra that merely widens
77+
# coverage. Dropping it turns the walker suite red, which is
78+
# the intended behaviour: see that file's header comment.
7779
run: npm install -g tree-sitter-cli@^0.25.0
7880
- name: Build pinned tree-sitter-rescript grammar
7981
run: ./editors/tree-sitter-rescript/scripts/install.sh
@@ -224,6 +226,20 @@ jobs:
224226
opam exec -- ocaml -version
225227
- name: Install dependencies
226228
run: opam install . --deps-only --with-test --with-doc --yes
229+
- name: Set up Node.js
230+
uses: actions/setup-node@v7.0.0
231+
with:
232+
node-version: "20"
233+
- name: Install tree-sitter CLI (for res-to-affine walker tests)
234+
# This job runs the WHOLE `dune runtest` sweep, which includes
235+
# tools/res-to-affine/test/test_walker.ml. Those tests now fail
236+
# rather than skip when the CLI or generated grammar is absent,
237+
# so this job needs the same grammar prerequisites as `build`.
238+
# Before the skip was removed, this job was green while running
239+
# zero walker tests.
240+
run: npm install -g tree-sitter-cli@^0.25.0
241+
- name: Build pinned tree-sitter-rescript grammar
242+
run: ./editors/tree-sitter-rescript/scripts/install.sh
227243
- name: Run tests with bisect_ppx instrumentation
228244
run: |
229245
opam exec -- dune runtest --force --instrument-with bisect_ppx

tools/res-to-affine/test/test_walker.ml

Lines changed: 54 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,16 @@
33

44
(** End-to-end tests for the tree-sitter walker (#57 Phase 2b).
55
6-
These tests shell out to the [tree-sitter] CLI; they are
7-
automatically skipped when the CLI is not on PATH so a fresh
8-
clone can still run [dune runtest] without bootstrapping the
9-
grammar. CI installs tree-sitter and runs them as a gate.
6+
These tests shell out to the [tree-sitter] CLI. They FAIL -- they do
7+
not skip -- when the CLI or the generated grammar is absent.
8+
9+
A skip here was a fake green: [Alcotest.skip] on every case makes
10+
alcotest print "Test Successful ... 0 test run" and exit 0, so the
11+
suite was green precisely when it tested nothing. [src/parser.c] is
12+
gitignored, so every fresh checkout hit that path. That is how the
13+
deletion of this suite's own fixtures (f766dcb) went unnoticed for
14+
three weeks. Owner ruling 2026-09-07: no environment may silently
15+
run zero tests.
1016
1117
To run locally: install tree-sitter (`cargo install
1218
tree-sitter-cli`), then `just install-grammar`, then `dune
@@ -62,23 +68,23 @@ let grammar_dir () =
6268
let grammar_built () =
6369
Sys.file_exists (Filename.concat (grammar_dir ()) "src/parser.c")
6470

65-
let skip_unless_ready () =
66-
if not (tree_sitter_available ()) then begin
67-
Printf.printf
68-
" [skip] tree-sitter CLI not on PATH; install via `cargo install \
69-
tree-sitter-cli`@\n";
70-
Alcotest.skip ()
71-
end;
72-
if not (grammar_built ()) then begin
73-
Printf.printf
74-
" [skip] grammar not built; run `just install-grammar`@\n";
75-
Alcotest.skip ()
76-
end
71+
(* Deliberately fails rather than skips: see the header comment. *)
72+
let require_ready () =
73+
if not (tree_sitter_available ()) then
74+
Alcotest.failf
75+
"tree-sitter CLI not on PATH. These tests are a gate, not an \
76+
optional extra; install it with `cargo install tree-sitter-cli`."
77+
;
78+
if not (grammar_built ()) then
79+
Alcotest.failf
80+
"tree-sitter grammar not built (%s/src/parser.c is absent). \
81+
Run `just install-grammar`."
82+
(grammar_dir ())
7783

7884
let fixture = "fixtures/sample.res"
7985

8086
let test_walker_finds_side_effect_import () =
81-
skip_unless_ready ();
87+
require_ready ();
8288
let source = read_file fixture in
8389
let path = Filename.concat (Sys.getcwd ()) fixture in
8490
let findings =
@@ -99,7 +105,7 @@ let test_walker_only_module_toplevel () =
99105
fixture, which has the regex-scanner-matching shape at module
100106
top level — the walker should match it. The negative case lives
101107
in Phase 2c's expanded corpus. *)
102-
skip_unless_ready ();
108+
require_ready ();
103109
let source = read_file fixture in
104110
let path = Filename.concat (Sys.getcwd ()) fixture in
105111
let findings =
@@ -135,15 +141,15 @@ let lines_for_kind findings k =
135141
findings
136142

137143
let test_walker_finds_raw_js () =
138-
skip_unless_ready ();
144+
require_ready ();
139145
let findings = scan_sample () in
140146
(* sample.res line 11: `let host = %raw(`globalThis.location.host`)`. *)
141147
Alcotest.(check (list int))
142148
"walker reports raw-js on line 11"
143149
[11] (lines_for_kind findings Scanner.Raw_js)
144150

145151
let test_walker_finds_mutable_global () =
146-
skip_unless_ready ();
152+
require_ready ();
147153
let findings = scan_sample () in
148154
let got = lines_for_kind findings Scanner.Mutable_global in
149155
(* sample.res line 14: `let currentUser = ref(None)` — top-level
@@ -154,7 +160,7 @@ let test_walker_finds_mutable_global () =
154160
true (List.mem 14 got && List.mem 15 got)
155161

156162
let test_walker_finds_untyped_exception () =
157-
skip_unless_ready ();
163+
require_ready ();
158164
let findings = scan_sample () in
159165
let got = lines_for_kind findings Scanner.Untyped_exception in
160166
(* sample.res has untyped-exception flavours at: line 19 (`try {`),
@@ -174,7 +180,7 @@ let test_walker_finds_untyped_exception () =
174180
let phase2c_fixture = "fixtures/phase2c.res"
175181

176182
let test_walker_finds_inline_callback_record () =
177-
skip_unless_ready ();
183+
require_ready ();
178184
let source = read_file phase2c_fixture in
179185
let path = Filename.concat (Sys.getcwd ()) phase2c_fixture in
180186
let findings =
@@ -186,7 +192,7 @@ let test_walker_finds_inline_callback_record () =
186192
true (got <> [])
187193

188194
let test_walker_finds_oversized_function () =
189-
skip_unless_ready ();
195+
require_ready ();
190196
let source = read_file phase2c_fixture in
191197
let path = Filename.concat (Sys.getcwd ()) phase2c_fixture in
192198
let findings =
@@ -224,28 +230,28 @@ let translate_phase3_blob () =
224230
String.concat "\n" (List.map snd (translate_phase3 ()))
225231

226232
let test_translate_count () =
227-
skip_unless_ready ();
233+
require_ready ();
228234
(* userId, color, shape, and (slice 2) the generic box — 4. theirMap
229235
(qualified) and the let/switch stay skipped. *)
230236
Alcotest.(check int)
231237
"four structural type decls are translated"
232238
4 (List.length (translate_phase3 ()))
233239

234240
let test_translate_generic_sum () =
235-
skip_unless_ready ();
241+
require_ready ();
236242
let blob = translate_phase3_blob () in
237243
Alcotest.(check bool)
238244
"generic sum -> type Box[A] = | Box(A)"
239245
true (contains blob "type Box[A] =" && contains blob "| Box(A)")
240246

241247
let test_translate_alias () =
242-
skip_unless_ready ();
248+
require_ready ();
243249
Alcotest.(check bool)
244250
"primitive alias -> capitalised TyCon + Int"
245251
true (contains (translate_phase3_blob ()) "type UserId = Int")
246252

247253
let test_translate_nullary_sum () =
248-
skip_unless_ready ();
254+
require_ready ();
249255
let blob = translate_phase3_blob () in
250256
let ok =
251257
contains blob "type Color =" && contains blob "| Red"
@@ -254,7 +260,7 @@ let test_translate_nullary_sum () =
254260
Alcotest.(check bool) "nullary sum -> leading-pipe variant form" true ok
255261

256262
let test_translate_payload_sum () =
257-
skip_unless_ready ();
263+
require_ready ();
258264
let blob = translate_phase3_blob () in
259265
let ok =
260266
contains blob "type Shape =" && contains blob "| Circle(Float)"
@@ -263,7 +269,7 @@ let test_translate_payload_sum () =
263269
Alcotest.(check bool) "primitive-payload sum -> mapped param types" true ok
264270

265271
let test_translate_skips_non_structural () =
266-
skip_unless_ready ();
272+
require_ready ();
267273
let blob = translate_phase3_blob () in
268274
(* the qualified Belt.Map.t and the let/switch must stay absent — the tool
269275
never guesses them; and no raw ReScript type-var ['a] leaks through. *)
@@ -291,14 +297,14 @@ let translate_phase3b_blob () =
291297
String.concat "\n" (List.map snd (translate_phase3b ()))
292298

293299
let test_translate_b_count () =
294-
skip_unless_ready ();
300+
require_ready ();
295301
(* point, box, id translate; counter (mutable) and config (optional) skip. *)
296302
Alcotest.(check int)
297303
"three of five record/generic decls translate"
298304
3 (List.length (translate_phase3b ()))
299305

300306
let test_translate_record () =
301-
skip_unless_ready ();
307+
require_ready ();
302308
let blob = translate_phase3b_blob () in
303309
let ok =
304310
contains blob "struct Point {" && contains blob "x: Int"
@@ -307,19 +313,19 @@ let test_translate_record () =
307313
Alcotest.(check bool) "record -> struct with mapped field types" true ok
308314

309315
let test_translate_generic_record () =
310-
skip_unless_ready ();
316+
require_ready ();
311317
let blob = translate_phase3b_blob () in
312318
let ok = contains blob "struct Box[A] {" && contains blob "value: A" in
313319
Alcotest.(check bool) "generic record -> struct with type params" true ok
314320

315321
let test_translate_generic_alias () =
316-
skip_unless_ready ();
322+
require_ready ();
317323
Alcotest.(check bool)
318324
"generic alias -> type Id[A] = A"
319325
true (contains (translate_phase3b_blob ()) "type Id[A] = A")
320326

321327
let test_translate_b_skips () =
322-
skip_unless_ready ();
328+
require_ready ();
323329
let blob = translate_phase3b_blob () in
324330
(* mutable + optional records must be skipped, never silently flattened. *)
325331
let leaked =
@@ -345,14 +351,14 @@ let translate_phase3c_blob () =
345351
String.concat "\n" (List.map snd (translate_phase3c ()))
346352

347353
let test_translate_c_count () =
348-
skip_unless_ready ();
354+
require_ready ();
349355
(* answer, pi, greeting, enabled, disabled -> 5; now/counter/(a,b) skip. *)
350356
Alcotest.(check int)
351357
"five literal let-bindings translate to const"
352358
5 (List.length (translate_phase3c ()))
353359

354360
let test_translate_const_int_float () =
355-
skip_unless_ready ();
361+
require_ready ();
356362
let blob = translate_phase3c_blob () in
357363
let ok =
358364
contains blob "const answer: Int = 42;"
@@ -361,7 +367,7 @@ let test_translate_const_int_float () =
361367
Alcotest.(check bool) "int + float literal -> typed const" true ok
362368

363369
let test_translate_const_string_bool () =
364-
skip_unless_ready ();
370+
require_ready ();
365371
let blob = translate_phase3c_blob () in
366372
let ok =
367373
contains blob "const greeting: String = \"hi\";"
@@ -371,7 +377,7 @@ let test_translate_const_string_bool () =
371377
Alcotest.(check bool) "string + bool literal -> typed const" true ok
372378

373379
let test_translate_c_skips () =
374-
skip_unless_ready ();
380+
require_ready ();
375381
let blob = translate_phase3c_blob () in
376382
(* call / ref / destructuring bindings must never become a const. *)
377383
let leaked =
@@ -399,42 +405,42 @@ let partial1_blob () =
399405
String.concat "\n" (List.map snd (translate_partial1 ()))
400406

401407
let test_partial_count () =
402-
skip_unless_ready ();
408+
require_ready ();
403409
Alcotest.(check int)
404410
"eleven module-top-level functions -> fn skeletons"
405411
11 (List.length (translate_partial1 ()))
406412

407413
let test_partial_array () =
408-
skip_unless_ready ();
414+
require_ready ();
409415
Alcotest.(check bool) "array literal translated"
410416
true (contains (partial1_blob ()) "[x, x]")
411417

412418
let test_partial_record () =
413-
skip_unless_ready ();
419+
require_ready ();
414420
(* nominal placeholder type `Rec`; field punning {x} -> x: x *)
415421
Alcotest.(check bool) "record literal -> Rec #{ ... }"
416422
true (contains (partial1_blob ()) "Rec #{ x: x, y: y }")
417423

418424
let test_partial_pipe () =
419-
skip_unless_ready ();
425+
require_ready ();
420426
let blob = partial1_blob () in
421427
(* x->doStuff(1) -> doStuff(x, 1); chain x->f->g(2) -> g(f(x), 2) *)
422428
Alcotest.(check bool) "pipe-first desugars, including left-nested chains"
423429
true (contains blob "doStuff(x, 1)" && contains blob "g(f(x), 2)")
424430

425431
let test_partial_if () =
426-
skip_unless_ready ();
432+
require_ready ();
427433
Alcotest.(check bool) "if/else translated"
428434
true (contains (partial1_blob ()) "if x > 0 { x } else { 0 }")
429435

430436
let test_partial_block () =
431-
skip_unless_ready ();
437+
require_ready ();
432438
let blob = partial1_blob () in
433439
Alcotest.(check bool) "block body with a let statement translated"
434440
true (contains blob "let y = x + 1" && contains blob "y * 2")
435441

436442
let test_partial_switch_to_match () =
437-
skip_unless_ready ();
443+
require_ready ();
438444
let blob = partial1_blob () in
439445
let ok =
440446
contains blob "fn classify(x: _) -> _" && contains blob "match x {"
@@ -443,7 +449,7 @@ let test_partial_switch_to_match () =
443449
Alcotest.(check bool) "switch -> match with translated arms + patterns" true ok
444450

445451
let test_partial_float_op_normalised () =
446-
skip_unless_ready ();
452+
require_ready ();
447453
let blob = partial1_blob () in
448454
Alcotest.(check bool) "float op normalised; multi-param skeleton"
449455
true
@@ -452,14 +458,14 @@ let test_partial_float_op_normalised () =
452458
&& not (contains blob "*."))
453459

454460
let test_partial_concat_and_call () =
455-
skip_unless_ready ();
461+
require_ready ();
456462
let blob = partial1_blob () in
457463
Alcotest.(check bool) "string concat + member-call translated"
458464
true
459465
(contains blob "\"hi \" ++ name" && contains blob "Js.log(msg)")
460466

461467
let test_partial_todo_hole () =
462-
skip_unless_ready ();
468+
require_ready ();
463469
let blob = partial1_blob () in
464470
Alcotest.(check bool) "untranslatable form becomes a () /* TODO */ hole"
465471
true (contains blob "() /* TODO:")

0 commit comments

Comments
 (0)