fix(tui): gate color on the CLI entry point, not cfg!(test) - #1765
Conversation
`colors_enabled()` opened with `if cfg!(test) { return false }`, whose
comment claims tests "never depend on the dev's TTY". It cannot do
that: `cfg!(test)` is false whenever mergify-tui is compiled as a
dependency, so the guard only ever covered mergify-tui's own tests.
Every *consumer* crate's tests fell through to the real policy and read
the developer's environment — `FORCE_COLOR=1 cargo test -p
mergify-events` fails on main today, on escape sequences injected into
asserted output.
Gate on the recorded `--color` choice instead. `set_color_choice()` is
called from exactly one place, `detect_dispatch()` in main, so an unset
choice means the process did not come through the CLI: a test harness,
a doctest, an embedder. Colors stay off there, in any environment,
across every crate.
`resolve_enabled()` takes the `Option<ColorChoice>` so the new case is
covered by the existing precedence unit test rather than by global
state.
Refs MRGFY-8533.
Change-Id: Id7eb12b906409122000a14722e5ce172ab93cb4c
|
This pull request is part of a Mergify stack:
|
Merge Protections🟢 All 6 merge protections satisfied — ready to merge. Show 6 satisfied protections🟢 🤖 Continuous Integration
🟢 👀 Review Requirements
🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 🔎 Reviews
🟢 📕 PR description
🟢 🚦 Auto-queueWhen all merge protections are satisfied, this pull request will be queued automatically. |
There was a problem hiding this comment.
Pull request overview
This PR fixes color-detection in mergify-tui by removing an ineffective cfg!(test) gate (which only applied to mergify-tui’s own unit tests) and instead disabling colors unless the CLI entry point explicitly records a --color preference via set_color_choice().
Changes:
- Make color enablement depend on whether
set_color_choice()has been called (i.e., whether execution came through the CLI entry point). - Refactor
resolve_enabledto acceptOption<ColorChoice>so the “no recorded choice” case is explicitly modeled and unit-tested. - Update documentation/comments and expand unit test coverage for the new precedence behavior.
Suppressed comments (1)
crates/mergify-tui/src/theme.rs:165
colors_enabled()already treats an unsetCOLOR_CHOICEas “colors off”, but it still readsNO_COLOR/FORCE_COLORand probesstdout().is_terminal()even though those values can’t affect the result whenchoiceisNone. Returning early when the choice is unset better matches the stated policy (“must not take a dependency on the developer's terminal or environment”) and avoids unnecessary syscalls in test/embedded contexts.
pub(crate) fn colors_enabled() -> bool {
// An unset choice means we are not the CLI: colors off. This
// used to be `cfg!(test)`, which cannot do the job — it is false
// whenever this crate is compiled as a dependency, so every
// *consumer* crate's tests were reading the developer's
// environment after all. `FORCE_COLOR=1 cargo test` failed on the
// escape sequences that leaked into asserted output.
let choice = COLOR_CHOICE.get().copied();
let no_color = std::env::var_os("NO_COLOR").is_some();
let force_color =
std::env::var_os("FORCE_COLOR").is_some() || std::env::var_os("CLICOLOR_FORCE").is_some();
resolve_enabled(
choice,
no_color,
force_color,
std::io::stdout().is_terminal(),
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merge Queue Status
This pull request spent 6 minutes 39 seconds in the queue, including 6 minutes 16 seconds running CI. Required conditions to merge
|
colors_enabled()opened withif cfg!(test) { return false }, whosecomment claims tests "never depend on the dev's TTY". It cannot do
that:
cfg!(test)is false whenever mergify-tui is compiled as adependency, so the guard only ever covered mergify-tui's own tests.
Every consumer crate's tests fell through to the real policy and read
the developer's environment —
FORCE_COLOR=1 cargo test -p mergify-eventsfails on main today, on escape sequences injected intoasserted output.
Gate on the recorded
--colorchoice instead.set_color_choice()iscalled from exactly one place,
detect_dispatch()in main, so an unsetchoice means the process did not come through the CLI: a test harness,
a doctest, an embedder. Colors stay off there, in any environment,
across every crate.
resolve_enabled()takes theOption<ColorChoice>so the new case iscovered by the existing precedence unit test rather than by global
state.
Refs MRGFY-8533.