Repository navigation
test: give the integration runner 40s per file instead of 30s - #515
Merged
Merged
Conversation
selection_held_spec.lua sleeps 21.6 s in 77 vim.wait calls, so its wall time is its own sleeping (21.0 to 22.9 s alone and beside a second full run), and 30 s left each call 0.1 s of slack on a loaded host. One run of #494 hit the limit. With every wait 0.2 s longer the spec takes 38.1 s: it is killed at 30 s and passes at 60 s, which leaves each call 0.5 s. No other file takes longer than 8 s. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
ThomasK33
marked this pull request as ready for review
November 11, 2026 15:21
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
plenary's test_file ends a file at 50 s on its own, without the runner's options: exit 1, no TIMEOUT line, and its nvim left running. The 60 s limit of the previous commit never fired for a hung file. The limit is now 40 s, so the limit plus the 5 s kill grace stays under 50 s, and each of the 77 vim.wait calls of selection_held_spec.lua has 0.24 s of slack instead of 0.11 s. The TIMEOUT line takes the limit from the variable that sets it. The runner spec fails when the limit plus the kill grace reaches 50 s, and when the TIMEOUT line names another limit than the one given to timeout. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
ThomasK33
changed the title
This was referenced Nov 11, 2026
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 12, 2026
…r#510) ## What changes The test suite now passes when `TMPDIR` holds a `%` or a relative path, for the unit suite and for the integration runner. Before this change, each case broke part of the suite on `main`. Only test files change. No script and no plugin code change. Unit suite, both on the same base (`d8e767c`): | `TMPDIR` | `main` | This branch | |---|---|---| | relative (`reltmp`) | 1450 passed, 13 failed (`integration_runner_spec`) | 1465 passed, 0 failed | | contains `%` | 1454 passed, 9 failed (`lockfile_test`) | 1465 passed, 0 failed | | unset | not run | 1465 passed, 0 failed | The branch adds two cases, so its suite has 1465 tests where `main` has 1463. Integration runner (`scripts/run_integration_tests_individually.sh`, all 22 files), on `main` `f932d7b` plus this branch's first two fixes, with plenary at CI's pinned commit and a stub `claude`: | Neovim | `TMPDIR` | Before the third fix | This branch | |---|---|---|---| | 0.12.3 | contains `%` | 28 failed (`diff_hand_over_spec`, `diff_new_tab_mode_spec`) | 291 passed, 0 failed | | v0.8.0 | contains `%` | 28 failed (same two files) | 204 passed, 0 failed | | 0.12.3 | relative | 291 passed, 0 failed | not rerun (no change to that path) | | v0.8.0 | relative | 204 passed, 0 failed | not rerun (no change to that path) | | 0.12.3, v0.8.0 | unset | not run | 291 and 204 passed, 0 failed | ## Why each case failed 1. **`%` in `TMPDIR`, unit suite.** `tests/lockfile_test.lua` expanded `~` with `path:gsub("~", home)`. `gsub` reads `%` in a replacement string as a capture reference, so `home` lost the character. The mock now gives `gsub` a function, which returns `home` unchanged. 2. **Relative `TMPDIR`.** `tests/unit/integration_runner_spec.lua` changes into a fake repository, then runs the integration runner. The runner calls `mktemp`, and a relative `TMPDIR` no longer named a folder after that `cd`. The spec now passes the folder that holds its temp dir as `TMPDIR` (`dir:match("^(.*)/[^/]+$")`), which is an absolute path. The script needs no change, because it never changes directory. 3. **`%` in `TMPDIR`, integration specs.** `diff_hand_over_spec.lua` (6 calls of `vim.cmd.edit(path)`) and `diff_new_tab_mode_spec.lua` (1 call of `vim.cmd("edit " .. dir .. "/a.txt")`) gave a temp path to `:edit` as it was. `:edit` reads a bare `%` in its argument as the current file name, so each call failed with `E499: Empty file name for '%'` (28 tests on both Neovim versions). Both specs now pass the path through `vim.fn.fnameescape`. I found this when I ran the integration runner under a `%` `TMPDIR` after coder#502 merged. CI does not run the integration runner under a `%` `TMPDIR`, so nothing guards these 7 calls. `tests/unit/tmpdir_suite_spec.lua` (new, 2 cases) runs one spec in a child `busted` under each odd `TMPDIR`, so a later change cannot bring the first two failures back silently. The relative-`TMPDIR` case is pending on a host with no `timeout` executable (stock macOS), because `integration_runner_spec.lua` registers only a pending case there. Codex found this in round 1 (P2). I reproduced it with `timeout` hidden from `PATH`, then fixed it in `b13bffa`. The lockfile case requires exit status 0 and at least one success, not an exact count, so a test added to `tests/lockfile_test.lua` (open PR coder#247 adds 7) cannot break it. ## Evidence - Gate on `19213e8`, with `main` `f932d7b` (coder#502) merged in: `mise run clean && mise run all` exit 0, seed 1794413672, 1517 passed, 0 failed, 20 pending. - Before the third fix (head `b7a2e93`, the merge only): the same gate gave 1517 passed, 0 failed, 20 pending (seed 1794412856). The full busted suite also passed under a `%` and a relative `TMPDIR` (1517 passed each, seed 777). - `tests/unit/integration_runner_spec.lua` 38 of 38, `tests/unit/tmpdir_suite_spec.lua` 2 of 2, `tests/lockfile_test.lua` 16 of 16 on the merge. - Mutant checks on the new spec (revert one fix at a time, run `tests/unit/tmpdir_suite_spec.lua`): | Mutant | Result | |---|---| | none | 2 passed, 0 failed | | `tests/lockfile_test.lua` back to `main` | lockfile case fails (child: 7 passed, 9 failed) | | `tests/unit/integration_runner_spec.lua` back to `main` | runner case fails (child: 0 passed, 1 failed) | ## Overlap with coder#502 coder#502 merged at 15:49:58Z. It changed `tests/unit/integration_runner_spec.lua`, which this branch also touches, and `scripts/run_integration_tests_individually.sh`, which this branch does not touch. Merging `main` into this branch had no conflict, and the runner spec passes 38 of 38 on the merge. coder#515 (merged later) also changed `tests/unit/integration_runner_spec.lua`; `git merge-tree` of this head with main after coder#515 and coder#480 is clean, and the merge queue reruns the tests on that merge. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 12, 2026
…der#522) ## What a developer sees The integration runner passes `{minimal_init = 'tests/minimal_init.lua'}` to plenary, but plenary's `test_harness.test_file(path)` takes no options and drops them. The Neovim that plenary starts for each spec has no `-u`, so: - it does not run `tests/minimal_init.lua`: no leader key, `timeoutlen` 1000 instead of 300, `updatetime` 4000 instead of 250, - it reads the developer's own init file, and runs the developer's `ftplugin` and `after` files, so a local run depends on `~/.config/nvim`, - every Neovim of a run writes ShaDa, a log and swap files under `~/.local/state/nvim`. The only part of `minimal_init.lua` that reached the spec's Neovim was the stub `claude` on `PATH`, which it inherited from the runner's own Neovim. ## What changes 1. **`VIMINIT`.** `scripts/run_integration_tests_individually.sh` sets `VIMINIT` for the Neovim it starts. A Neovim without `-u` runs `$VIMINIT` instead of the user's init file, so the Neovim that plenary starts now runs `tests/minimal_init.lua`. `VIMINIT` reads the file from `CLAUDECODE_MINIMAL_INIT`, an absolute path (`$PWD/tests/minimal_init.lua`), so a path with a space or `%` needs no escaping and a nested Neovim started from another folder finds the file. A comment says that `VIMINIT` and the variable reach every process a spec starts. The runner's own Neovim still has `-u`, and the unused `{minimal_init = ...}` argument is gone. 2. **State and config folders.** Each file gets one `XDG_STATE_HOME` and one `XDG_CONFIG_HOME`, in temp folders the runner removes afterwards. The Neovim the runner starts and the Neovim that plenary starts for the spec share them, so the runner's own Neovim takes `-i NONE`: two Neovims that write ShaDa into one new folder made the first fail with `E886` after a timeout. A run no longer writes `~/.local/state/nvim`, and the `ftplugin` and `after` files under `~/.config/nvim` do not run. Files in the data folder (`~/.local/share/nvim/site/...`) still run, as on `main`. `XDG_DATA_HOME` stays, because plenary lives there. 3. **Cleanup on SIGTERM.** An `EXIT` trap kills the process group of the file that runs, then removes its temp files and folders. Bash runs the trap when the runner gets SIGTERM too, just before the runner dies with 143 as it did before. On `main`, a runner that got SIGTERM left its temp files and left the file's Neovim running until its timeout. Killing the group first matters: a Neovim that ends later writes ShaDa and makes the removed state folder again. Two known limits: a SIGTERM that arrives before `timeout` has created the file's process group misses the kill (the file's Neovim then runs until its timeout, as on `main`), and on hosts whose `timeout` is uutils (this repo's CI image included) the SIGTERM test cannot tell a group kill from a kill of the `timeout` process alone. A follow-up PR fixes the test and the README wording. 4. **`tests/minimal_init.lua`** loses its `VimLeavePre` cleanup (`_G.claudecode_test_cleanup`). It ran only when `PLENARY_TEST_HARNESS` was set, and nothing sets it (not this repo and not plenary at CI's pin). I deleted it instead of arming it: it never ran, the plugin stops its own server in its `VimLeavePre` handler, and arming it would add an exit-time path to every spec Neovim, where a hung handler turns into a `TIMEOUT:`. 5. **Tests.** `tests/unit/integration_runner_spec.lua` gets six tests. The Neovim that plenary starts runs `tests/minimal_init.lua` of the working folder by an absolute path (the test runs the `VIMINIT` command with `dofile` stood in for). The `VIMINIT` command loads no file when `CLAUDECODE_MINIMAL_INIT` is missing (without its `if f then` guard, `dofile(nil)` would read standard input). Each file gets state and config folders that are removed afterwards. `XDG_DATA_HOME` is the one the runner got. The runner's own Neovim takes `-i NONE`. A runner that gets SIGTERM during a file stops that file's Neovim, leaves no temp file or folder, and exits 143. 6. `scripts/README.md` lists the two per-file temp folders (shared by the file's two Neovims), their removal including on SIGTERM, and the four variables the runner sets for each file's Neovim (`VIMINIT`, `CLAUDECODE_MINIMAL_INIT`, `XDG_STATE_HOME`, `XDG_CONFIG_HOME`). 7. A comment at `PLENARY_SHA` in `.github/workflows/test.yml` tells whoever bumps the pin to recheck plenary's own 50 s per-file timeout against the runner's `FILE_TIMEOUT`. 8. The cache-check comment in `.github/workflows/test.yml` now says which code writes under `stdpath("cache")`: only the legacy `diff.open_diff`, which `diff_legacy_swap_spec.lua` points at a temp folder. 9. `tests/integration/diff_hand_over_spec.lua` asserts that the `:split` step opened the expected file. ### Why `VIMINIT` and not `test_directory(<file>, opts)` `test_directory` or a direct `plenary.busted` call would carry the options, but they change the text `test_file('<file>'` in the runner's command line, which the stub `nvim` of the runner spec reads the file name from. `VIMINIT` keeps the command line as it is, and it also stops the developer's init file from running, which options alone would not. Its cost is that `VIMINIT` reaches nested Neovims too (the comment says so). ### One behavior change The spec Neovims of `command_args_spec.lua` and `mcp_tools_spec.lua` now run the `setup({ auto_start = false, log_level = "trace" })` of `tests/minimal_init.lua` before their tests, because their command line matches its `should_load` check. That adds debug log lines. The results are identical. ### The 50 s timeout plenary applies its own per-file timeout of 50 s (`timeout = 50000` in `test_harness.lua`), and `test_file` has no way to change it. The runner's `FILE_TIMEOUT` (40 s) plus its 5 s kill grace stays under it, as coder#515 set. ## Evidence The runner's real script on a tree whose only spec prints the state of its own Neovim, with a fake developer init file and a fake developer `ftplugin/lua.lua` in `XDG_CONFIG_HOME`: | Tree | `mapleader` | `timeoutlen` | `updatetime` | developer init ran | developer ftplugin ran | state folder under `$HOME` | |---|---|---|---|---|---|---| | `main` | none | 1000 | 4000 | yes | yes | yes | | this branch | `" "` | 300 | 250 | no | no | no | The whole integration runner, Neovim 0.12.3 and v0.8.0, `main` against this branch: 316 passed (237 rows on v0.8.0, 229 passed and 8 pending), 0 failed on both. The sorted `Success`, `Fail` and `Pending` rows are identical for both trees on both versions, and so are the success counts of all 24 files. The same runner with `HOME` set to an empty folder: | Tree | Left under that `HOME` | |---|---| | `main`, Neovim 0.12.3 | `.local/state/nvim/` with `lsp.log`, `nvim.log`, `shada`, `swap` | | `main`, Neovim v0.8.0 | `.local/state/nvim/` with `log`, `shada`, `swap` | | this branch, both versions | nothing | The two new runner spec tests, with each mutant of the script (a mutant is red when its row says "red"): | Mutant of the script | `VIMINIT` test | state and config test | |---|---|---| | none | green | green | | `VIMINIT` renamed so it is not set | red | green | | the variable names another file than `tests/minimal_init.lua` | red | green | | the variable holds a relative path | red | green | | the runner of `main` | red | red | | `XDG_STATE_HOME` and `XDG_CONFIG_HOME` not set | green | red | | the temp folders not removed | green | red | Two more mutants, for the two new assertions: | Mutant of the script | Assertion | Result | |---|---|---| | the `if f then` guard dropped | no file loaded without the variable | red (green on the head) | | `XDG_DATA_HOME` also set to a temp folder | `XDG_DATA_HOME` unchanged | red (green on the head) | Mutants for the two tests of this round (green on the head): | Mutant of the script | `-i NONE` test | SIGTERM test | |---|---|---| | `-i NONE` removed | red | green | | no `EXIT` trap (the trap on `HUP`) | green | red | | the trap without the process group kill | green | red | | the temp files not recorded for the trap | green | red | | the process group file not recorded | green | red | | the runner of `main` | red | red | Nested Neovim without `-u` from another folder (the verifier's finding): with the relative `luafile tests/minimal_init.lua` of the first version, both Neovim versions print `E5112 ... cannot open tests/minimal_init.lua` and run with defaults. With the absolute path in the variable, neither prints an error and `mapleader` is `" "`. A root folder named `odd dir 50%` works too. The swap file step assertion in `diff_hand_over_spec.lua`, under `TMPDIR` set to `odd tmp 50%`: | Plugin | Spec | Result | |---|---|---| | as is | with the assertion | 13 passed, 0 failed | | the step's `fnameescape` removed | with the assertion | 10 passed, 3 failed | | the step's `fnameescape` removed | as on `main` | 13 passed, 0 failed (the silent pass) | Without the odd `TMPDIR`, the mutant with the assertion is green too: the escape only matters for a `%` or a space. Hang probe from coder#515 (a spec that sleeps 120 s), three runs on this branch: each exits 1, prints `TIMEOUT: hang_spec.lua was stopped (exit 124): nvim did not exit within 40s`, and leaves no Neovim running. The `TIMEOUT:` line came after 40.4 s, 44.4 s and 40.5 s. The 44.4 s run was on a loaded host (the Opus verifier saw 41.8 s to 43.7 s at a load of 37 to 40, and 41.0 s to 42.2 s on `main`), so the limit plus the 5 s kill grace (45 s) is not far from a loaded run, and still under plenary's 50 s. The runner's state and config folders are gone from the run's `TMPDIR`. Only the temp folder of the killed Neovim itself is left, as before. `E886` after a timeout, the hang probe on the head of the previous round and on this branch, three runs each: | Head | Runs with an `E886` line | |---|---| | previous round (`6ed9c83`) | 1 of 3 (the Opus verifier saw 3 of 4) | | this branch | 0 of 3 | SIGTERM sent to the runner while the hang spec runs (the runner's `TMPDIR` is a fresh folder): | Tree | Runner exit | Left in `TMPDIR` right after | Hang spec's Neovim | |---|---|---|---| | `main` | 143 at once | 2 temp files | runs on until its timeout (39 s) | | this branch | 143 at once | nothing (only Neovim's own folder) | killed at once, nothing recreated at the end | The trap without the process group kill left a recreated state folder in `TMPDIR` after the hang Neovim ended. Gate: `mise run clean && mise run all` exit 0 (seed 1794435649, 1661 passed, 0 failed, 22 pending). `actionlint` and `shellcheck` pass. The whole runner on Neovim 0.12.3 gives 316 passed, 0 failed, with rows identical to `main` at the branch base. Merged with current `main` (which adds the 3 rows of coder#529's spec), it gives 319 passed on 0.12.3 and 232 passed plus 8 pending on v0.8.0, with rows identical to `main`. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
scripts/run_integration_tests_individually.shstops a file's Neovim after 30 s (timeout -k 5s 30s).tests/integration/selection_held_spec.luais a real-timer spec: it sleeps 21.6 s itself, in 77vim.waitcalls (counted by wrappingwaitin a copy of the spec:WAITED_MS=21640 CALLS=77). One run of #494 on a loaded host reached the 30 s limit and passed on rerun.I could not reproduce that run, but the arithmetic says why it can happen. The spec has 8.4 s of slack in 30 s, which is 0.11 s per
vim.waitcall. A loaded host that makes each call return 0.11 s late is enough.What changes
The per-file limit is 40 s instead of 30 s. That leaves each of the 77 calls 0.24 s of slack (18.4 s / 77), twice the slack it had.
scripts/run_integration_tests_individually.sh: one variable,FILE_TIMEOUT=40s, sets both thetimeoutlimit and the number in theTIMEOUT:line, so they cannot drift.scripts/README.mdline 22 names the new limit and the 50 s ceiling.tests/unit/integration_runner_spec.lua: the two assertions that pin the limit take the new value. Two new tests: one fails when the limit plus the kill grace reaches 50 s, and one fails when theTIMEOUT:line names another limit than the one given totimeout.I did not shorten the spec's waits. They are the debounce (100 ms) plus a margin on each step. Cutting them would trade a slow runner for a flaky spec.
Why the limit stays under 50 s
plenary's
test_harnessapplies its own per-file timeout of 50 s (timeout = 50000intest_harness.lua), andtest_filedoes not pass on the runner's options. So with a limit of 50 s or more, the runner'stimeoutnever decides. A hung file ends at about 51 s with exit 1, noTIMEOUT:line, and its nvim left running with parent pid 1. A first version of this PR used 60 s and had exactly that problem (found by the verifier). With 40 s, the limit plus the 5 s kill grace is 45 s, under 50 s.This is existing behavior on
main, and this PR does not change plenary's 50 s. Passing a timeout in the harness options is a separate follow-up.A hung file now costs 40 s instead of 30 s before the runner reports it.
Evidence
main(30 s)TIMEOUT:line at 30.8 s, no nvim leftTIMEOUT:line, nvim left running (ppid 1)TIMEOUT: hang_spec.lua was stopped (exit 124): nvim did not exit within 40s, saved nvim pid not runningselection_held_spec.luaTIMEOUT:linevim.wait0.2 s longer (+15.4 s), limit 30 sSuccessrows, noFailrow, no summaryvim.wait0.2 s longer, limit 40 sSo the failure mode of a slow host is the runner's limit, not an assertion: the file is killed while its tests still pass.
Mutants of the script, each run through
tests/unit/integration_runner_spec.luabefore #502 was merged in (15 tests, all pass unmutated):TIMEOUT:line names 30 sGate on the head with
mainf932d7b(#502) merged in:mise run clean && mise run allexit 0 (seed 1794412306, 1517 passed, 0 failed, 20 pending).tests/unit/integration_runner_spec.luapasses 40 of 40 on that merge, and the diff againstmainis these 3 files.The success count
Before #502, the runner printed
Total Success: 311where 290 cases ran: it counted each file'sSuccess:summary line as one more passed test (290Success ||rows plus 21 summary lines). #502 reads only the summary count, and its spec checks the totals. I ran #502's script on a tree with this change:Total Success: 290. That was before #501 merged; at this head, with 22 files, a local run and the CI stable job printTotal Success: 291(the v0.8.0 job prints 204). This PR does not touch that code.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh