janestreet / janestreet/ppx_expect

Inline expect tests in a subdirectory crash at exit: Sys_error "../<file>.ml: No such file or directory" (regression in v0.18~preview.130.100+614)

Open
#65 0 comments 1 reaction 0 assignees View on GitHub
forwarded-to-js-devs
Dominant language
OCaml
Stars
194
Forks
35
PR merge metrics
No merged PRs in 30d

Description

Since `v0.18~preview.130.100+614`, every `dune runtest` for an inline-test library
located in a **subdirectory** of the project (e.g. `test/`) crashes at the exit-time
"write corrected file" step:

```
(Sys_error "../t.ml: No such file or directory")
```

The crash is **unconditional** — it fires for any expect-test file in a subdirectory
regardless of whether tests pass or fail, because the source file is opened at exit
before deciding whether any correction is needed. So the suite cannot run at all in
this configuration.

This is a regression: the immediately preceding release (`v0.18~preview.130.91+190`)
works fine in the exact same dune setup.

## Environment

- **ppx_expect / ppx_inline_test**: `v0.18~preview.130.100+614`
(commit `d36acdd3fb1409cd6422fe3064741251fe585061`, currently `master` HEAD)
- **dune**: 3.22.2 (any dune ≥ 3.22, which sandboxes inline-test runners)
- **OCaml**: 5.2.1

## Reproduction

```
repro/
dune-project ; (lang dune 3.22)
test/
dune ; (library (name t) (inline_tests) (preprocess (pps ppx_expect)))
t.ml ; let%expect_test "x" = print_string "hi"; [%expect {| hi |}]
```

```sh
dune runtest
```

**Expected:** the test passes (output matches), exit 0.

**Actual:**

```
File "test/dune", line 3, characters 1-15:
3 | (inline_tests)
^^^^^^^^^^^^^^
Uncaught exception:

(Sys_error "../t.ml: No such file or directory")

Raised by primitive operation at Stdlib.open_in_gen in file "stdlib.ml", line 405
Called from Stdlib.open_in_bin in file "stdlib.ml" (inlined), line 413
Called from Ppx_expect_runtime__Write_corrected_file.f in file "runtime/write_corrected_file.ml", line 42
Called from Base__List.map.map_loop in file "src/list.ml", line 913
Called from Ppx_expect_runtime.(fun) in file "runtime/ppx_expect_runtime.ml", lines 9-24
Called from Ppx_inline_test_lib.evaluate_exit_status in file "runtime-lib/ppx_inline_test_lib.ml", line 886
```

Note the test itself need not fail — an all-passing block crashes too, because the
source is read unconditionally at exit.

## Root cause

The regression is this hunk, introduced in `d36acdd` (`runtime/ppx_expect_runtime.ml`,
the `Ppx_inline_test_lib.add_evaluator` callback):

```diff
Test_node.Global_results_table.process_each_file
~f:(fun ~filename ~test_nodes ~postprocess ->
+ let filename =
+ match Ppx_inline_test_lib.source_tree_root () with
+ | None -> filename
+ | Some source_tree_root ->
+ Stdlib.Filename.concat source_tree_root (Stdlib.Filename.basename filename)
+ in
Write_corrected_file.f ... ~filename)
```

dune invokes the inline-test runner from the test directory and passes the **project
root** as `-source-tree-root` (it expands `%{workspace_root}`):

```
(cd _build/.sandbox//default/test
&& .t.inline-tests/inline-test-runner.exe inline-test-runner t \
-partition t.ml -source-tree-root .. -diff-cmd -)
```

So `source_tree_root` is `".."` and the runner's CWD is `.../default/test`. At this
point `filename` is the correct absolute path of the source
(`.../default/test/t.ml`, produced earlier by `Current_file.absolute_path`). The new
code rewrites it to:

```ocaml
Filename.concat ".." (Filename.basename ".../default/test/t.ml") (* = "../t.ml" *)
```

The evaluator has already `chdir`-ed to `Current_file.initial_dir` (the runner CWD,
`.../default/test`), so `"../t.ml"` resolves to `.../default/t.ml` — **one directory
too high**. The actual file is at `.../default/test/t.ml`. `Filename.basename`
discards the `test/` component, so `source_tree_root ^/ basename filename` can only
resolve when the source sits *directly* inside `source_tree_root` — i.e. only for a
library at the project root (where dune passes `-source-tree-root .`).

`Write_corrected_file.f` (`runtime/write_corrected_file.ml:42`) then opens that path
**unconditionally**, before deciding whether a correction is needed:

```ocaml
let f ... ~filename ... =
let dot_corrected = filename ^ ".corrected" in
let original_file_contents =
let in_channel = Stdlib.open_in_bin filename in (* Sys_error raised here *)
```

and `Test_node.Global_results_table.process_each_file` (`runtime/test_node.ml:343`)
calls it for every file that registered a test — hence the crash is independent of
pass/fail.

## Why it worked before `+614`

In `v0.18~preview.130.91+190` the evaluator passed `~filename` straight through, and
`filename` was already the correct absolute path (`initial_dir ^/ basename`, where
`initial_dir` is the runner's startup CWD = the test subdirectory). `source_tree_root`
was not consulted at all in the correction path, so `-source-tree-root ..` was
harmless and subdirectory libraries worked. dune's behavior is unchanged across both
versions; only the `+614` hunk above is new.

## Suggested fix

The re-rooting assumes `source_tree_root` is the directory *directly containing the
basename*, but dune passes the *project root* and `basename` drops any intervening
subdirectory. Either:

- preserve the path relative to the project root instead of taking `basename`
(so `test/t.ml` stays `test/t.ml` under `source_tree_root`), or
- drop the re-rooting and keep the pre-`+614` behavior of using the
already-correct absolute `filename`.

Contributor guide

Open the contributing guide

Research direction

Reproduce the failure with the provided subdirectory layout using dune runtest, then inspect the add_evaluator callback in runtime/ppx_expect_runtime.ml alongside runtime/write_corrected_file.ml and runtime/test_node.ml. Trace how source_tree_root and filename are passed to the correction step. Done means expect tests in a subdirectory run successfully without the ../t.ml Sys_error, including an all-passing test.

Written by the indexing model from the issue text.

Assessment

Tech stack
ocaml
Domain
testing
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.