Repair the Tauri retry test against the download-marker pipeline by danielhanchen · Pull Request #8996 · unslothai/unsloth · GitHub
Skip to content

Repair the Tauri retry test against the download-marker pipeline - #8996

Merged
danielhanchen merged 3 commits into
mainfrom
fix-sh-installer-harness
Aug 16, 2026
Merged

Repair the Tauri retry test against the download-marker pipeline#8996
danielhanchen merged 3 commits into
mainfrom
fix-sh-installer-harness

Conversation

@danielhanchen

Copy link
Copy Markdown
Member

Shell installer tests is failing on main, and failing in the worst way: the file prints its header and exits 1 with no FAIL line at all.

##[group]tests/sh/test_tauri_retry_failure_context.sh
=== run_install_cmd_retry Tauri failure context ===
##[error]Process completed with exit code 1.

Two separate things broke, and both are silent.

Cause

#8805 (b69bfe021) added the uv download markers, which put the wrapped command inside { ...; } | _uv_download_markers .... The verbose path was already a pipe. The quiet path, which is the one this file exercises, was not:

-    } | _redact_install_output
+    } | _uv_download_markers "" "$UNSLOTH_DL_MARKER_MIN_BYTES" | _redact_install_output
+    } | _uv_download_markers "$_log" "$UNSLOTH_DL_MARKER_MIN_BYTES"

That has two consequences for this test:

  1. The helper is missing from the harness. This file hand-copies five functions out of install.sh and stubs five more. It never learned about _uv_download_markers, so the last stage of the pipeline was "command not found", every wrapped command looked like it exited 127, and set -e ended the run.

  2. The command now runs in a subshell. _test_command simulated "fail once, then succeed" by incrementing a shell variable. Those increments are discarded now, so it failed on every attempt instead of succeeding on the second, which is precisely the retry-recovers path the first assertion exists to check.

#8805 is correct on both counts. Streaming output through a marker filter requires a pipe, and real installs run external commands, so running them in a subshell costs them nothing. It is the test harness that had gone stale.

Fix

  • Extract _uv_download_markers from install.sh rather than stub it, so the test still exercises the real pipe the installer runs.
  • Move the attempt counter into a file, so it survives the subshell.
  • Add || true to the two grep -c calls gating the first assertion. grep -c exits 1 on a zero count, which under set -e aborts with no output. That is why this read as silence rather than as a failure. The same guard is already applied to the grep -c calls further down the file.

Verification

Both fixes are needed. Each alone still fails, at a different assertion:

applied outcome
neither silent exit 1 at the header
counter fix only FAIL: quiet failure did not bind its command output to the structured error
extraction only silent exit 1 at the header
both 15 checks pass

Whole suite, discovered and run the way the workflow does it: 47 files, 0 failures.

Shell installer tests has been failing on main, and failing in the worst way:
the file printed its header and exited 1 with no FAIL line, because two
separate things broke and both of them are silent.

#8805 added the uv download markers, which put the wrapped command inside
`{ ...; } | _uv_download_markers ...`. The verbose path was already a pipe;
the quiet path, which is the one this file exercises, was not. Two
consequences:

1. This file hand-copies five functions out of install.sh and stubs five
   more. It never learned about _uv_download_markers, so the last stage of
   that pipeline was "command not found", every wrapped command looked like
   it exited 127, and `set -e` ended the run. Extract the real function
   rather than stub it, so the test still exercises the pipe the installer
   actually runs.

2. The command now runs in a subshell, so _test_command's `_test_attempt`
   increments were discarded. It failed on every attempt instead of
   succeeding on the second, which is precisely the retry-recovers path the
   first assertion is about. Move the counter into a file.

Both are needed; each alone still fails, at a different assertion.

#8805 is right on both counts. Streaming output through a marker filter
requires a pipe, and real installs run external commands, so a subshell costs
them nothing. It was the test's harness that had gone stale.

Also add `|| true` to the two `grep -c` calls that gate the first
assertion. grep -c exits 1 on a zero count, which under `set -e` aborts with
no output. That is why this read as silence rather than as a failure, and the
same guard is already applied to the grep -c calls later in the file.

tests/sh: 47 files, all passing.
chatgpt-codex-connector[bot]

This comment was marked as resolved.

The second trap replaces the first, so naming the attempt file only in the
first one leaked it: one stray temp file per run, measured.
@chatgpt-codex-connector

Copy link
Copy Markdown

My earlier commit message claimed the grep -c guard was already applied to the
rest of the file. That was wrong: lines 231, 326 and 368 were still unguarded,
so a zero count there aborts under set -e before the FAIL line it gates.

Worse, the guard I did add only covers the case where a wrapped command
succeeds and emits the wrong markers. The dominant path is the command itself
returning non-zero: under set -e that aborts at the call, before any assertion
runs. A broken installer retry therefore still reproduced the exact
header-then-silence symptom this file was fixed for.

Guard the three grep -c sites, and give the four commands that must succeed an
explicit failure message instead of letting set -e end the run mutely.

Each path mutation checked, silence to a named failure:

  retry never recovers       -> FAIL: recovered retry returned non-zero
  rollback_substep renamed   -> FAIL: successful Unix rollback output ...
  UNSLOTH_TAURI_MODE renamed -> FAIL: Unix installer does not pass Tauri mode

tests/sh: 47 files, all passing, no temp files leaked.
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 16, 2026
@danielhanchen
danielhanchen merged commit c87fe20 into main Aug 16, 2026
15 of 17 checks passed
@danielhanchen
danielhanchen deleted the fix-sh-installer-harness branch August 16, 2026 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant