From 498c473e91eda56e496627612994209a5dcd01a1 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Wed, 29 Apr 2026 14:27:55 +0200 Subject: [PATCH 1/5] feat(io): tput-based capability probe and centralized color escapes Adopt a hybrid tput + ANSI strategy per ADR-006: keep ANSI as the primary emission mechanism via the existing bashunit::sgr helper, and use tput for terminal capability probing and screen clearing. - Add bashunit::env::supports_color: false on TERM=dumb or when tput colors reports fewer than 8. - Wire supports_color into colors.sh init so colors auto-disable when the terminal cannot render them, complementing --no-color / NO_COLOR. - Add bashunit::io::clear_screen using tput clear with an ANSI fallback; replace the hardcoded \033[2J\033[H sequence in --watch mode. - Replace hardcoded \033[...m escapes in coverage reporting with the central _BASHUNIT_COLOR_* constants. Closes #247 --- CHANGELOG.md | 3 + adrs/adr-006-tput-vs-ansi-escapes.md | 83 ++++++++++++++++++++++++++++ src/colors.sh | 2 +- src/coverage.sh | 41 +++++--------- src/env.sh | 26 +++++++++ src/io.sh | 13 +++++ src/main.sh | 2 +- tests/unit/env_test.sh | 41 ++++++++++++++ tests/unit/io_test.sh | 17 ++++++ 9 files changed, 200 insertions(+), 28 deletions(-) create mode 100644 adrs/adr-006-tput-vs-ansi-escapes.md create mode 100644 tests/unit/io_test.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 2456d7f9..3cfac1c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,11 @@ ### Added - Display captured test output on assertion failures when `--show-output` is enabled (#637) +- Auto-disable colored output when the terminal cannot render it: `TERM=dumb` or `tput colors` reporting fewer than 8 now suppress color the same way `--no-color` / `NO_COLOR` does (#247) +- `bashunit::env::supports_color` helper exposing the capability probe; `bashunit::io::clear_screen` helper that prefers `tput clear` and falls back to the raw ANSI sequence (#247) ### Changed +- Centralize all ANSI escape emission through the existing `_BASHUNIT_COLOR_*` constants. `src/coverage.sh` and the `--watch` screen-clear in `src/main.sh` no longer hardcode escape sequences (#247) - Speed up coverage report generation by collapsing the per-line non-executable pattern checks in `bashunit::coverage::is_executable_line` into a single combined `grep` invocation (#636) - Speed up coverage report generation further by combining executable + hit counting into a single source-file pass (`bashunit::coverage::compute_file_coverage`) shared across text/lcov/html reporters, removing per-line `get_line_hits` scans of the coverage data file (#636) - Replace `echo | sed` / `echo | grep` subshells in `bashunit::coverage::extract_functions` with bash native regex matching and parameter expansion (#636) diff --git a/adrs/adr-006-tput-vs-ansi-escapes.md b/adrs/adr-006-tput-vs-ansi-escapes.md new file mode 100644 index 00000000..7d04385a --- /dev/null +++ b/adrs/adr-006-tput-vs-ansi-escapes.md @@ -0,0 +1,83 @@ +# Hybrid tput + ANSI for terminal output + +* Status: accepted +* Deciders: @Chemaclass +* Date: 2026-04-29 + +Technical Story: [#247](https://github.com/TypedDevs/bashunit/issues/247) — evaluate replacing hardcoded ANSI escape sequences with `tput` (terminfo). + +## Context and Problem Statement + +bashunit emits colored output and screen-clear sequences via hardcoded ANSI escapes (`\e[31m`, `\033[2J\033[H`). `tput` queries the `terminfo` database, adapting to terminal capabilities and providing safer, more portable codes. A previous attempt to fully migrate to `tput` (around PR #245) caused widespread test instability across CI environments, so any move toward `tput` must be incremental and reliable. + +Should bashunit replace ANSI escapes with `tput`? + +## Decision Drivers + +* Bash 3.0+ portability (macOS, Linux, BSD, Windows runners) +* Reliability across CI matrices (GitHub Actions, dumb terminals, non-TTY pipelines) +* Minimal behavioral churn — tests and snapshots must keep passing +* Single source of truth for color/control sequences +* Auto-disable color when terminal does not support it (avoid garbled output) + +## Considered Options + +* A. Full migration: replace every ANSI escape with `tput` +* B. Status quo: keep ANSI everywhere +* C. Hybrid: keep ANSI as primary mechanism via centralized helper, adopt `tput` for capability probing and select control sequences (e.g. screen clear) + +## Decision Outcome + +Chosen option: **C. Hybrid**. + +Rationale: ANSI SGR codes are stable and identical to what `tput setaf` emits on color terminals, so a wholesale `tput` rewrite buys little while introducing the failure modes that broke the previous attempt (e.g. `tput` returning empty strings under `TERM=dumb`, missing `terminfo` entries on stripped-down CI images, subprocess overhead per call). The real wins from `tput` are (1) capability probing (`tput colors`) to auto-disable color when unsupported and (2) portability for non-color sequences like screen clear. We adopt those targeted uses while keeping the existing centralized `bashunit::sgr` helper as the only emitter of color escape sequences. + +Concretely: + +1. Keep `bashunit::sgr` and `_BASHUNIT_COLOR_*` constants as the only place that emits color sequences. All ad-hoc `\033[...m` literals in `src/` are migrated to these constants. +2. Add `bashunit::env::supports_color` that returns false when `TERM=dumb`, `NO_COLOR` is set, or `tput colors` reports fewer than 8 colors. Wire this into `colors.sh` so colors auto-disable. +3. Replace the hardcoded `printf '\033[2J\033[H'` screen clear with `tput clear` when available, falling back to the ANSI sequence otherwise. +4. `tput` is already used for `tput cols` in `src/env.sh:215-219` — that pattern (probe + ANSI fallback) is the model. + +### Positive Consequences + +* One place (`bashunit::sgr` + `_BASHUNIT_COLOR_*`) to change colors. +* Color auto-disables on dumb terminals and non-TTY pipelines without requiring `--no-color`. +* Screen clear works on terminals where the hardcoded sequence is wrong. +* No subprocess explosion: `tput` is invoked once at init for capability probing, not per emitted color. +* Avoids the failure mode from the previous attempt (per-call `tput setaf` returning empty strings under unusual `TERM` values). + +### Negative Consequences + +* Slight init-time cost for the `tput colors` probe. +* Test suite must mock `tput` in scenarios that depended on guaranteed-color output. Existing tests already mock `tput` for `find_terminal_width` (see `tests/unit/env_test.sh:144`), so the pattern is established. + +## Pros and Cons of the Options + +### A. Full migration + +* Good, because terminfo is the canonical Unix way to handle terminals. +* Good, because `tput` adapts to capability quirks (e.g. 8 vs 256 colors). +* Bad, because the previous attempt destabilized CI tests across environments. +* Bad, because every color emission becomes a subprocess call (`$(tput setaf 1)`), measurable overhead in tight loops like the runner output. +* Bad, because `terminfo` databases on minimal CI images may lack capabilities, returning empty strings and silently breaking output. + +### B. Status quo + +* Good, because it is known to work across the entire current CI matrix. +* Bad, because color does not auto-disable on dumb terminals. +* Bad, because ad-hoc `\033[...m` literals in `coverage.sh` and `main.sh` bypass the centralized helper. + +### C. Hybrid (chosen) + +* Good, because it gets the practical benefits of `tput` (capability probing, portable control sequences) without the per-emission failure modes. +* Good, because it consolidates color emission through one helper, simplifying future changes (themes, 256-color, truecolor). +* Good, because it aligns with the existing `tput cols` pattern in `env.sh`. +* Bad, because two mechanisms coexist; contributors must know to use the constants, not raw escapes. Mitigated by lint/grep rules and code review. + +## Links + +* [Issue #247](https://github.com/TypedDevs/bashunit/issues/247) +* [PR #245 — Increase contrast of test results](https://github.com/TypedDevs/bashunit/pull/245) +* [terminfo(5) man page](https://man7.org/linux/man-pages/man5/terminfo.5.html) +* [NO_COLOR specification](https://no-color.org/) diff --git a/src/colors.sh b/src/colors.sh index 09765fd7..681c0823 100644 --- a/src/colors.sh +++ b/src/colors.sh @@ -18,7 +18,7 @@ bashunit::sgr() { echo $'\e'"[${codes}m" } -if bashunit::env::is_no_color_enabled; then +if bashunit::env::is_no_color_enabled || ! bashunit::env::supports_color; then _BASHUNIT_COLOR_BOLD="" _BASHUNIT_COLOR_FAINT="" _BASHUNIT_COLOR_BLACK="" diff --git a/src/coverage.sh b/src/coverage.sh index 1d6f7045..f15c77f2 100644 --- a/src/coverage.sh +++ b/src/coverage.sh @@ -737,16 +737,14 @@ function bashunit::coverage::report_text() { total_executable=$((total_executable + executable)) total_hit=$((total_hit + hit)) - # Determine color based on class - local color="" reset="" - if [ "${BASHUNIT_NO_COLOR:-false}" != "true" ]; then - reset=$'\033[0m' - case "$class" in - high) color=$'\033[32m' ;; # Green - medium) color=$'\033[33m' ;; # Yellow - low) color=$'\033[31m' ;; # Red - esac - fi + # Determine color based on class. Constants are empty when --no-color + # or NO_COLOR is set (see src/colors.sh), so no extra guard is needed. + local color="" reset="$_BASHUNIT_COLOR_DEFAULT" + case "$class" in + high) color="$_BASHUNIT_COLOR_PASSED" ;; + medium) color="$_BASHUNIT_COLOR_SKIPPED" ;; + low) color="$_BASHUNIT_COLOR_FAILED" ;; + esac # Display relative path local display_file="${file#"$(pwd)"/}" @@ -767,15 +765,12 @@ function bashunit::coverage::report_text() { total_pct=$(bashunit::coverage::calculate_percentage "$total_hit" "$total_executable") total_class=$(bashunit::coverage::get_coverage_class "$total_pct") - local color="" reset="" - if [ "${BASHUNIT_NO_COLOR:-false}" != "true" ]; then - reset=$'\033[0m' - case "$total_class" in - high) color=$'\033[32m' ;; - medium) color=$'\033[33m' ;; - low) color=$'\033[31m' ;; - esac - fi + local color="" reset="$_BASHUNIT_COLOR_DEFAULT" + case "$total_class" in + high) color="$_BASHUNIT_COLOR_PASSED" ;; + medium) color="$_BASHUNIT_COLOR_SKIPPED" ;; + low) color="$_BASHUNIT_COLOR_FAILED" ;; + esac printf "%sTotal: %d/%d (%d%%)%s\n" \ "$color" "$total_hit" "$total_executable" "$total_pct" "$reset" @@ -839,14 +834,8 @@ function bashunit::coverage::check_threshold() { pct=$(bashunit::coverage::get_percentage) if [ "$pct" -lt "$BASHUNIT_COVERAGE_MIN" ]; then - local color="" - local reset="" - if [ "${BASHUNIT_NO_COLOR:-false}" != "true" ]; then - color=$'\033[31m' - reset=$'\033[0m' - fi printf "%sCoverage %d%% is below minimum %d%%%s\n" \ - "$color" "$pct" "$BASHUNIT_COVERAGE_MIN" "$reset" + "$_BASHUNIT_COLOR_FAILED" "$pct" "$BASHUNIT_COVERAGE_MIN" "$_BASHUNIT_COLOR_DEFAULT" return 1 fi diff --git a/src/env.sh b/src/env.sh index 2a809131..43c3db26 100644 --- a/src/env.sh +++ b/src/env.sh @@ -182,6 +182,32 @@ function bashunit::env::is_no_color_enabled() { [ "$BASHUNIT_NO_COLOR" = "true" ] } +## +# Whether the current terminal can render ANSI color sequences. +# Returns 1 when TERM=dumb or when `tput colors` reports fewer than 8. +# Returns 0 when tput is missing (assume colors work — preserves prior behavior). +## +function bashunit::env::supports_color() { + if [ "${TERM:-}" = "dumb" ]; then + return 1 + fi + + if ! command -v tput >/dev/null 2>&1; then + return 0 + fi + + local n + n=$(tput colors 2>/dev/null) + case "$n" in + '' | *[!0-9]*) + return 0 + ;; + *) + [ "$n" -ge 8 ] + ;; + esac +} + function bashunit::env::is_coverage_enabled() { [ "$BASHUNIT_COVERAGE" = "true" ] } diff --git a/src/io.sh b/src/io.sh index bfe9d29e..b687d6a9 100644 --- a/src/io.sh +++ b/src/io.sh @@ -1,5 +1,18 @@ #!/usr/bin/env bash +## +# Clear the terminal screen and move the cursor to the home position. +# Uses `tput clear` when available (queries terminfo for the right sequence) +# and falls back to the ANSI sequence \033[2J\033[H otherwise. +## +function bashunit::io::clear_screen() { + if command -v tput >/dev/null 2>&1; then + tput clear + else + printf '\033[2J\033[H' + fi +} + function bashunit::io::download_to() { local url="$1" local output="$2" diff --git a/src/main.sh b/src/main.sh index d33d8335..b805511d 100644 --- a/src/main.sh +++ b/src/main.sh @@ -586,7 +586,7 @@ function bashunit::main::watch_loop() { if [ "$current_checksum" != "$last_checksum" ]; then last_checksum="$current_checksum" - printf '\033[2J\033[H' + bashunit::io::clear_screen printf "%s[watch] Running tests...%s\n\n" \ "${_BASHUNIT_COLOR_SKIPPED}" \ "${_BASHUNIT_COLOR_DEFAULT}" diff --git a/tests/unit/env_test.sh b/tests/unit/env_test.sh index b78e93ca..e68c1539 100644 --- a/tests/unit/env_test.sh +++ b/tests/unit/env_test.sh @@ -150,6 +150,47 @@ function test_find_terminal_width_fallback_returns_100() { assert_equals "100" "$result" } +function test_supports_color_returns_failure_when_TERM_is_dumb() { + local original_term="${TERM:-}" + export TERM="dumb" + + if bashunit::env::supports_color; then + export TERM="$original_term" + fail "Expected supports_color to fail when TERM=dumb" + return + fi + + export TERM="$original_term" + assert_successful_code 0 +} + +function test_supports_color_returns_failure_when_tput_reports_below_8_colors() { + local original_term="${TERM:-}" + export TERM="xterm" + bashunit::mock tput <<<"2" + + if bashunit::env::supports_color; then + export TERM="$original_term" + fail "Expected supports_color to fail when tput colors reports 2" + return + fi + + export TERM="$original_term" + assert_successful_code 0 +} + +function test_supports_color_returns_success_when_tput_reports_8_or_more_colors() { + local original_term="${TERM:-}" + export TERM="xterm" + bashunit::mock tput <<<"256" + + bashunit::env::supports_color + local result=$? + + export TERM="$original_term" + assert_equals 0 "$result" +} + function test_print_verbose_outputs_env_var_names() { local original="$BASHUNIT_VERBOSE" export BASHUNIT_VERBOSE="true" diff --git a/tests/unit/io_test.sh b/tests/unit/io_test.sh new file mode 100644 index 00000000..7945b6c6 --- /dev/null +++ b/tests/unit/io_test.sh @@ -0,0 +1,17 @@ +#!/usr/bin/env bash + +function test_clear_screen_uses_tput_when_available() { + bashunit::mock tput <<<"CLEARED" + + local output + output=$(bashunit::io::clear_screen) + + assert_contains "CLEARED" "$output" +} + +function test_clear_screen_emits_non_empty_output() { + local output + output=$(bashunit::io::clear_screen) + + assert_not_empty "$output" +} From c51e2c8e413e388e1ac5eecfa68bf4ffa744e5d3 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Wed, 29 Apr 2026 14:33:57 +0200 Subject: [PATCH 2/5] fix(colors): defer supports_color auto-disable in colors.sh init GitHub Actions runners report TERM=dumb, which made the new tput-based capability probe disable color globally and broke snapshot/escape-asserting tests across the CI matrix (same failure mode as PR #245). Keep bashunit::env::supports_color exposed as a query, but stop wiring it into colors.sh init. ADR-006 updated to record the deferral and CHANGELOG adjusted accordingly. Auto-disable will land in a follow-up once a CI-aware override (CI=true / FORCE_COLOR) is in place and validated on the matrix. --- CHANGELOG.md | 3 +-- adrs/adr-006-tput-vs-ansi-escapes.md | 4 ++-- src/colors.sh | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cfac1c4..554c5116 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,7 @@ ### Added - Display captured test output on assertion failures when `--show-output` is enabled (#637) -- Auto-disable colored output when the terminal cannot render it: `TERM=dumb` or `tput colors` reporting fewer than 8 now suppress color the same way `--no-color` / `NO_COLOR` does (#247) -- `bashunit::env::supports_color` helper exposing the capability probe; `bashunit::io::clear_screen` helper that prefers `tput clear` and falls back to the raw ANSI sequence (#247) +- `bashunit::env::supports_color` helper exposing a capability probe (`TERM=dumb` / `tput colors < 8`) for future auto-detection use; `bashunit::io::clear_screen` helper that prefers `tput clear` and falls back to the raw ANSI sequence (#247) ### Changed - Centralize all ANSI escape emission through the existing `_BASHUNIT_COLOR_*` constants. `src/coverage.sh` and the `--watch` screen-clear in `src/main.sh` no longer hardcode escape sequences (#247) diff --git a/adrs/adr-006-tput-vs-ansi-escapes.md b/adrs/adr-006-tput-vs-ansi-escapes.md index 7d04385a..45dc3158 100644 --- a/adrs/adr-006-tput-vs-ansi-escapes.md +++ b/adrs/adr-006-tput-vs-ansi-escapes.md @@ -35,14 +35,14 @@ Rationale: ANSI SGR codes are stable and identical to what `tput setaf` emits on Concretely: 1. Keep `bashunit::sgr` and `_BASHUNIT_COLOR_*` constants as the only place that emits color sequences. All ad-hoc `\033[...m` literals in `src/` are migrated to these constants. -2. Add `bashunit::env::supports_color` that returns false when `TERM=dumb`, `NO_COLOR` is set, or `tput colors` reports fewer than 8 colors. Wire this into `colors.sh` so colors auto-disable. +2. Expose `bashunit::env::supports_color` returning false when `TERM=dumb` or `tput colors` reports fewer than 8 colors. Available for callers that need a capability check. **Not** wired into `colors.sh` init: GitHub Actions sets `TERM=dumb` on runners, and PR #245's instability was caused by exactly this style of auto-disable. Auto-disable is deferred until we add a CI-aware override (e.g. `CI=true` / `FORCE_COLOR`) and validate across the matrix. 3. Replace the hardcoded `printf '\033[2J\033[H'` screen clear with `tput clear` when available, falling back to the ANSI sequence otherwise. 4. `tput` is already used for `tput cols` in `src/env.sh:215-219` — that pattern (probe + ANSI fallback) is the model. ### Positive Consequences * One place (`bashunit::sgr` + `_BASHUNIT_COLOR_*`) to change colors. -* Color auto-disables on dumb terminals and non-TTY pipelines without requiring `--no-color`. +* `supports_color` is now available for future use (e.g. CLI auto-detect, theming). * Screen clear works on terminals where the hardcoded sequence is wrong. * No subprocess explosion: `tput` is invoked once at init for capability probing, not per emitted color. * Avoids the failure mode from the previous attempt (per-call `tput setaf` returning empty strings under unusual `TERM` values). diff --git a/src/colors.sh b/src/colors.sh index 681c0823..09765fd7 100644 --- a/src/colors.sh +++ b/src/colors.sh @@ -18,7 +18,7 @@ bashunit::sgr() { echo $'\e'"[${codes}m" } -if bashunit::env::is_no_color_enabled || ! bashunit::env::supports_color; then +if bashunit::env::is_no_color_enabled; then _BASHUNIT_COLOR_BOLD="" _BASHUNIT_COLOR_FAINT="" _BASHUNIT_COLOR_BLACK="" From 1e8a8ff1dee028b8557666969781f131c185b3eb Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Wed, 29 Apr 2026 14:47:02 +0200 Subject: [PATCH 3/5] fix(io): clear_screen falls back to ANSI when tput emits nothing CI runners often have TERM unset, so `tput clear` writes "tput: No value for \$TERM and no -T specified" to stderr and produces empty stdout. Capture tput output and only use it when non-empty; otherwise emit the raw \033[2J\033[H sequence directly. --- src/io.sh | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/io.sh b/src/io.sh index b687d6a9..37df004b 100644 --- a/src/io.sh +++ b/src/io.sh @@ -7,10 +7,14 @@ ## function bashunit::io::clear_screen() { if command -v tput >/dev/null 2>&1; then - tput clear - else - printf '\033[2J\033[H' + local out + out=$(tput clear 2>/dev/null) + if [ -n "$out" ]; then + printf '%s' "$out" + return + fi fi + printf '\033[2J\033[H' } function bashunit::io::download_to() { From 484fe83ccffa4108075a254105971589a49fccc4 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Wed, 29 Apr 2026 15:00:41 +0200 Subject: [PATCH 4/5] docs(adr): simplify ADR-006 prose, drop em-dashes --- adrs/adr-006-tput-vs-ansi-escapes.md | 88 ++++++++++------------------ 1 file changed, 30 insertions(+), 58 deletions(-) diff --git a/adrs/adr-006-tput-vs-ansi-escapes.md b/adrs/adr-006-tput-vs-ansi-escapes.md index 45dc3158..997664cb 100644 --- a/adrs/adr-006-tput-vs-ansi-escapes.md +++ b/adrs/adr-006-tput-vs-ansi-escapes.md @@ -1,83 +1,55 @@ -# Hybrid tput + ANSI for terminal output +# Keep ANSI for colors, use tput where it pays off * Status: accepted * Deciders: @Chemaclass * Date: 2026-04-29 -Technical Story: [#247](https://github.com/TypedDevs/bashunit/issues/247) — evaluate replacing hardcoded ANSI escape sequences with `tput` (terminfo). +Technical Story: [#247](https://github.com/TypedDevs/bashunit/issues/247) -## Context and Problem Statement +## Context -bashunit emits colored output and screen-clear sequences via hardcoded ANSI escapes (`\e[31m`, `\033[2J\033[H`). `tput` queries the `terminfo` database, adapting to terminal capabilities and providing safer, more portable codes. A previous attempt to fully migrate to `tput` (around PR #245) caused widespread test instability across CI environments, so any move toward `tput` must be incremental and reliable. +bashunit prints colors and clears the screen with hardcoded ANSI escapes (`\e[31m`, `\033[2J\033[H`). The idea floated in #247 was to switch to `tput`, which reads terminfo and is in theory more portable. -Should bashunit replace ANSI escapes with `tput`? +We tried something similar around PR #245 and it broke the test suite across CI envs. Lots of runners ship with `TERM=dumb` or no `TERM` at all, so `tput setaf` returns empty and colored output silently disappears. -## Decision Drivers +So the question is not really "tput or ANSI" but "where does tput actually help us, and where does it just break things?" -* Bash 3.0+ portability (macOS, Linux, BSD, Windows runners) -* Reliability across CI matrices (GitHub Actions, dumb terminals, non-TTY pipelines) -* Minimal behavioral churn — tests and snapshots must keep passing -* Single source of truth for color/control sequences -* Auto-disable color when terminal does not support it (avoid garbled output) +## Options -## Considered Options +* A. Replace every ANSI escape with `tput`. +* B. Keep ANSI everywhere, change nothing. +* C. Keep ANSI for colors. Use tput only where it gives us something ANSI cannot. -* A. Full migration: replace every ANSI escape with `tput` -* B. Status quo: keep ANSI everywhere -* C. Hybrid: keep ANSI as primary mechanism via centralized helper, adopt `tput` for capability probing and select control sequences (e.g. screen clear) +## Decision -## Decision Outcome +Option C. -Chosen option: **C. Hybrid**. +Reasoning: -Rationale: ANSI SGR codes are stable and identical to what `tput setaf` emits on color terminals, so a wholesale `tput` rewrite buys little while introducing the failure modes that broke the previous attempt (e.g. `tput` returning empty strings under `TERM=dumb`, missing `terminfo` entries on stripped-down CI images, subprocess overhead per call). The real wins from `tput` are (1) capability probing (`tput colors`) to auto-disable color when unsupported and (2) portability for non-color sequences like screen clear. We adopt those targeted uses while keeping the existing centralized `bashunit::sgr` helper as the only emitter of color escape sequences. +* For colors, tput just emits the same ANSI codes we already write by hand. The only thing it adds is breaking on dumb terminals. +* For things ANSI cannot do well, like probing whether the terminal supports color at all, or producing the right "clear screen" sequence on weird terminals, tput is genuinely useful. +* We already use `tput cols` in `src/env.sh` with an ANSI/`stty` fallback. Same pattern fits here. -Concretely: +What this PR does: -1. Keep `bashunit::sgr` and `_BASHUNIT_COLOR_*` constants as the only place that emits color sequences. All ad-hoc `\033[...m` literals in `src/` are migrated to these constants. -2. Expose `bashunit::env::supports_color` returning false when `TERM=dumb` or `tput colors` reports fewer than 8 colors. Available for callers that need a capability check. **Not** wired into `colors.sh` init: GitHub Actions sets `TERM=dumb` on runners, and PR #245's instability was caused by exactly this style of auto-disable. Auto-disable is deferred until we add a CI-aware override (e.g. `CI=true` / `FORCE_COLOR`) and validate across the matrix. -3. Replace the hardcoded `printf '\033[2J\033[H'` screen clear with `tput clear` when available, falling back to the ANSI sequence otherwise. -4. `tput` is already used for `tput cols` in `src/env.sh:215-219` — that pattern (probe + ANSI fallback) is the model. +1. All color escapes go through `bashunit::sgr` and the `_BASHUNIT_COLOR_*` constants. No more raw `\033[...m` literals in `src/coverage.sh` or `src/main.sh`. +2. New `bashunit::env::supports_color` (false on `TERM=dumb` or `tput colors < 8`). Exposed but not wired into `colors.sh` init yet. The same auto-disable broke CI in PR #245 and again on the first push of this branch, so it waits until we add a `CI` / `FORCE_COLOR` override. +3. New `bashunit::io::clear_screen` runs `tput clear` and falls back to `\033[2J\033[H` if tput is missing or returns nothing. Replaces the hardcoded clear in `--watch` mode. -### Positive Consequences +## Consequences -* One place (`bashunit::sgr` + `_BASHUNIT_COLOR_*`) to change colors. -* `supports_color` is now available for future use (e.g. CLI auto-detect, theming). -* Screen clear works on terminals where the hardcoded sequence is wrong. -* No subprocess explosion: `tput` is invoked once at init for capability probing, not per emitted color. -* Avoids the failure mode from the previous attempt (per-call `tput setaf` returning empty strings under unusual `TERM` values). +Good: -### Negative Consequences +* One place to change colors. +* Screen clear works on terminals where the literal ANSI is wrong. +* `supports_color` is ready for the next step (auto-detect with a CI override). -* Slight init-time cost for the `tput colors` probe. -* Test suite must mock `tput` in scenarios that depended on guaranteed-color output. Existing tests already mock `tput` for `find_terminal_width` (see `tests/unit/env_test.sh:144`), so the pattern is established. +Bad: -## Pros and Cons of the Options - -### A. Full migration - -* Good, because terminfo is the canonical Unix way to handle terminals. -* Good, because `tput` adapts to capability quirks (e.g. 8 vs 256 colors). -* Bad, because the previous attempt destabilized CI tests across environments. -* Bad, because every color emission becomes a subprocess call (`$(tput setaf 1)`), measurable overhead in tight loops like the runner output. -* Bad, because `terminfo` databases on minimal CI images may lack capabilities, returning empty strings and silently breaking output. - -### B. Status quo - -* Good, because it is known to work across the entire current CI matrix. -* Bad, because color does not auto-disable on dumb terminals. -* Bad, because ad-hoc `\033[...m` literals in `coverage.sh` and `main.sh` bypass the centralized helper. - -### C. Hybrid (chosen) - -* Good, because it gets the practical benefits of `tput` (capability probing, portable control sequences) without the per-emission failure modes. -* Good, because it consolidates color emission through one helper, simplifying future changes (themes, 256-color, truecolor). -* Good, because it aligns with the existing `tput cols` pattern in `env.sh`. -* Bad, because two mechanisms coexist; contributors must know to use the constants, not raw escapes. Mitigated by lint/grep rules and code review. +* Two mechanisms (constants for colors, tput for clear/probe). Contributors need to know not to add raw escapes back. ## Links -* [Issue #247](https://github.com/TypedDevs/bashunit/issues/247) -* [PR #245 — Increase contrast of test results](https://github.com/TypedDevs/bashunit/pull/245) -* [terminfo(5) man page](https://man7.org/linux/man-pages/man5/terminfo.5.html) -* [NO_COLOR specification](https://no-color.org/) +* Issue [#247](https://github.com/TypedDevs/bashunit/issues/247) +* PR [#245](https://github.com/TypedDevs/bashunit/pull/245) +* [NO_COLOR spec](https://no-color.org/) From 806938568a194f27b0f0e2b1ee720aeb9d89e9ff Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Wed, 29 Apr 2026 16:42:40 +0200 Subject: [PATCH 5/5] refactor: address self-review on PR #646 - Add bashunit::dependencies::has_tput and reuse it from bashunit::env::supports_color and bashunit::io::clear_screen, matching the existing has_curl / has_wget pattern. - Extract bashunit::coverage::get_color_for_class to remove the duplicated case block in coverage::report_text (per-file row and total summary). - Drop a verbose WHAT comment in coverage.sh now that the constants speak for themselves. --- src/coverage.sh | 26 ++++++++++++-------------- src/dependencies.sh | 4 ++++ src/env.sh | 4 ++-- src/io.sh | 2 +- 4 files changed, 19 insertions(+), 17 deletions(-) diff --git a/src/coverage.sh b/src/coverage.sh index f15c77f2..839512ac 100644 --- a/src/coverage.sh +++ b/src/coverage.sh @@ -159,6 +159,14 @@ function bashunit::coverage::get_coverage_class() { fi } +function bashunit::coverage::get_color_for_class() { + case "$1" in + high) printf '%s' "$_BASHUNIT_COLOR_PASSED" ;; + medium) printf '%s' "$_BASHUNIT_COLOR_SKIPPED" ;; + low) printf '%s' "$_BASHUNIT_COLOR_FAILED" ;; + esac +} + # Calculate percentage from hit and executable counts function bashunit::coverage::calculate_percentage() { local hit="$1" @@ -737,14 +745,8 @@ function bashunit::coverage::report_text() { total_executable=$((total_executable + executable)) total_hit=$((total_hit + hit)) - # Determine color based on class. Constants are empty when --no-color - # or NO_COLOR is set (see src/colors.sh), so no extra guard is needed. - local color="" reset="$_BASHUNIT_COLOR_DEFAULT" - case "$class" in - high) color="$_BASHUNIT_COLOR_PASSED" ;; - medium) color="$_BASHUNIT_COLOR_SKIPPED" ;; - low) color="$_BASHUNIT_COLOR_FAILED" ;; - esac + local color reset="$_BASHUNIT_COLOR_DEFAULT" + color=$(bashunit::coverage::get_color_for_class "$class") # Display relative path local display_file="${file#"$(pwd)"/}" @@ -765,12 +767,8 @@ function bashunit::coverage::report_text() { total_pct=$(bashunit::coverage::calculate_percentage "$total_hit" "$total_executable") total_class=$(bashunit::coverage::get_coverage_class "$total_pct") - local color="" reset="$_BASHUNIT_COLOR_DEFAULT" - case "$total_class" in - high) color="$_BASHUNIT_COLOR_PASSED" ;; - medium) color="$_BASHUNIT_COLOR_SKIPPED" ;; - low) color="$_BASHUNIT_COLOR_FAILED" ;; - esac + local color reset="$_BASHUNIT_COLOR_DEFAULT" + color=$(bashunit::coverage::get_color_for_class "$total_class") printf "%sTotal: %d/%d (%d%%)%s\n" \ "$color" "$total_hit" "$total_executable" "$total_pct" "$reset" diff --git a/src/dependencies.sh b/src/dependencies.sh index c605167f..1d12c034 100644 --- a/src/dependencies.sh +++ b/src/dependencies.sh @@ -40,3 +40,7 @@ function bashunit::dependencies::has_python() { function bashunit::dependencies::has_node() { command -v node >/dev/null 2>&1 } + +function bashunit::dependencies::has_tput() { + command -v tput >/dev/null 2>&1 +} diff --git a/src/env.sh b/src/env.sh index 43c3db26..8a1ab14f 100644 --- a/src/env.sh +++ b/src/env.sh @@ -185,14 +185,14 @@ function bashunit::env::is_no_color_enabled() { ## # Whether the current terminal can render ANSI color sequences. # Returns 1 when TERM=dumb or when `tput colors` reports fewer than 8. -# Returns 0 when tput is missing (assume colors work — preserves prior behavior). +# Returns 0 when tput is missing (assume colors work, preserving prior behavior). ## function bashunit::env::supports_color() { if [ "${TERM:-}" = "dumb" ]; then return 1 fi - if ! command -v tput >/dev/null 2>&1; then + if ! bashunit::dependencies::has_tput; then return 0 fi diff --git a/src/io.sh b/src/io.sh index 37df004b..c3f2a1e4 100644 --- a/src/io.sh +++ b/src/io.sh @@ -6,7 +6,7 @@ # and falls back to the ANSI sequence \033[2J\033[H otherwise. ## function bashunit::io::clear_screen() { - if command -v tput >/dev/null 2>&1; then + if bashunit::dependencies::has_tput; then local out out=$(tput clear 2>/dev/null) if [ -n "$out" ]; then