mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
2278115028
Fixes #644. ## Before ``` $ argent flow run echo-only # .argent/flows/echo-only.yaml: two `echo` steps [Tool:flow-execute] No booted device found. Pass a device id or platform explicitly. Available devices: 18BE573F-… (ios, Shutdown), <70 more> ``` and when a device *was* booted: ``` PASS on 18BE573F-… — 0 passed, 0 failed, 0 errored, 0 skipped ``` ## After ``` $ argent flow run echo-only # nothing booted Flow "echo-only" › first step › second step PASS — 0 passed, 0 failed, 0 errored, 0 skipped (no test steps) ``` ## Symptom 1 — the device Whether a device is needed is decided from the flow's own steps, before resolution. Classification is **per step kind and defaults to needing one**, so a kind added later inherits today's behaviour instead of silently running against no device — and the `never` binding makes leaving a new kind unclassified a compile error, backed by a test keyed on the step union so it fails to build rather than to run. Two classifications worth calling out: - **`when` always needs a device**, whatever its body contains, because the guard reads one itself (its platform, or its view tree). A `when: {platform: ios}` block containing only narration still resolves. - **`run` needs one without the fragment being read here.** The named flow is resolved at run time; resolving it a second time would duplicate that lookup and could disagree with it if the file changed in between. A deliberate over-approximation: composing a narration-only fragment still resolves a device. Stated in the code and in the tool description. For `tool:` steps the tool's own schema decides, via a superset of the keys the runner injects — `device` is included because a nested `flow-execute` step drives a device without being handed the run's own. A tool that declares no input at all (`stop-all-simulator-servers`) is device-free, and reading its absent schema must not throw — it previously would have. `ExecState.device` is nullable, which is the safety mechanism rather than just typing: the compiler enumerates every site that acts on a device, each of which now says so via one `deviceEnv()` narrowing. `flow-actions.ts`, `flow-visual.ts` and `ActionEnv` are untouched. The executor also re-checks each step against the **same predicate**, so if the scan and the executor ever disagree the step reports it instead of failing obscurely deeper in. A run that resolved no device reports `device: ""`, and neither renderer claims it ran somewhere. ## Symptom 2 — the counters Reproducing this showed the issue's framing is slightly off: `--json` doesn't only disagree with the text, it disagrees with **itself** — counters all zero while its own `steps[]` marks both steps `"pass"`. So the CLI text was a faithful rendering of the server's counters. **The counters are right and stay as they are.** Narration is deliberately excluded, and it should be: after a hard stop every remaining step *including narration* is reported skipped, so counting it would inflate `skipped` on every real failing run — a worse and far more common lie than the vacuous one being reported here. It would also contradict the JUnit model in #578, where echo is `<system-out>` and not a `<testcase>`. What was missing was saying *why* zero. Both the CLI and the MCP renderer now append `(no test steps)`, and only on a pass — a cancelled run can be a FAIL with all four counters zero, where "no test steps" would read as though the failure had no cause. No wire format changes: `steps[].kind === "echo"` already distinguishes counted from uncounted steps, so the JSON is self-describing without widening the status enum. ## Conflicts, for whoever lands second This is a hot area — seven open PRs touch `flow-run.ts`. - **#578** rewrites the CLI summary line I also change (one hunk, one 7-line function, semantically independent — resolution is keep-both), and its `attachFailureDiagnostics` dereferences `env.device.platform`. **It will need a null-device guard**, or every failure in a device-less flow silently degrades to `read-failed` diagnostics. Happy to rebase onto it rather than the reverse. - **#585** rewrites `resolveRunDevice` at the same seam and edits the same description sentence. - **#538** rewrites `flow-device.ts`. Worth noting it makes `resolveFlowDevice` reject a physical iPhone for flows — skipping resolution means a narration-only flow now passes on a physical-iPhone-only host, which I think is desirable but is a second behaviour change on this seam. All new tests are in new files, since every existing flow test file in the blast radius is rewritten by an open PR. ## Behaviour changes a reviewer should weigh - A device-free flow no longer prints `PASS on <id>` **even when a device is booted**. That is the point: otherwise an identical flow reports differently in CI (no simulator) than on a laptop with one open. An explicit `--device` is still honoured and still named. - `--platform` is ignored for a device-free flow, where it previously errored with nothing booted. - A 0.18.1 CLI against a newer server would print `Flow "x" on ` — cosmetic version skew. ## Verification Live against the built CLI: device-less flow with nothing booted (the reported bug) and with a device booted (identical output, no ambient attribution); `--json` showing `device: ""`; explicit `--device` still naming it; a mixed flow and a composed flow both still demanding a device; a mixed flow with a device booted still reporting `PASS on <id> — 1 passed`. 24 new tests. tool-server 3103 passed, CLI 284 passed, MCP 78 passed — no existing test modified. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- > **#697 is stacked on this branch** — it conflicts with this one in `flow-run.ts`'s import block, so it was rebased on top. Merge this first.
64 lines
2.2 KiB
TypeScript
64 lines
2.2 KiB
TypeScript
import { describe, it, expect } from "vitest";
|
|
import { renderReport, renderSummary, type FlowReport } from "../src/flow.js";
|
|
|
|
function report(overrides: Partial<FlowReport> = {}): FlowReport {
|
|
return {
|
|
flow: "echo-only",
|
|
device: "",
|
|
ok: true,
|
|
passed: 0,
|
|
failed: 0,
|
|
skipped: 0,
|
|
errored: 0,
|
|
steps: [],
|
|
...overrides,
|
|
} as FlowReport;
|
|
}
|
|
|
|
describe("rendering a run that resolved no device", () => {
|
|
it("does not claim the run happened on a device", () => {
|
|
const line = renderSummary(report(), { withDevice: true });
|
|
|
|
expect(line).not.toContain(" on ");
|
|
expect(line).toBe("PASS — 0 passed, 0 failed, 0 errored, 0 skipped (no test steps)");
|
|
});
|
|
|
|
it("omits the device from the report header too", () => {
|
|
const lines = renderReport(report()).split("\n");
|
|
|
|
expect(lines[0]).toBe('Flow "echo-only"');
|
|
});
|
|
|
|
it("still names a device when the run had one", () => {
|
|
const line = renderSummary(report({ device: "UDID-1", passed: 2 }), { withDevice: true });
|
|
|
|
expect(line).toBe("PASS on UDID-1 — 2 passed, 0 failed, 0 errored, 0 skipped");
|
|
});
|
|
});
|
|
|
|
describe("the no-test-steps note", () => {
|
|
it("explains a passing run whose counters are all zero", () => {
|
|
expect(renderSummary(report())).toContain("(no test steps)");
|
|
});
|
|
|
|
it("is absent whenever anything was counted", () => {
|
|
expect(renderSummary(report({ passed: 1 }))).not.toContain("(no test steps)");
|
|
expect(renderSummary(report({ skipped: 1 }))).not.toContain("(no test steps)");
|
|
});
|
|
|
|
it("is absent on a failure, where the counts are not what needs explaining", () => {
|
|
// A cancelled run can be a FAIL with every counter still zero; calling that
|
|
// "no test steps" would read as though the failure had no cause.
|
|
const line = renderSummary(report({ ok: false }));
|
|
|
|
expect(line).toBe("FAIL — 0 passed, 0 failed, 0 errored, 0 skipped");
|
|
expect(line).not.toContain("(no test steps)");
|
|
});
|
|
|
|
it("leaves an ordinary summary byte-for-byte unchanged", () => {
|
|
const line = renderSummary(report({ ok: false, passed: 2, failed: 1, skipped: 1 }));
|
|
|
|
expect(line).toBe("FAIL — 2 passed, 1 failed, 0 errored, 1 skipped");
|
|
});
|
|
});
|