test: pin Rich colour and width so CLI-output tests stop failing outside CI - #1332
Open
saltas888 wants to merge 3 commits into
Open
test: pin Rich colour and width so CLI-output tests stop failing outside CI#1332saltas888 wants to merge 3 commits into
saltas888 wants to merge 3 commits into
Conversation
Tests that assert on CLI text let Rich decide colour and width from the ambient environment, so a developer whose shell exports FORCE_COLOR got ANSI escapes and truncated Rich tables in captured output. 37 tests across test_marketplace_app.py, test_repository_app.py, test_task_app.py and test_schema.py failed locally while staying green in CI, which trains contributors to ignore red test output. Pin the rendering environment from pytest_configure in tests/conftest.py: unset FORCE_COLOR, set NO_COLOR=1 and COLUMNS=200. The hook has to run there rather than in a fixture: Rich snapshots no_color when a Console is constructed, and many infrahub_sdk.ctl modules build a module-level Console() during collection, before any fixture runs. Rich also treats any FORCE_COLOR value, empty string included, as proof it is writing to a terminal, so the variable has to be removed rather than blanked. TERM is deliberately left alone. TERM=dumb sends Rich down its dumb-terminal path, which pins the width to 80 and ignores COLUMNS, which would truncate the wide tables the CLI-output fixtures record. One central hook covers all 24 CliRunner call sites, so no per-test env plumbing is needed. The three consoles that tests/unit/sdk/test_schema.py builds itself are made explicit with no_color=True and force_terminal=False so they do not depend on the environment at all. The suite is now green under FORCE_COLOR=1 COLUMNS=40, NO_COLOR=1, TERM=dumb and a bare environment alike. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deploying infrahub-sdk-python with
|
| Latest commit: |
caada89
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1ea6d260.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pha-inbox-166.infrahub-sdk-python.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## develop #1332 +/- ##
===========================================
- Coverage 85.54% 84.24% -1.31%
===========================================
Files 148 147 -1
Lines 14271 13047 -1224
Branches 1953 1930 -23
===========================================
- Hits 12208 10991 -1217
+ Misses 1496 1493 -3
+ Partials 567 563 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review on #1332 asked whether TERM=dumb defeats the COLUMNS=200 pin, since pytest_configure leaves TERM alone. It does not, but nothing in the suite said so, so the question was fair. Rich clamps to width 80 and ignores COLUMNS only on a dumb terminal, and is_dumb_terminal is `is_terminal and TERM in ("dumb", "unknown")`. Captured test output is never a terminal, and the hook removes the one variable that would make Rich claim otherwise -- FORCE_COLOR, which Rich reads as proof of a terminal for any value. So removing FORCE_COLOR is what defuses the dumb path; pinning TERM would be a no-op. Add tests/unit/test_render_env.py to hold that: the hook's env is applied, the width survives TERM in dumb/unknown/xterm-256color/screen/empty, and the one combination that would clamp to 80 (FORCE_COLOR set with TERM=dumb) is pinned as the failure mode the hook exists to prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The regression test pinned the FORCE_COLOR + TERM=dumb clamp with
FORCE_COLOR=1 only, while the invariant it exists to document is that Rich
tests `FORCE_COLOR is not None` -- so `export FORCE_COLOR=` forces a
terminal exactly as `1` does. That empty form is the case that makes
removing the variable the only correct fix rather than overriding it with
a falsy value, and it was the one form the test never exercised.
Parameterize over "1", "3" and "" ("3" being what a real shell exports).
All three clamp to width 80, so the documented claim is now pinned instead
of asserted in a comment.
Raised by cubic-dev-ai on PR #1332.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
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.
Why
Tests that assert on CLI text compare against output whose colour and width Rich decides from the ambient environment. On a developer machine that exports
FORCE_COLOR(or has a narrow terminal), Rich emits ANSI escapes and truncates its tables, so a large block of tests fails locally while staying green in CI — which trains contributors to explain away red test output.This was reported independently by two contributors in the same week, in their PR descriptions: #1227 recorded 3 pre-existing failures in
tests/unit/sdk/test_schema.pythat "assert on plain strings while Rich emits ANSI colour codes in this environment", and #1233 recorded further unrelated-to-that-PR failures onstable.Goal: make the suite's rendering environment deterministic, so CLI-output assertions give the same answer everywhere.
Non-goals: no production code changes, and no loosening of any assertion — the exact-output tests keep their value.
Closes INBOX-166
What changed
tests/conftest.py— apytest_configurehook pins the rendering environment for the whole suite: unsetFORCE_COLOR, setNO_COLOR=1andCOLUMNS=200.tests/unit/sdk/test_schema.py— the three consoles the tests build themselves are now explicit:Console(file=StringIO(), width=1000, no_color=True, force_terminal=False), so they do not depend on the environment at all.tests/unit/test_render_env.py(new) — pins the pinning: that the hook ran before this module was imported, that the width survives anyTERM, and thatFORCE_COLORis what would actually clamp it. A future edit that loosens the hook fails here with an explanation, instead of as a scatter of puzzling output-comparison failures across the ctl tests.tests/AGENTS.md— documents the invariant and the two traps below, so the next contributor does not rediscover them.Implementation notes
Three details drove the design, all verified against
rich13.9.4 /click8.3.3 rather than assumed:It has to be
pytest_configure, not a fixture. Rich snapshotsno_colorinConsole.__init__, and manyinfrahub_sdk.ctlmodules build a module-levelConsole()at import time — which happens while pytest collects the test modules, before any fixture can run. A session-scoped autouse fixture is already too late for those consoles.FORCE_COLORmust be removed, not blanked. Rich treats anyFORCE_COLORvalue as proof it is writing to a terminal, empty string included (if force_color is not None: self._force_terminal = True).TERM=dumbis deliberately not set. It looks like the obvious way to disable colour, but it is the trigger for Rich's dumb-terminal path, which pins the width to 80 and ignoresCOLUMNS— truncating the very wide tables the CLI-output fixtures record (e.g. therepository_listfixture is 139 columns).To be precise about the mechanism, since it was raised in review:
TERM=dumbalone is harmless here, becauseis_dumb_terminalisis_terminal and TERM in ("dumb", "unknown")and captured test output is never a terminal. It is removingFORCE_COLOR(point 2) that keepsis_terminalFalseand so defuses the dumb path. PinningTERMto a non-dumb value would therefore be a no-op for width while implying a knob that does nothing — whereas adoptingTERM=dumb, as the card originally suggested, is what creates the failure if anything ever does force a terminal.test_render_env.pypins both halves of this.One central hook covers all 24
CliRunner()call sites, so no per-test env plumbing was needed — passingenv=to each runner would have been 24 edits that drift, and (per point 1) would not have fixed the module-level consoles anyway.What stayed the same
No production code touched — the diff is
tests/plus a changelog fragment. No API, schema, dependency, or CI-workflow changes.How to review
tests/conftest.pyis the whole change; the rest follows from it. The comment block there records the three findings above.How to test
The card's own acceptance gate, plus the environments that reproduce the original reports:
Before (on
develop, withFORCE_COLOR=3exported — the reported dev-machine case): 37 failed, 1781 passed, spread acrosstests/unit/ctl/test_marketplace_app.py(31),tests/unit/sdk/test_schema.py(3),tests/unit/ctl/test_repository_app.py(2) andtests/unit/ctl/test_task_app.py(1).After: 1827 passed, 1 xfailed — identical under all six environments above.
ruff check,ruff format --check,ty,mypy(158 files) andrumdl(129 files) are all clean;invoke lintreports onlyvale is not installedlocally.Two notes for the reviewer:
test_gitrepo_initintests/unit/sdk/test_repository.py. It does not fail on currentdevelopin any of the environments tested, so nothing was changed for it.test_marketplace_app.py's 31 failures were not in the card's list but share the exact same root cause and are fixed by the same hook.Impact & rollout
NO_COLOR/COLUMNSand unsetsFORCE_COLORfor itself. A side effect is that pytest's own report renders at 200 columns.Checklist
tests/AGENTS.md)Filed from Engineering Inbox card INBOX-166 by the platform-health agent. Opened for human review — the agent does not merge.
🤖 Generated with Claude Code
Summary by cubic
Pins Rich's colour and width for the test suite so CLI-output assertions behave the same everywhere, fixing 37 tests that failed on developer machines but passed in CI. Closes INBOX-166.
tests/conftest.pynow unsetsFORCE_COLORand setsNO_COLOR=1andCOLUMNS=200frompytest_configurebefore any test module is imported.tests/unit/sdk/test_schema.pyare constructed withno_color=Trueandforce_terminal=Falseso they don't depend on the environment.tests/unit/test_render_env.pyguards the pinning: the env is applied, the width survives anyTERM, andFORCE_COLORset withTERM=dumbis pinned as the failure mode the hook prevents, for every value including the empty string.Written for commit caada89. Summary will update on new commits.