Repository navigation
test: fail the test when a shell setup command exits with a non-zero status - #570
Conversation
…status `assert(os.execute(...))` never fails under LuaJIT, which Neovim, busted and the test rocks all run on. It returns the raw status: 0 for success and 256 for exit status 1, and every number is truthy. So a mkdir, ln or touch that failed left the test to run on a setup that did not exist, and the test could pass for the wrong reason. lockfile_test.lua gets a local `shell` helper that asserts `status == 0 or status == true`, which holds on LuaJIT and on Lua 5.2 and later. It replaces nine assert(os.execute(...)) calls, two of them spread over three lines. temp_dir_spec.lua checks its one mkdir the same way. The other files that run shell commands already test the status that way.
|
@codex review |
|
@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. Keep them coming! 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". |
The "should not delete a folder that takes the place of the lock file after the check" test runs `chmod 000` and `chmod 600` on the lock file, with no check of the result. Unlike the other chmod calls in the file, it has no probe that marks the test pending when the mode did not change. So if chmod failed, the "mode 000" pass ran on a readable file, and the test stayed green. It now calls the file's `shell` helper, which fails the test and names the command. With chmod forced to fail, the file gave 33 successes before and 1 failure with `failed: false && chmod 000 ...` now.
🛡️ 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. |
|
@codex review |
|
@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. |
Why
assert(os.execute(...))never fails under LuaJIT. Neovim, busted and the test rocks all run on LuaJIT. Itsos.executereturns the raw status: 0 for success and 256 for exit status 1. Every number is truthy in Lua, so the assert passes whatever the command did. Amkdir,lnortouchthat failed left the test to run on a setup that did not exist, and the test could pass for the wrong reason. A verifier of #561 found this.I checked it on busted's LuaJIT (2.1.1744318430) and on Neovim 0.8.0 and 0.12.3 (both
Lua 5.1with LuaJIT):os.execute("exit 1")returns 256 andos.execute("true")returns 0 on each.What changed
tests/lockfile_test.luagets a localshell(command)helper that assertsstatus == 0 or status == true, and names the command when it fails. Theor status == truepart keeps it right on Lua 5.2 and later, whereos.executereturnstruefor success. It replaces nineassert(os.execute(...))calls, two of them spread over three lines.tests/unit/temp_dir_spec.luachecks its onemkdir -pthe same way, inline.chmodthat the folder-swap test runs ("should not delete a folder that takes the place of the lock file after the check",lockfile_test.lualine 669) now goes throughshelltoo. That test has no probe, so a failedchmod 000left its "mode 000" pass running on a readable file, and the test stayed green. Withchmodforced to fail, the file gave 33 successes and 0 failures before, and gives 1 failure (failed: false && chmod 000 ...) now. Normally it gives 33 successes.Only two files had the bug, so there is no new shared helper. Every other place in
tests/that reads anos.executeresult already tests== 0 or == true(diff_spec.lua,integration_runner_spec.lua,test_order_helper_spec.lua,tmpdir_suite_spec.luaand the status checks insidelockfile_test.lua).tests/unit/cwd_spec.luahas two more of the broken calls, from #561. They are not in this PR. A forced failure there still turns the test red, at a later line. They go to a tracking issue (F27) that this run files after it lands.Verification
I made each of the ten commands fail on purpose (
false && <command>) and ran its file onorigin/mainand on this branch. The full table is in the details below.lockfile_test.lua958 and 972 on main, which are 964 and 977 after the change (twoauthentication tokentests: a folder named like a lock file, and a link that points nowhere).failed: false && mkdir ...).temp_dir_spec.luasite passed its setup before, and three later tests failed attemp_dir_spec.lua:47. It now fails at once withcannot create ....Failure demonstration, each site on
origin/mainand on this branchEach site was made to fail by putting
false &&in front of its command. Each row runs the whole file once.lockfile_test.lua610 -> 618lockfile_test.lua670 -> 678lockfile_test.lua709 -> 716lockfile_test.lua721 -> 727lockfile_test.lua735 -> 741lockfile_test.lua747 -> 753lockfile_test.lua748 -> 754lockfile_test.lua958 -> 964lockfile_test.lua972 -> 977temp_dir_spec.lua15 -> 16Left as it is
Three other
chmod 000andchmod 555calls inlockfile_test.lua(lines 583, 633 and 904) have no check either, and I left them. Each is followed by a probe: if the file is still readable or the folder is still writable, the test returnspending("... (root)"). So achmodthat failed shows as a pending test in the run output, but the run stays green and the shown reason wrongly says "(root)". The same tracking issue (F27) covers these three calls and themkdir -pat line 416, together with movingshelltotests/helpers/. Thechmod 600andchmod 755calls only restore a mode in cleanup.lockfile_test.luaandintegration_runner_spec.luanow each hold a localshellhelper. A change that fixestests/unit/cwd_spec.luacan move it totests/helpers/.Gates on
c71fe8e9, which mergesorigin/main48ca93b into the branch (private XDG folders). A first round of Codex review on55bc57befound nothing. The chmod commit and the merge came after it:mise run all: native exit 0. luacheck: 0 warnings / 0 errors in 190 files. busted: 1832 successes / 0 failures / 0 errors / 30 pending.mise run format-check: native exit 0 (432 files, 0 changed).check.sh --hash-onlymatches on all four skills..scripts/run_integration_tests_individually.shon Neovim 0.12.3: 600 successes, 0 failed, 0 errors, native exit 0. On Neovim 0.8.0: 582 successes, 0 failed, 0 errors, native exit 0.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh