diff options
| -rwxr-xr-x | archsetup | 32 | ||||
| -rw-r--r-- | tests/installer-steps/test_run_step.py | 92 |
2 files changed, 113 insertions, 11 deletions
@@ -287,22 +287,29 @@ mark_complete() { } run_step() { - step_name="$1" - step_func="$2" + local step_name="$1" + local step_func="$2" if step_completed "$step_name"; then printf "Skipping %s (already completed)\n" "$step_name" return 0 fi + # A step function that returns here has already passed every fatal check -- + # error_fatal exits the script outright, so it never reaches this point on a + # fatal error. A non-zero return can therefore only be a trailing non-fatal + # warning (error_warn returns 1), and the step's body still ran to + # completion. Record the marker either way so resume does not re-run a step + # that already did its work; surface a warning line when the return was + # non-zero. if $step_func; then mark_complete "$step_name" return 0 - else - printf "FAILED: %s\n" "$step_name" - printf "To retry this step, remove: %s/%s\n" "$state_dir" "$step_name" - return 1 fi + local rc=$? + printf "Step %s completed with warnings (last status %s)\n" "$step_name" "$rc" + mark_complete "$step_name" + return 0 } show_status() { @@ -315,6 +322,7 @@ show_status() { exit 0 fi echo "Completed steps:" + local step timestamp for step in "${STEPS[@]}"; do if step_completed "$step"; then timestamp=$(cat "$state_dir/$step") @@ -1038,15 +1046,17 @@ bootstrap_pacman_keyring() { # so transient mirror stalls recover instead of killing the run. action="refreshing the package cache" && display "task" "$action" refresh_ok=false + refresh_rc=0 for attempt in $(seq 1 "$MAX_INSTALL_RETRIES"); do - if pacman -Syu --noconfirm >> "$logfile" 2>&1; then - refresh_ok=true - break - fi + pacman -Syu --noconfirm >> "$logfile" 2>&1 && { refresh_ok=true; break; } + # Capture pacman's real exit here: after `if pacman; then ...; fi` bash + # reports the compound's status (0 when the condition was false), so + # error_fatal would otherwise log "error code: 0" for a genuine failure. + refresh_rc=$? [ "$attempt" -lt "$MAX_INSTALL_RETRIES" ] && \ display "task" "retrying package cache refresh (attempt $((attempt + 1))/$MAX_INSTALL_RETRIES)" done - $refresh_ok || error_fatal "$action" "$?" "run pacman -Syu manually to see the failure, or switch mirrors in /etc/pacman.d/mirrorlist" + $refresh_ok || error_fatal "$action" "$refresh_rc" "run pacman -Syu manually to see the failure, or switch mirrors in /etc/pacman.d/mirrorlist" } diff --git a/tests/installer-steps/test_run_step.py b/tests/installer-steps/test_run_step.py new file mode 100644 index 0000000..994e8c5 --- /dev/null +++ b/tests/installer-steps/test_run_step.py @@ -0,0 +1,92 @@ +"""Test run_step's state-marker and scope behavior. + +Bug (Major): run_step marked a step complete only when its function returned 0. +But error_fatal exits the script outright, so a step function that *returns* has +already survived every fatal check -- a non-zero return can only come from a +trailing non-fatal warning (error_warn returns 1). The old code read that as a +step failure and withheld the state marker, so the step re-ran on every resume. +run_step now records the marker whenever the function returns. + +Bug (Minor): run_step's step_name/step_func leaked to global scope. + +Method: sed-extract run_step + step_completed + mark_complete from the real +`archsetup`, point state_dir at a temp dir, and drive it with fake step +functions. + +Run from repo root: + python3 -m unittest tests.installer-steps.test_run_step +""" + +import os +import subprocess +import tempfile +import textwrap +import unittest + +REPO_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..")) +ARCHSETUP = os.path.join(REPO_ROOT, "archsetup") + +EXTRACT = ( + "source <(sed -n '/^run_step() {{/,/^}}/p;" + "/^step_completed() {{/,/^}}/p;" + "/^mark_complete() {{/,/^}}/p' \"{archsetup}\")" +).format(archsetup=ARCHSETUP) + + +def run(step_body, pre_marked=False): + """Run run_step against a fake step function whose body is step_body.""" + with tempfile.TemporaryDirectory() as state_dir: + marker_setup = ( + f'mkdir -p "{state_dir}"; echo pre > "{state_dir}/mystep"\n' + if pre_marked else "" + ) + script = textwrap.dedent(f"""\ + state_dir="{state_dir}" + {marker_setup} + mystep() {{ + {step_body} + }} + {EXTRACT} + run_step "mystep" "mystep" + rc=$? + echo "RUN_STEP_RC=$rc" + # marker present? + [ -f "$state_dir/mystep" ] && echo "MARKED=yes" || echo "MARKED=no" + # locals must not leak + echo "LEAK_step_name=[${{step_name:-}}]" + echo "LEAK_step_func=[${{step_func:-}}]" + """) + return subprocess.run( + ["bash", "-c", script], capture_output=True, text=True, timeout=10, + ) + + +class RunStep(unittest.TestCase): + def test_success_marks_complete(self): + r = run("return 0") + self.assertIn("RUN_STEP_RC=0", r.stdout) + self.assertIn("MARKED=yes", r.stdout) + + def test_nonfatal_warning_return_still_marks_complete(self): + # The bug fix: a step ending in a non-fatal warning returns 1 but has + # done its work; it must be marked so resume does not re-run it. + r = run("return 1") + self.assertIn("MARKED=yes", r.stdout, + "a returned-non-zero step must still record its marker") + self.assertIn("RUN_STEP_RC=0", r.stdout, + "run_step returns 0 so the main loop treats it as handled") + + def test_already_completed_skips_and_does_not_rerun(self): + # Body would create a sentinel if it ran; pre-marked step must skip it. + r = run("echo RAN-BODY", pre_marked=True) + self.assertIn("Skipping", r.stdout) + self.assertNotIn("RAN-BODY", r.stdout) + + def test_locals_do_not_leak_to_global_scope(self): + r = run("return 0") + self.assertIn("LEAK_step_name=[]", r.stdout) + self.assertIn("LEAK_step_func=[]", r.stdout) + + +if __name__ == "__main__": + unittest.main() |