Make the test suite environment-independent, fix UTF-8 redirection, and a UI-call race - #1752
Merged
Merged
Conversation
Rich and cmd2 consult several environment variables when deciding whether to emit styling, and the suite inherited them. Exporting any one of them made large numbers of unrelated tests fail depending on who ran the suite: NO_COLOR failed 15 tests, and FORCE_COLOR and TTY_COMPATIBLE 53 each. The failures look like product regressions, which makes them expensive to diagnose. Neutralize them for every test. Tests that exercise these variables set them explicitly, which still works because a test's own monkeypatching runs after the fixture. A guard test fails if any of them reaches a test again.
Command output is rendered by Rich and routinely contains non-ASCII, but redirection targets and pipes were opened with the locale's encoding. On any system whose default is not UTF-8 -- a Windows console using a legacy code page, for instance -- redirecting output raised UnicodeEncodeError, and the user was left with an empty file and advice to set PYTHONIOENCODING. Open both with UTF-8 explicitly. Two tests that read redirected output back were relying on the locale encoding for decoding as well, so they now name it too.
_call_in_ui() polls the pending future with a 0.1s timeout and, on expiry, re-raises when the future is already done. That branch exists because concurrent.futures.TimeoutError is TimeoutError on Python 3.11+, so a callback raising a timeout of its own cannot be told apart by type from the poll expiring. Re-raising the caught exception conflates the two. When the callback completes in the window between the poll expiring and the future being inspected, the caller is told the call timed out even though it succeeded. Ask the future for its outcome instead: a callback that raised a timeout still propagates it, and one that produced a value now returns it. Found while investigating an intermittent failure of test_command_toolbar_ui_call_propagates_failures, which reproduced once in 60 runs before this change and not once in 120 after. The added regression test drives the interleaving deterministically rather than relying on timing. (cherry picked from commit 84bc19e)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## consolidated_toolbar #1752 +/- ##
=====================================================
Coverage 99.67% 99.67%
=====================================================
Files 25 25
Lines 6464 6464
=====================================================
Hits 6443 6443
Misses 21 21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
The test piped through `cat`, which cmd.exe does not provide. On a Windows system without Unix utilities installed it would fail before reaching the encoding behavior it exists to check -- and Windows is exactly what the UTF-8 redirection fix targets. Use a sys.executable pass-through instead, matching the pipe tests already in tests/test_command_toolbar.py. Reverting either the pipe or the redirect encoding still fails these tests.
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.
Three fixes, all pre-existing on
consolidated_toolbarand none specific to thereserved-row toolbar work. Found while investigating a reviewer's report of failures in
unchanged tests.
1. The suite inherited the caller's colour environment
Rich and cmd2 both consult environment variables when deciding whether to emit styling, and
the suite inherited them. Exporting any one made large numbers of unrelated tests fail
depending on who ran it:
NO_COLOR=1FORCE_COLOR=1TTY_COMPATIBLE=1The failures look like product regressions, which makes them expensive to diagnose — this
cost a reviewer and me a full round trip. An autouse fixture now neutralizes them, and a
guard test fails if any reaches a test again. Tests that exercise these variables still set
them explicitly, because a test's own monkeypatching runs after the fixture.
2. Redirection and piping failed on non-UTF-8 systems (user-visible)
Command output is rendered by Rich and routinely contains non-ASCII, but redirection targets
and pipes were opened with the locale's encoding. On a system whose default is not UTF-8 —
a Windows console using a legacy code page —
help > out.txtraisedUnicodeEncodeErrorand left the user an empty file plus advice to set
PYTHONIOENCODING.Both now use UTF-8 explicitly. Two tests that read redirected output back were relying on the
locale for decoding as well, so they name it too. CHANGELOG updated.
3. A race reported a timeout for a successful UI call (back-port)
Cherry-picked from the reserved-row branch, where it was found; it is not specific to that
work.
CommandToolbar._call_in_ui()polls the pending future with a 0.1s timeout andre-raises when the future is already done. That branch exists because
concurrent.futures.TimeoutErrorisTimeoutErroron Python 3.11+, so a callbackraising a timeout of its own cannot be told apart by type from the poll expiring.
Re-raising the caught exception conflates the two: when the callback completes in the window
between the poll expiring and the future being inspected, the caller is told the call timed
out although it succeeded. It now asks the future for its outcome instead.
test_command_toolbar_ui_call_propagates_failuresfailed once in 40 runs on this branchbefore the fix and not once in 60 after. The added regression test drives the interleaving
deterministically rather than relying on timing.
Verification
Full suite under every environment that previously broke it, three runs of the combined case:
make check,make testandmake docs-testall pass.