Skip to content

test(cli): cover execute_plan early stop on failure/reboot (#602) - #611

Merged
vladimirrott merged 4 commits into
lacs-project:mainfrom
ITSMERNB:test/cli-execute-plan-regression-602
Oct 16, 2026
Merged

vladimirrott merged 4 commits into
lacs-project:mainfrom
ITSMERNB:test/cli-execute-plan-regression-602

Conversation

@ITSMERNB

Copy link
Copy Markdown
Contributor

test(cli): guard execute_plan against failed/rebooting steps (#602)

Closes #602.

Thanks to @vladimirrott for catching this gap.

The existing daemon-double tests checked the step verdict, but none exercised the error propagation in the actual execute_plan loop.

This PR extends the existing mock daemon to return a chosen status per action, then runs a real three-step execute_plan for both failure cases:

  • failed on the second step: exit code 2.
  • needs_reboot on the second step: exit code 5.
  • In both cases, the third step is neither previewed nor executed.

Verification on Linux (rust:1-slim):

  • cargo fmt --all -- --check — pass.
  • cargo test -p sysknife-cli --bins --offline — 321 passed; 0 failed.
  • Mutation: replaced exec_result?; with let _ = exec_result;. The new test fails as expected, because the third action executes and the plan returns success:
    test runner::tests::plan_preflight_tests::execute_plan_halts_after_failed_or_rebooting_second_step ... FAILED
    daemon failure/reboot must stop the plan: ()
    test result: FAILED. 0 passed; 1 failed; 320 filtered out
    
  • Restored the original line afterward; working tree clean.

This is test-only; no production changes.

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 01736334. Approving.

You extended mock_execution_daemon with a per-action status map and left run() as a thin wrapper, so the existing tests that call run() did not change and the new test drives the real execute_plan loop. That was the gap in #602: the earlier tests each wrote their own loop. Asserting the full event sequence, and the step name in the error, pins down both halves of the contract.

What I ran, in a --network=none rust:1-slim container:

filter:    execute_plan_halts_after_failed_or_rebooting_second_step
mutation:  s/^        exec_result?;$/        let _ = exec_result;/
unmutated: 1 executed unit, passing
mutated:   test runner::tests::plan_preflight_tests::execute_plan_halts_after_failed_or_rebooting_second_step ... FAILED (rc=101)

Also checked:

  • cargo fmt --all -- --check on the PR tree: rc=0.
  • Test count: runner.rs has 101 test attributes on main and 102 here. The diff adds one #[tokio::test] and removes none, so 1,991 is right in the JSON and in all three docs.
  • CI: 12 pass, container-smoke skipped.

Nothing blocking and nothing optional. Test-only, so no CHANGELOG line.

If you want the next one, #561 sits beside this code: --dry-run exits 0 for a plan the real run would refuse, because the dry-run path returns before the --max-risk ceiling and the distro check. It is in the pool now. Say so on #561 and I'll hold it for you.

@vladimirrott
vladimirrott merged commit 10d7483 into lacs-project:main Oct 16, 2026
13 checks passed
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.

cli: no test fails when the step loop stops halting on a failed step

2 participants