test(settings): handle TERM_PROGRAM env var in no_animations_env_reco…#2171
Merged
Conversation
…gnises_truthy_spellings_only Clear and restore TERM_PROGRAM environment variable during tests to prevent low_motion being forced in vscode, ghostty and Termius.
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the TUI settings tests to save, clear, and restore the TERM_PROGRAM environment variable, preventing it from interfering with NO_ANIMATIONS tests in environments like VS Code, Ghostty, or Termius. The reviewer suggests extending this logic to also handle other environment variables that can independently force low_motion (such as SSH_CLIENT, SSH_TTY, TILIX_ID, and TERMINATOR_UUID) to ensure test reliability across different environments.
…ntly force low_motion in no_animations_env_recognises_truthy_spellings_only
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.
…gnises_truthy_spellings_only
Summary
Clear and restore TERM_PROGRAM environment variable during tests to prevent low_motion being forced in vscode, ghostty and Termius.
Problem
The test "codewhale_tui::settings::tests::no_animations_env_recognises_truthy_spellings_only" fails on some terminals, such as vscode, ghostty and Termius.
Root Cause
When TERM_PROGRAM is set to vscode, ghostty, or Termius, low_motion is enforced, causing the test to fail, and TERM_PROGRAM is not cleared before the test.
Fix
Additional Notes
The Cargo Clippy check has completed. These changes did not introduce any new warnings. However, some existing warnings were detected.
Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-featurescargo test --workspace --all-featuresChecklist
Greptile Summary
This PR fixes test failures in
no_animations_env_recognises_truthy_spellings_onlythat occurred in terminals such as VS Code, Ghostty, and Termius, where environment variables (TERM_PROGRAM,SSH_CLIENT,SSH_TTY,TILIX_ID,TERMINATOR_UUID) independently triggerlow_motion, causing false assertion failures.TMUX/STYhandling pattern.WT_SESSION,TMUX,STY).Confidence Score: 5/5
Safe to merge — the change is narrowly scoped to one test function and correctly covers all env vars that apply_env_overrides checks for low_motion.
The fix is complete: every environment variable that apply_env_overrides reads to set low_motion (TERM_PROGRAM, SSH_CLIENT, SSH_TTY, TILIX_ID, TERMINATOR_UUID, plus the pre-existing TMUX/STY) is now saved, cleared, and restored around the assertion loops. The implementation follows the established pattern in the test exactly, no production code is touched, and the pre-existing panic-safety concern is unchanged and well-understood.
No files require special attention — only one test function in crates/tui/src/settings.rs was modified.
Important Files Changed
Sequence Diagram
sequenceDiagram participant T as Test participant E as Environment participant S as Settings::apply_env_overrides T->>E: save TMUX, STY, TERM_PROGRAM, SSH_CLIENT, SSH_TTY, TILIX_ID, TERMINATOR_UUID T->>E: remove_var(all above) loop truthy spellings ["1","true","YES","on","True"] T->>E: set_var(NO_ANIMATIONS, truthy) T->>S: apply_env_overrides() S-->>T: "low_motion = true ✓" end loop falsy spellings ["0","false","no","off",""] T->>E: set_var(NO_ANIMATIONS, falsy) T->>S: apply_env_overrides() S-->>T: "low_motion = false ✓" end T->>E: remove_var(NO_ANIMATIONS) T->>E: restore all saved varsComments Outside Diff (1)
crates/tui/src/settings.rs, line 1356-1375 (link)If any
assert!inside the truthy/falsy loops fires, the function unwinds before reaching the cleanup block, leavingTERM_PROGRAM(andTMUX,STY,NO_ANIMATIONS) removed from the environment permanently for the rest of the test process. TheMutexGuard_gis dropped correctly via unwind, but that only releases the lock — it does not restore the env vars.This is a pre-existing issue that affects all the env vars managed in this test. The
TERM_PROGRAMaddition is consistent with the existing pattern, but none of them are panic-safe. Wrapping the state in a customDropguard (one struct that holds all theprev_*values and restores them on drop) would make the entire group panic-safe and could replace the manual cleanup block.Reviews (2): Last reviewed commit: "test(settings): handle other environment..." | Re-trigger Greptile