test: strengthen suite + restructure + convert e2e scenarios + fix explorer refresh race - #511
Merged
Conversation
The 5 scenarios under tests/e2e/ (explorer_layout, explorer_toggle, explorer_tree, history_layout, tab_cycle_untracked) were consumed only by scripts/nvim-e2e.lua and never invoked by any GitHub workflow — the in-tree test framework discovers *_spec.lua under tests/, not the scenario tables under tests/e2e/. Post-audit these were the only tests in the tree that could break silently on main. Add a thin shell wrapper (tests/run_e2e.sh + .cmd) that iterates the scenarios, launches one Neovim per scenario via SCENARIO_FILE (which triggers the runner's auto-cquit(1) on failure), and returns non-zero if any scenario fails. Wire it via 'make test-e2e' and add the step to _platform-linux.yml immediately after 'make test-lua'. Linux-only for now: scenarios exercise UI layout which is platform-agnostic; expanding to macOS/Windows if they stay green for a release is straightforward. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Audit found 17 tests spread across lifecycle_spec.lua (7), view_spec.lua (6), and core_spec.lua (rest) that only asserted 'no throw' after wrapping the SUT in pcall. State-mutating bugs (leaked sessions, render no-ops, buffer clobbering) would pass silently. Verified via bug injection: commenting out 'active_diffs[tabpage] = nil' in cleanup.lua:130 (silent session-registry leak) now fails 3 tests in this file; before the strengthening it would have passed all of them. Each strengthened case now also verifies a state predicate: - lifecycle_spec.lua: cleanup_all removes registry entries; re-registration overwrites session fields; cleanup(invalid_tab) doesn't touch other sessions; cleanup with deleted buffers/closed windows leaves get_session(tp) == nil; empty-diff session is still registered with valid buffer numbers. - view_spec.lua: view.create's promised session lands (with an async-safe vim.wait guard, since side_by_side registers via vim.schedule); buffers hold the expected content; iterating create produces distinct sessions matching each iteration's input. - core_spec.lua: render_diff on empty-vs-content preserves both buffers' source data and doesn't corrupt them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The test was previously marked pending() because it used 'vim.wait(10000, function() return false end, 50)' — an unconditional 10-second sleep that raced with the two-step async chain (git apply --reverse -> refresh -> status -> re-render), yielding flakes on Windows CI. Convert to an it() with a deterministic predicate-based wait on the welcome buffer becoming visible (that terminal state is reached only after the entire chain lands). Also stub vim.fn.confirm (used by discard_hunk for its destructive-op prompt) to return 1 = '&Discard' since headless nvim otherwise returns 0 = 'no user input' and the whole callback silently aborts. Adds a working-tree file-content sanity check so a stale UI can't disguise a real discard failure. Local run: 585ms (vs the pending version's guaranteed 10s+ sleep). Full layout_toggle_spec.lua: 14 passed, 0 failed, 0 pending. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Makefile at the repo root is regenerated by 'file(WRITE ...)' in CMakeLists.txt on every 'cmake -B build -S .' (which the CI 'make build' step runs). My hand-edit to add 'test-e2e' to the tree Makefile got silently overwritten in CI, causing 'make test-e2e' to fail with 'No rule to make target'. Fix at the source: add test-e2e to the .PHONY list, the default 'test' target, and the help string in the Makefile template embedded in CMakeLists.txt (Linux/macOS block). Windows nmake wrapper left as-is; the e2e job is Linux-only for now. Verified locally: regenerated Makefile via 'cmake -B build -S .', 'make test-e2e' now finds the rule and passes (5/5 scenarios). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pivot from the shell-wrapped scenario runner introduced in 6ddb099 + 0be9a1d to the framework's native describe/it pattern: - Rewrite each of the 5 tests/e2e/*.lua scenarios into a proper *_spec.lua file that the in-tree framework auto-discovers. - Delete the shell-runner scaffolding (scripts/nvim-e2e.lua, tests/run_e2e.sh, tests/run_e2e.cmd), the make test-e2e target (both live Makefile and the CMake template in CMakeLists.txt), and the extra CI step in _platform-linux.yml. - Add find_window_by_filetype, wait_for_explorer, wait_for_diff_ready to tests/helpers.lua so the converted specs use the same helper surface as the other 80 specs. Coverage is unchanged — every setup/run/validate/cleanup callback in the scenarios maps 1:1 to before_each/it/after_each blocks with the same assertions, plus more explicit assert.* calls (instead of one boolean returned from validate()) so failures now surface with precise messages. The scenarios were orphaned by the July 2026 in-tree framework rewrite (5d8c4cb): the framework discovers *_spec.lua, not { setup, run, validate, cleanup } tables. This migrates them onto the same runway. Placement: tests/e2e/explorer_layout -> tests/ui/explorer/explorer_layout_e2e_spec.lua tests/e2e/explorer_toggle -> tests/ui/explorer/explorer_toggle_e2e_spec.lua tests/e2e/explorer_tree -> tests/ui/explorer/explorer_tree_e2e_spec.lua tests/e2e/history_layout -> tests/ui/history/history_layout_e2e_spec.lua tests/e2e/tab_cycle_untracked -> tests/ui/explorer/tab_cycle_untracked_e2e_spec.lua Full suite: 85 spec files, 35.0s, all green (vs 80 + 5-scenario shell path in 36.1s before this commit). Net diff: -988 lines removed, +436 lines added. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Four categories of changes; net -356 lines, 85 -> 83 spec files, and
the framework can now parallelize the previously monolithic
explorer_spec.lua across two workers (wall-clock 35.0s -> 28.4s in
a full local run).
A. Move specs whose source is elsewhere:
tests/ui/scrollsync_spec.lua -> tests/scrollsync_spec.lua
(source: lua/codediff/scrollsync.lua, top-level not under ui/)
tests/ui/keymap/ -> tests/keymap/
(source: lua/codediff/keymap/, top-level not under ui/)
tests/flatten_dirs_spec.lua -> tests/ui/explorer/flatten_dirs_spec.lua
(tests an explorer feature; belongs with the other explorer specs)
tests/ui/core_spec.lua -> tests/ui/render_core_spec.lua
(source: lua/codediff/ui/core.lua; renamed so it no longer collides
mentally with tests/core/ which covers lua/codediff/core/*)
B. Split explorer_spec.lua at its natural describe seam:
explorer_spec.lua -> explorer_spec.lua (17 tests)
+ explorer_refresh_spec.lua (5 tests)
Same coverage; now runs as two parallel workers instead of one 34s
serial file. Same before_each/setup_command scaffolding duplicated
(deliberate — the two describes were self-contained already).
C. Consolidate 4 near-identical *_modules_spec.lua into one file:
explorer_modules_spec.lua + lifecycle_modules_spec.lua +
conflict_modules_spec.lua + history_modules_spec.lua
-> tests/module_loading_spec.lua
All 43 tests preserved; one place to keep the require-graph +
public-API smoke in sync with the source.
D. Drop the _e2e_spec suffix on the recently-converted specs and
normalize the issue-# convention:
explorer_layout_e2e_spec.lua -> explorer_window_layout_spec.lua
explorer_toggle_e2e_spec.lua -> explorer_visibility_spec.lua
explorer_tree_e2e_spec.lua -> explorer_tree_render_spec.lua
history_layout_e2e_spec.lua -> history_layout_spec.lua
tab_cycle_untracked_e2e_spec.lua -> issue_309_spec.lua
(matches sibling issue_390_spec.lua / issue_496_spec.lua /
issue_498_spec.lua naming in the same directory)
Internal describe("… (E2E)") titles updated to match.
Verified: bash tests/run_tests.sh -> ALL TESTS PASSED (83 spec files,
28.4s), same 866-test surface as before this commit.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ot yet a repo
The 500ms explorer polling timer could fire against a directory that
is either being torn down or hasn't yet been initialized as a git
repo, at which point M.refresh -> git.get_status returned 'not a git
repository (or any of the parent directories): .git' and process_result
surfaced it as vim.notify(..., ERROR).
Two real races (both observed in Windows CI, one intermittently on Linux):
1. Teardown race: an 'after_each' does rm -rf on the temp repo and
the TabClosed autocmd running the timer cleanup hadn't fired yet.
Any in-flight scheduled tick then polled the deleted dir.
2. Setup race: on a slow filesystem, :CodeDiff opens the explorer
before 'git init' has finished writing .git/. The first 500ms
tick beats the initialization.
Fix at the source: at the top of tick(), if explorer.git_root doesn't
exist or has no .git (either directory or file, the latter covers
worktrees and submodules whose .git is a gitdir pointer), silently
return. The next tick will pick up the state once it's valid, or the
tab will be gone.
User-visible improvement: rm -rf on your own repo behind the explorer
no longer flashes an ERROR notification.
Test-visible improvement: eight or so spec files that were emitting
'Failed to refresh: fatal: not a git repository' / 'Invalid revision
HEAD' etc. to stderr (visible in the CI log's per-spec stderr blocks)
now run silently. The specs already passed, the fix is only about
noise, but the noise was masking real issues if any appeared.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tion
The pre-strengthening assertion checked only that a modified buffer
still existed after ]f fired. That leftover buffer was ALWAYS present
because :CodeDiff opens with an initial selection, so the assertion
passed even when ]f itself no-op'd. Verified locally by injecting
`do return end` at the top of navigation.next_file — the old assertion
passed anyway, exactly the kind of hidden-failure test the audit was
meant to catch.
Two fixes:
1. Call navigation.next_file() directly instead of feeding "]f"
through nvim_feedkeys(..., "nx"). The tab-scoped keymap ]f is
bound via lifecycle.set_tab_keymap, and feedkeys drains against
whichever buffer is current at drain time, which in headless
runs can end up being the explorer buffer with its own maps
shadowing the tab-level entry, silently missing the navigation
call and printing E447 to stderr. Calling the module directly
tests the actual navigation logic without the keymap-plumbing
noise.
2. Assert that explorer.current_file_path CHANGES after next_file()
and that the modified pane's buffer name matches the newly
selected file. Uses a predicate-based vim.wait so the async diff
render has time to land, then asserts the terminal state.
Guard: if there's fewer than 2 changed files (next_file cycle
is 1 element), fail early with a clear message instead of
silently having next_file be a no-op-by-design.
Also add vim.fn.fnameescape to the initial :edit for path-safety.
Bug-injection verification: with `do return end` at the top of
navigation.next_file:
BEFORE this commit: test passes (the bug is hidden)
AFTER this commit: test fails with 'next_file must select a
different file; still on src/a.txt'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
esmuellert
enabled auto-merge
August 2, 2026 05:53
esmuellert
disabled auto-merge
August 2, 2026 07:07
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
1 participant
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.
Summary
A test-suite audit pass. Nine commits, ranging from CI wire-up through a real behavior fix in the explorer's auto-refresh timer. All specs green on every platform (linux-x64/arm64, macos-x64/arm64, windows-x64/arm64, android-x64/arm64) plus 5 standalone-build variants; previous PR-validation run at branch tip already verified this before the rebase.
Net change to the test tree: 80 spec files (+ 5 orphaned scenarios) → 83 spec files, one uniform runner, ~30s wall-clock (previously 36s + a separate shell path).
Commits (in order)
1.
test: strengthen 'success = pcall / assert.is_true(success)' checks17 tests across
lifecycle_spec.lua(7),view_spec.lua(6),core_spec.lua(rest) that only asserted "no throw" now also verify state predicates. Bug-injection proof: commenting outactive_diffs[tabpage] = nilincleanup.lua:130now fails 3 tests that used to pass.2.
test(layout_toggle): implement 'keeps discard hunk working after toggle'Was
pending()because it usedvim.wait(10000, () -> false)— an unconditional 10-second sleep that raced with the async discard chain. Converted to a realit()with a predicate-based wait on the welcome buffer becoming visible; stubbedvim.fn.confirmfor headless. 585ms local run vs guaranteed 10s+ before.3-5. E2E wire-up, then pivot to native specs
tests/e2e/scenarios never ran in CI — the in-tree framework discovers*_spec.lua, not scenario tables. I initially wired a shell wrapper (#3, #4), then reconsidered and converted the 5 scenarios into native specs (#5), deletingscripts/nvim-e2e.lua(478 lines),tests/run_e2e.{sh,cmd}, themake test-e2etarget (both live Makefile and CMake template), and the CI step. Net -988 lines / +436 lines, single invocation path.6.
test: restructure spec tree to mirror lua/ layouttests/ui/scrollsync_spec.lua→tests/,tests/ui/keymap/→tests/keymap/,tests/flatten_dirs_spec.lua→tests/ui/explorer/,tests/ui/core_spec.lua→tests/ui/render_core_spec.lua.explorer_spec.lua(34s serial, two describes) →explorer_spec.lua(17 tests) +explorer_refresh_spec.lua(5 tests), parallelizable across framework workers.*_modules_spec.lua→ onetests/module_loading_spec.lua._e2e_specsuffix; renametab_cycle_untracked_e2e→issue_309_spec.luato match siblingissue_XXX_spec.luaconvention.7.
fix(explorer): skip auto-refresh ticks whose target repo is gone or not yet a repoReal behavior fix, discovered by reading CI stderr on Windows. The 500ms explorer polling
tick()could fire against a directory that had just been deleted (after_eachteardown race withTabClosedcleanup) or wasn't yet a git repo (setup race on slow filesystems::CodeDiffopens beforegit initfinishes).git statusreturned "not a git repository";process_resultsurfaced it asvim.notify(ERROR), flooding stderr in 8+ spec files.Root-cause fix: guard
tick()onisdirectory(git_root)and existence of.git(as directory OR file, so worktrees still work). Silently return in either race. User-visible:rm -rfon your own repo behind the explorer no longer flashes an ERROR notification.8.
test(explorer): tighten explorer_tree_render_spec ]f-navigation assertionDiscovered during the CI-log audit: my converted spec asserted only that the modified buffer still existed after
]f, which was trivially true because:CodeDiffopened with an initial selection. Verified with bug injection: withdo return endat the top ofnavigation.next_file, the old assertion still passed. Now usesnavigation.next_file()directly (avoiding anvim_feedkeysshadowing problem that produced theE447in the earlier CI runs), capturesexplorer.current_file_pathbefore, and asserts (a) it changes and (b) the modified pane's buffer name matches the newly-selected file.9.
chore: bump version to 2.66.0Verification
regression-checkjob fails on anyworkflow_dispatchtrigger with emptygithub.base_ref— pre-existing infrastructure issue, unrelated. Runs correctly onpull_requestevents.Compatibility
lua/change (ui/explorer/refresh.lua, +28 lines) — behavior improvement only (silent no-op instead of error notification for a legitimately-unpollable state). Cannot break any existing behavior.