Repository navigation
test(cli): synchronize reap tests with shell readiness - #2204
Merged
Merged
Conversation
Wait for a marker written after the HUP trap is installed, and cover one-second delayed setup. Skip setup waits for well-behaved children and the real PTY group, which exists before spawn returns. Closes #2174
CoverageTotal: 97.93% ⚪ 0 pp vs Comparing
🔇 269 ignored region(s), 0 tolerated region(s)
Patch coveragePatch: 100% (54/54 new lines covered)
|
Extract the readiness wait with an injectable deadline, and exercise its timeout without delaying the suite. Assert the panic diagnostic, SIGKILL, OS-level reaping, and removal of the process group.
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.
Description
The reap test helper previously signalled its shell after a fixed 100 ms setup sleep, which could arrive before
trap '' HUPwas installed. It now waits for a ready file written after the trap, with a 10-second diagnostic deadline and process-group cleanup on timeout. Well-behaved children skip that setup wait.Type of Change
Related Issue
Closes #2174
Implementation plan
Changes Made
All changes are in
src/cli/worktrees/ui/terminal/pty.rs, withinreap_tests:group_leaderaccepts an optional ready path, passed through a child-only environment variable so shell quoting handles temporary paths safely.setsid()before spawn returns.Production reaping and
REAP_GRACEare unchanged. This test-only change needs no user-facing docs or changelog entry.Testing
All checks were repeated after rebasing onto
origin/mainat83085b7cb:nohuplaunches.cargo insta test --test integration_test --no-quietpassed 64/64 with no snapshots to review. The integration/snapshot tests passed again in both rebased full suites; no.snap.newfiles remain.Some(SIGHUP)instead ofSome(SIGKILL)). A shortened-deadline harness also verified timeout diagnostics and group cleanup, including a temporary path containing spaces and a quote.Test Commands
A targeted cargo-llvm-cov run after the coverage follow-up measured 54/54 changed executable lines covered locally (100%), including every region of
wait_for_ready. Overall project coverage was not re-measured locally. Local platform validation was on macOS; Linux/Windows and beta/nightly checks are left to CI.Coverage follow-up
The reported five uncovered timeout lines are now exercised by
missing_readiness_kills_and_reaps_the_child_before_panicking.wait_for_readyaccepts the deadline for tests whilegroup_leaderretains its 10-second default. The test waits for the HUP trap, then uses an immediate deadline and a missing marker. It verifies the diagnostic, SIGKILL,waitpidreturning ECHILD beforetry_waitcan reap anything, and the group's disappearance. No coverage exclusions were added.Follow-up validation passed: formatting; all-target/all-feature Clippy; 7/7 focused tests in plain and nohup launches; 64/64 integration tests with no snapshot drift; instrumented focused tests; and strict commit lint.
cargo llvm-cov --manifest-path "$WT/Cargo.toml" --lib --json \ --output-path /private/tmp/issue-2174-coverage-final.json -- reap_testsReview Focus Areas
Review the ordering of trap installation and marker creation, and timeout cleanup. The committed diff and surrounding code were reviewed once against
origin/main...HEAD; there were no actionable findings, fixes, or skipped findings.Checklist
Performance Impact
Test setup drops the unnecessary 100 ms waits and the real-PTY test's 150 ms wait. The delayed regression adds one second of deliberate setup. No production performance change.
Security Considerations
No production security change. Ready paths are confined to fresh temporary directories and passed through the child environment.
Deployment Notes
No special deployment requirements or breaking changes.
Additional Notes
coverage lint-markersfrom the build script. That command passed before rebase and is no longer applicable to the rebased repository.