test(cli): clean up before failing the install-plan receipt fixture - #1775
Merged
Conversation
cli_install_plan_receipt_no_mutation_issue388 asserted inline, so any failure returned before free(json) and test_rmdir_r(tmpdir). A red run therefore leaked the receipt and left a stray /tmp/cli-plan-* directory behind -- the failure report was accompanied by exactly the noise that makes the next debugging session harder. Record what went wrong, release everything, then fail. The message now names the specific missing marker rather than reporting that some marker was absent, which is the difference between reading a failure and bisecting one. The env-isolation half of the original PR is not carried over: main's tf_setup_cache_sentinel already unsets CODEX_HOME along with every other client home override, so a per-test save/unset/restore would be redundant now. Distilled from #1149 by Anand Aiyer, which predates that sentinel and could not land as written once its companion PR closed unmerged. Co-authored-by: Anand Aiyer <anand@aiyer.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
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.
cli_install_plan_receipt_no_mutation_issue388asserted inline, so any failure returned beforefree(json)andtest_rmdir_r(tmpdir). A red run therefore leaked the receipt and left a stray/tmp/cli-plan-*directory behind — the failure report came with exactly the noise that makes the next debugging session harder than the failure itself.What changed
Record what went wrong, release everything, then fail:
free(json)andtest_rmdir_r(tmpdir)now run unconditionallyinstall plan receipt is missing next_safe_command) instead of reporting that some marker was absentThat last point is deliberate: folding the checks into one boolean would have kept the cleanup fix while losing which
strstrfailed. Naming the marker is the difference between reading a failure and bisecting one.What is deliberately not carried over
The original PR also saved, unset and restored
CODEX_HOMEaround the test. That is redundant on currentmain:tf_setup_cache_sentinelintests/test_main.calready unsetsCODEX_HOMEalong with every other entry intf_client_home_overrides[]at suite setup. Adding a per-test save/restore now would be dead code.Verification
build/c/test-runner cli— 281 passed, 0 failed, with the restructured test passing.git clang-formatclean against the Homebrew LLVM build.Credit
Distilled from #1149 by @aaiyer. That PR was correct when written, but its sequencing depended on the companion PR #1150 landing first — and #1150 was closed unmerged, so the plan it was waiting on no longer existed. The contributor also correctly diagnosed the 4,976-byte "leak" as an assertion aborting before the frees rather than a production leak, which is the harder and more useful call.