From cafedfdee3d3fb9b474243b4c35ab7e5e1466bbf Mon Sep 17 00:00:00 2001 From: GenericJam Date: Fri, 4 Sep 2026 19:22:04 -0600 Subject: [PATCH] Stop mix mob.deploy reporting success after shipping nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four ways it could, all observed rather than theorised. The one that started this: `mix mob.deploy --android --native` with no `sdk.dir` printed a skip warning, built nothing, and exited 0 — because `ok_count == length(results)` is `0 == 0` for a run that produced no results. The interesting half is telling a skip that was REQUESTED from one that was INCIDENTAL. A skip means "this device is not a target for this app", which is ordinary for a phone that happens to be attached and a failure when the run asked for that platform. `requested_platforms/1` reads the raw flags rather than the resolved list, since resolution collapses "no flag" into every platform and would make every default run fatal. An adversarial review caught that the first version of this rule was much coarser than its own description. It filtered SKIPPED devices by requested platform — but `deploy_all/1` only enumerates devices for platforms in the resolved list, and that list is a subset of the requested one, so the filter was always true. The shipped rule would have been "any skip at all is fatal once you name a platform", which fails ordinary setups: two booted simulators with the app on only the one you are working on, or a spare phone plugged in. Since `--ios`/`--android` is the only way to scope a run on macOS, that is the common invocation. It is now per platform — fatal only when nothing of that platform landed — and the partial-success case is device-verified. The same review found `--json` was unusable for its only purpose. Every progress line in this task, the deployer and the native build is a plain `IO.puts/1` to `:stdio`, so the document arrived buried in ANSI prose. The run now repoints its group leader at stderr, which moves all of it — subprocess output included — and writes the document to the real stdout captured beforehand. That last part matters: `:standard_io` also resolves through the group leader, so the obvious fix sent the JSON to stderr as well and the flag emitted nothing a pipe could read. Caught by testing it rather than by reading it. The document is also emitted on the native-build failure path, which produced none. Three further fixes from the review: an auto-detected iPhone narrows Android out of the build, so `--android --ios --native` with a phone attached counted Android as unserved and failed a run that did what was asked; a named `--device` that was found and deployed nothing still exited 0; and two build skips printed no reason while the failure told the user to read one. Test gaps it found, both since closed and mutation-checked: nothing in the suite called `build_all/1`, so deleting either half of the `requested:` wiring fully restored the bug with everything green — the same shape as the `Mix.raise` gap found last time, which is why those assertions now live in one file named for what they do. And the "only the requested platform's skips count" test asserted a state `deploy_all/1` cannot produce, so it was green and proved nothing. One review finding was wrong and is recorded rather than acted on: `&opts[&1]` was called a truthiness bug for `--no-android`, but `false` is already falsy, so the negated flag was excluded correctly. The explicit `== true` is kept for clarity, and the test says so. Device-verified on a physical iPhone SE and an iOS simulator: no flags with a skip present exits 0; `--ios` with one simulator deployed and the phone skipped exits 0; `--ios --device ` where the phone was skipped exits 1; `--device NOPE` exits 1; `--android --native` with `sdk.dir` removed exits 1; and `--json` parses as a single document from stdout with the prose on stderr. Refs MOB-150 Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 29 +++ lib/mix/tasks/mob.deploy.ex | 207 +++++++++++++++++- lib/mob_dev/native_build.ex | 74 ++++++- test/mix/tasks/mob_deploy_beam_flags_test.exs | 195 +++++++++++++++++ test/mob_dev/native_build_test.exs | 51 +++++ ...dle_id_wiring_test.exs => wiring_test.exs} | 86 +++++++- 6 files changed, 628 insertions(+), 14 deletions(-) rename test/mob_dev/{ios_bundle_id_wiring_test.exs => wiring_test.exs} (56%) diff --git a/CHANGELOG.md b/CHANGELOG.md index d0211ee..8d246d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,35 @@ Full module documentation: [hexdocs.pm/mob_dev](https://hexdocs.pm/mob_dev). ### Changed +- **`mix mob.deploy` no longer exits 0 after shipping nothing.** Four ways it + could, all fixed (MOB-150): + - `--android --native` with no `sdk.dir` in `android/local.properties` + printed a skip warning, built nothing, and succeeded. The old rule was + `ok_count == length(results)`, which is `0 == 0` for a run that produced no + results at all. + - `--ios` / `--android` where every device of that platform was skipped. + A skip stays non-fatal when it is incidental — a phone that happens to be + attached — and is fatal when the run named that platform. The rule is per + **platform**, not per device: one simulator deploying while a stale one is + skipped is a success, not a failure. + - `--device X` that reached X and deployed nothing to it, and `--device NOPE` + matching no device at all. + - `--ios` on Linux, which resolves to no platforms and enumerated no devices. + + A `--native` run that built the artifact and found no device to push it to + still exits 0 — "build the APK now, attach the phone after" is unchanged. + +### Added + +- **`mix mob.deploy --json`** — a machine-readable result on stdout listing the + deployed, failed and skipped devices with their per-device reasons, and an + `outcome` that mirrors the exit status. Progress output is redirected to + stderr for the run, so `mix mob.deploy --json | jq` receives exactly one + document. Emitted on the native-build failure path too, which is when a + caller most needs it. + +### Changed + - **`mix mob.deploy` now exits non-zero when a device fails.** A run that printed `Failed on 1 device(s)` previously still returned status 0, so CI and wrapper scripts read a failed deploy as success. Every device is still diff --git a/lib/mix/tasks/mob.deploy.ex b/lib/mix/tasks/mob.deploy.ex index 3e76764..31526ef 100644 --- a/lib/mix/tasks/mob.deploy.ex +++ b/lib/mix/tasks/mob.deploy.ex @@ -33,6 +33,8 @@ defmodule Mix.Tasks.Mob.Deploy do for scripted scenarios where you need a specific naming scheme. * `--schedulers ` — set BEAM scheduler count (saved to mob.exs) * `--beam-flags ""` — arbitrary BEAM flags string (saved to mob.exs) + * `--json` — machine-readable result on stdout; progress goes to + stderr, so `mix mob.deploy --json | jq` gets one document * `--slim` — strip OTP source/debug for size measurement on a real device. OFF by default for dev iteration (the strip pass adds ~5-10s per build); use this @@ -123,12 +125,34 @@ defmodule Mix.Tasks.Mob.Deploy do task exits non-zero if **any** device landed in the `Failed on N device(s)` bucket — including a partial success where other devices deployed fine. - Devices under `Skipped on N device(s)` (app not installed for that - platform) do not fail the run. + Devices under `Skipped on N device(s)` (app not installed for that platform) + do not fail the run — *unless you named that platform*. A skip means "this + device is not a target for this app", which is ordinary when it is a phone + that happens to be attached, and a failure when the run asked for it: + + * `mix mob.deploy` with an unrelated phone attached — exit 0. + * `mix mob.deploy --ios` where every iOS device was skipped — exit 1. + * `mix mob.deploy --ios` where one simulator deployed and a stale one was + skipped — exit 0. A partial success is a success; the rule is per + platform, not per device. + * `mix mob.deploy --device X` that reached X and deployed nothing — exit 1. + * `mix mob.deploy --device NOPE` matching no device — exit 1. + * `mix mob.deploy --android --native` that built the APK with no device + attached — exit 0. The artifact is what was asked for. + + `--native` fails the run when a platform you named built nothing at all, which + is what a missing `sdk.dir` in `android/local.properties` produces. """ + alias MobDev.Device + @switches [ native: :boolean, + # Machine-readable result on stdout, for a caller that needs to know which + # targets got what without parsing coloured prose. Progress is redirected + # to stderr for the run (see the group-leader swap in `run/1`) so stdout + # carries exactly one document. + json: :boolean, restart: :boolean, android: :boolean, ios: :boolean, @@ -157,6 +181,22 @@ defmodule Mix.Tasks.Mob.Deploy do def run(args) do {opts, _, _} = OptionParser.parse(args, switches: @switches) + # Under --json, stdout must carry ONE document and nothing else. Every + # progress line in this task, the deployer and the native build is a plain + # `IO.puts/1`, which resolves `:stdio` through the group leader — so + # repointing it sends all of them, and the `into: IO.stream()` subprocess + # output too, to stderr. The JSON is then written to the real stdout + # explicitly. Rewriting several hundred call sites to take a device would + # be the alternative. + if opts[:json] do + # Capture the real stdout FIRST. `:standard_io` also resolves through the + # group leader, so swapping it without saving this sends the document to + # stderr along with the prose — the flag then emits nothing a pipe can + # read, which is the whole failure it exists to prevent. + Process.put(:mob_deploy_stdout, Process.group_leader()) + Process.group_leader(self(), Process.whereis(:standard_error)) + end + restart = Keyword.get(opts, :restart, true) native = Keyword.get(opts, :native, false) device_id = opts[:device] @@ -220,7 +260,8 @@ defmodule Mix.Tasks.Mob.Deploy do MobDev.NativeBuild.build_all( platforms: platforms, device: effective_device_id, - slim: slim + slim: slim, + requested: requested_platforms(opts) ) end @@ -233,6 +274,7 @@ defmodule Mix.Tasks.Mob.Deploy do "#{IO.ANSI.yellow()}Run `mix mob.doctor` to check your environment, or `mix mob.deploy` (without --native) once the issue is fixed.#{IO.ANSI.reset()}" ) + emit_json(opts, [], [], [], "Native build failed") Mix.raise("Native build failed") end @@ -254,7 +296,19 @@ defmodule Mix.Tasks.Mob.Deploy do # The full summary is printed first, then the status code is set — the # fan-out across devices is unchanged, only the exit code is. - case failure_message(deployed, failed, skipped) do + message = + missing_device_message(device_id, deployed, failed, skipped) || + failure_message( + deployed, + failed, + skipped, + requested_platforms(opts), + native and native_ok == true + ) + + emit_json(opts, deployed, failed, skipped, message) + + case message do nil -> :ok message -> Mix.raise(message) end @@ -278,11 +332,138 @@ defmodule Mix.Tasks.Mob.Deploy do out if the status code says everything is fine. """ @spec failure_message([Device.t()], [Device.t()], [Device.t()]) :: String.t() | nil - def failure_message(_deployed, [], _skipped), do: nil + def failure_message(deployed, failed, skipped), + do: failure_message(deployed, failed, skipped, []) + + @doc """ + As `failure_message/3`, but knowing which platforms were explicitly asked for. + + A skipped device is normally not a failure — it means "this device is not a + target for this app", the expected outcome of an Android phone being attached + during a default run. It IS a failure when the run named that platform: a + `mix mob.deploy --android` that skips every Android device asked for + something and got nothing, and must not report success. + + Pass `[]` for requested and every skip is incidental, which is the + `failure_message/3` behaviour. + """ + @spec failure_message([Device.t()], [Device.t()], [Device.t()], [atom()]) :: String.t() | nil + def failure_message(_deployed, failed, _skipped, _requested) when failed != [], + do: "Deploy failed on #{length(failed)} device(s) — see errors above" - def failure_message(_deployed, failed, _skipped), + def failure_message(deployed, failed, skipped, requested), + do: failure_message(deployed, failed, skipped, requested, false) + + @doc """ + As `failure_message/4`, but knowing whether a native build succeeded. + + A `--native` run that built the artifact and found no device to push it to + did its main job. Failing it would break "build the APK now, attach the phone + after", which used to exit 0. + """ + @spec failure_message([Device.t()], [Device.t()], [Device.t()], [atom()], boolean()) :: + String.t() | nil + def failure_message(_deployed, failed, _skipped, _requested, _native) when failed != [], do: "Deploy failed on #{length(failed)} device(s) — see errors above" + def failure_message(deployed, _failed, skipped, requested, native_built?) do + # Per PLATFORM, not per device. `deploy_all/1` only enumerates devices for + # platforms in the resolved list, and the resolved list is a subset of the + # requested one, so "is this skip's platform requested?" is always true and + # the rule would reduce to "any skip at all is fatal once you name a + # platform". That fails a perfectly good run: two booted simulators with + # the app on only the one you are working on, or a spare phone plugged in. + # A platform is unserved only when it skipped AND nothing of it landed. + unserved = + Enum.filter(requested, fn platform -> + Enum.any?(skipped, &(&1.platform == platform)) and + not Enum.any?(deployed, &(&1.platform == platform)) + end) + + cond do + unserved != [] -> + "Deploy reached no #{names(unserved)} device — every one was skipped, " <> + "and you asked for it" + + # Asked for a platform and reached nothing at all. Covers `--ios` on + # Linux, where platform resolution yields an empty list, so no device is + # even enumerated and every bucket is empty. + # + # Not when a native build succeeded: that run produced the artifact it + # was asked for and merely had nowhere to push it. + requested != [] and deployed == [] and skipped == [] and not native_built? -> + "Deploy reached no device for #{names(requested)} — none was connected" + + true -> + nil + end + end + + defp names(platforms), do: platforms |> Enum.map(&"--#{&1}") |> Enum.join(", ") + + # Written to the REAL stdout, not the group leader — which under --json now + # points at stderr so the progress prose gets out of the document's way. + defp emit_json(opts, deployed, failed, skipped, message) do + if opts[:json] do + json = Jason.encode!(json_result(deployed, failed, skipped, message), pretty: true) + IO.puts(Process.get(:mob_deploy_stdout, :standard_io), json) + end + end + + @doc """ + The machine-readable result of a finished deploy. + + Exists because an agent driving `mix mob.deploy` otherwise has to infer the + outcome from coloured prose, and the exit code alone does not say *which* + target missed out. `outcome` mirrors the exit status: `"ok"` when the task + returns 0, `"error"` when it raises. + """ + @spec json_result([Device.t()], [Device.t()], [Device.t()], String.t() | nil) :: map() + def json_result(deployed, failed, skipped, message) do + %{ + "outcome" => if(message, do: "error", else: "ok"), + "message" => message, + "deployed" => Enum.map(deployed, &json_device/1), + "failed" => Enum.map(failed, &json_device/1), + "skipped" => Enum.map(skipped, &json_device/1) + } + end + + defp json_device(%MobDev.Device{} = device) do + %{ + "name" => device.name, + "serial" => device.serial, + "platform" => to_string(device.platform), + "reason" => device.error + } + end + + @doc """ + The message for a run that named a device and did not find it, or `nil`. + + `mix mob.deploy --device NOPE` printed "No devices found." and exited 0. The + device filter matches nothing, every bucket comes back empty, and a run that + shipped to a device you named by id is indistinguishable from one that + shipped nowhere. + + Only fires when a device was named: with no `--device`, an empty run is the + ordinary "nothing is plugged in" case and stays non-fatal. + """ + @spec missing_device_message(String.t() | nil, [Device.t()], [Device.t()], [Device.t()]) :: + String.t() | nil + def missing_device_message(nil, _deployed, _failed, _skipped), do: nil + + def missing_device_message(device_id, [], [], []), + do: "No device matched --device #{device_id} — nothing was deployed" + + # Found, but nothing landed on it. Naming a device by id is at least as + # explicit as naming a platform, so a run that shipped nowhere must say so. + # `failed` is left to `failure_message/5`, which reports the actual error. + def missing_device_message(device_id, [], [], skipped) when skipped != [], + do: "--device #{device_id} was skipped — nothing was deployed to it" + + def missing_device_message(_device_id, _deployed, _failed, _skipped), do: nil + @doc """ Build the per-deploy summary lines from the three device buckets. @@ -353,6 +534,20 @@ defmodule Mix.Tasks.Mob.Deploy do acc ++ [header | rows] end + @doc """ + The platforms the user explicitly asked for, from the raw flags. + + Deliberately NOT `resolve_platforms/1`, which collapses "no flag given" into + every platform with a scaffold. That distinction is the whole point: a device + skipped during a default run is incidental (a phone that happens to be + attached), while one skipped during `--android` is a request that went + unserved. Returns `[]` when no platform flag was given. + """ + @spec requested_platforms(keyword()) :: [:android | :ios] + def requested_platforms(opts) do + Enum.filter([:android, :ios], &(opts[&1] == true)) + end + defp resolve_platforms(opts) do android = opts[:android] ios = opts[:ios] diff --git a/lib/mob_dev/native_build.ex b/lib/mob_dev/native_build.ex index 2d95a39..6e8941b 100644 --- a/lib/mob_dev/native_build.ex +++ b/lib/mob_dev/native_build.ex @@ -79,6 +79,12 @@ defmodule MobDev.NativeBuild do results not File.dir?("android") -> + if :android in Keyword.get(opts, :requested, []) do + IO.puts( + " #{IO.ANSI.yellow()}⚠ Skipping Android build — no android/ directory in this project#{IO.ANSI.reset()}" + ) + end + results not android_toolchain_available?() -> @@ -115,6 +121,12 @@ defmodule MobDev.NativeBuild do [build_ios(cfg, device_id) | results] true -> + if :ios in Keyword.get(opts, :requested, []) do + IO.puts( + " #{IO.ANSI.yellow()}⚠ Skipping iOS build — no ios/build.zig in this project#{IO.ANSI.reset()}" + ) + end + results end else @@ -137,10 +149,68 @@ defmodule MobDev.NativeBuild do ) end) - ok_count = Enum.count(results, &match?({:ok, _}, &1)) - ok_count == length(results) + # Intersect with the NARROWED platform list. `narrow_platforms_for_device/2` + # above drops Android when the target is an iOS UDID — including one this + # task auto-detected rather than one the user named — and counting that as + # an unserved `--android` request fails a build that did exactly what was + # asked of it. + requested = Enum.filter(Keyword.get(opts, :requested, []), &(&1 in platforms)) + + case build_outcome(results, requested) do + :ok -> + true + + {:error, message} -> + IO.puts(" #{IO.ANSI.red()}✗ #{message}#{IO.ANSI.reset()}") + false + end end + @doc """ + Whether a native build run succeeded, given what it produced and what the + user explicitly asked for. + + `results` entries are `{:ok, label}` / `{:error, label, reason}` where label + is the display name ("Android", "iOS", "iOS (device)"). + + `requested` is the platforms named by an explicit `--android` / `--ios` + flag — NOT the resolved platform list, which collapses "no flag given" into + every platform and would make an ordinary skip fatal. + + The rule this exists for: `ok_count == length(results)` is `0 == 0` for a run + that built nothing, so `mix mob.deploy --android --native` with no `sdk.dir` + printed a warning, built nothing, and reported success. A skip is fine when + nobody asked for that platform; it is a failure when they did. + """ + @spec build_outcome([{:ok, String.t()} | {:error, String.t(), term()}], [atom()]) :: + :ok | {:error, String.t()} + def build_outcome(results, requested) do + built = results |> Enum.map(&result_platform/1) |> Enum.reject(&is_nil/1) |> Enum.uniq() + failed = Enum.filter(results, &match?({:error, _, _}, &1)) + missing = requested -- built + + cond do + failed != [] -> + {:error, "#{length(failed)} native build(s) failed — see errors above"} + + missing != [] -> + names = missing |> Enum.map(&"--#{&1}") |> Enum.join(", ") + + {:error, + "nothing was built for #{names}, which you asked for — see the skip reason above"} + + true -> + :ok + end + end + + defp result_platform({:ok, label}), do: label_platform(label) + defp result_platform({:error, label, _reason}), do: label_platform(label) + + defp label_platform("Android" <> _), do: :android + defp label_platform("iOS" <> _), do: :ios + defp label_platform(_), do: nil + # ── Android ────────────────────────────────────────────────────────────────── defp build_android(cfg, device_id) do diff --git a/test/mix/tasks/mob_deploy_beam_flags_test.exs b/test/mix/tasks/mob_deploy_beam_flags_test.exs index 32c7e19..f619a2c 100644 --- a/test/mix/tasks/mob_deploy_beam_flags_test.exs +++ b/test/mix/tasks/mob_deploy_beam_flags_test.exs @@ -118,6 +118,11 @@ defmodule Mix.Tasks.Mob.DeployBeamFlagsTest do defp device(name, error \\ nil), do: %MobDev.Device{name: name, serial: name, platform: :android, error: error} + # The requested-vs-incidental rule keys on `:platform`, so the fixtures + # have to be able to be an iPhone. + defp ios_device(name, error \\ nil), + do: %MobDev.Device{name: name, serial: name, platform: :ios, error: error} + defp strip_ansi(line), do: String.replace(line, ~r/\e\[[0-9;]*m/, "") test "all three buckets empty → 'No devices found' hint" do @@ -268,4 +273,194 @@ defmodule Mix.Tasks.Mob.DeployBeamFlagsTest do assert Deploy.failure_message([], failed, []) =~ "Deploy failed on 3 device(s)" end end + + # ── MOB-150: a run that shipped nothing must not report success ────────────── + # + # PR #44 made a *failed* device fatal and deliberately left a *skipped* one + # alone, on the grounds that a skip means "this device is not a target". + # That is right for an incidental skip and wrong for a requested one, and it + # left three ways to exit 0 having deployed nothing. + + describe "requested_platforms/1" do + test "reads the raw flags, not the resolved platform list" do + assert Deploy.requested_platforms(android: true) == [:android] + assert Deploy.requested_platforms(ios: true) == [:ios] + assert Deploy.requested_platforms(android: true, ios: true) == [:android, :ios] + end + + test "a negated flag is not a request" do + # `--no-android` must not register as "you asked for Android", which + # would make the run fatal for a platform the user explicitly turned off. + # + # A review flagged the original `&opts[&1]` as a truthiness bug here. It + # was not: `false` is falsy, so that already excluded a negated flag, and + # mutating it back leaves this test green. The explicit `== true` is + # clarity about intent, not a fix — worth keeping, worth not + # misdescribing. + assert Deploy.requested_platforms(android: false) == [] + assert Deploy.requested_platforms(android: false, ios: true) == [:ios] + end + + test "no flag means nothing was explicitly requested" do + # The load-bearing case. `resolve_platforms/1` turns "no flag" into every + # platform with a scaffold; using that here would make an ordinary + # incidental skip fatal on every default run. + assert Deploy.requested_platforms([]) == [] + assert Deploy.requested_platforms(restart: true, slim: false) == [] + end + end + + describe "failure_message/4 — a skip you asked for" do + test "an incidental skip is still not a failure" do + skip = ios_device("iPhone", "app not installed") + assert Deploy.failure_message([device("emulator")], [], [skip], []) == nil + end + + test "every device of a platform you named being skipped is a failure" do + skip = ios_device("iPhone", "app not installed") + + message = Deploy.failure_message([], [], [skip], [:ios]) + + assert message =~ "reached no --ios device" + end + + test "a partial success is NOT a failure — one sim skipped, another deployed" do + # The false positive that made the first version of this rule unshippable. + # Two booted simulators with the app on only the one you are working on, + # or a spare phone plugged in, is an ordinary setup — and `--ios` is the + # only way to scope a run on macOS, where the default is both platforms. + # + # The earlier rule filtered SKIPPED devices by requested platform, which + # `deploy_all/1` makes a tautology: it only enumerates devices for + # platforms in the resolved list, and that list is a subset of the + # requested one. So the filter was always true and the rule reduced to + # "any skip at all is fatal once you name a platform". + deployed = ios_device("iPhone 17 Pro") + skipped = ios_device("stale sim", "app not installed") + + assert Deploy.failure_message([deployed], [], [skipped], [:ios]) == nil + end + + test "one platform fully skipped still fails when the other succeeded" do + # `--android --ios` where every Android device skipped: iOS working must + # not mask the half that was asked for and got nothing. + message = + Deploy.failure_message( + [ios_device("iPhone")], + [], + [device("emulator-5554", "not installed")], + [:android, :ios] + ) + + assert message =~ "--android" + refute message =~ "--ios" + end + + test "a real failure still outranks a skip" do + failed = device("buggy", "push timed out") + skip = ios_device("iPhone", "app not installed") + + assert Deploy.failure_message([], [failed], [skip], [:ios]) =~ "failed on 1 device(s)" + end + + test "asking for a platform and reaching nothing at all is a failure" do + # `--ios` on Linux resolves to no platforms, so no device is even + # enumerated: every bucket is empty and the run previously exited 0. + assert Deploy.failure_message([], [], [], [:ios]) =~ "reached no device for --ios" + end + + test "a successful native build with no device attached is not a failure" do + # "Build the APK now, attach the phone after" exited 0 before this ticket + # and must keep doing so — the run produced the artifact it was asked + # for and merely had nowhere to push it. + assert Deploy.failure_message([], [], [], [:android], true) == nil + + # Without a native build the run's only purpose was to push, and it + # pushed nowhere. + assert Deploy.failure_message([], [], [], [:android], false) =~ "none was connected" + end + + test "reaching nothing without asking for anything is fine" do + assert Deploy.failure_message([], [], [], []) == nil + end + + test "failure_message/3 keeps its old meaning" do + skip = ios_device("iPhone", "app not installed") + assert Deploy.failure_message([], [], [skip]) == nil + end + end + + describe "missing_device_message/4" do + test "a named device that was not found is a failure" do + message = Deploy.missing_device_message("NOPE", [], [], []) + assert message =~ "No device matched --device NOPE" + end + + test "no --device means an empty run is just nothing plugged in" do + assert Deploy.missing_device_message(nil, [], [], []) == nil + end + + test "a named device that WAS found and deployed is not a failure" do + assert Deploy.missing_device_message("serial", [device("serial")], [], []) == nil + end + + test "a named device that failed is left to failure_message" do + # It reports the actual error; double-reporting would mask the reason. + assert Deploy.missing_device_message("serial", [], [device("serial", "boom")], []) == nil + end + + test "a named device that was found but skipped is a failure" do + # Naming a device by id is at least as explicit as naming a platform, so + # a run that shipped nothing to it must say so. This exited 0. + skipped = [device("emulator-5554", "app not installed")] + + assert Deploy.missing_device_message("emulator-5554", [], [], skipped) =~ + "was skipped — nothing was deployed" + end + end + + describe "json_result/4" do + # An agent driving mob.deploy otherwise infers the outcome from coloured + # prose, and the exit code alone does not say WHICH target missed out. + test "a clean run reports ok and lists what was deployed" do + result = Deploy.json_result([device("emulator-5554")], [], [], nil) + + assert result["outcome"] == "ok" + assert result["message"] == nil + assert [%{"serial" => "emulator-5554", "platform" => "android"}] = result["deployed"] + end + + test "outcome mirrors the exit status, not the buckets" do + # A skip is fatal or not depending on whether it was requested, so the + # buckets alone cannot tell a caller what the exit code will be. The + # message is the decision, and outcome must follow it. + skip = ios_device("iPhone", "app not installed") + + assert Deploy.json_result([], [], [skip], nil)["outcome"] == "ok" + assert Deploy.json_result([], [], [skip], "asked for --ios")["outcome"] == "error" + end + + test "each bucket carries the per-device reason" do + failed = device("buggy", "push timed out") + result = Deploy.json_result([], [failed], [], "Deploy failed on 1 device(s)") + + assert [%{"name" => "buggy", "reason" => "push timed out"}] = result["failed"] + assert result["message"] == "Deploy failed on 1 device(s)" + end + + test "each device reports its own platform" do + # Only one assertion checked `platform`, and it expected "android", so + # hardcoding that string passed the suite. + result = Deploy.json_result([ios_device("iPhone")], [], [device("emu")], nil) + + assert [%{"platform" => "ios"}] = result["deployed"] + assert [%{"platform" => "android"}] = result["skipped"] + end + + test "it survives a round trip through Jason" do + result = Deploy.json_result([device("a")], [device("b", "boom")], [ios_device("c")], nil) + + assert result |> Jason.encode!() |> Jason.decode!() == result + end + end end diff --git a/test/mob_dev/native_build_test.exs b/test/mob_dev/native_build_test.exs index 1dbdae4..57e94ea 100644 --- a/test/mob_dev/native_build_test.exs +++ b/test/mob_dev/native_build_test.exs @@ -2249,4 +2249,55 @@ defmodule MobDev.NativeBuildTest do assert NativeBuild.ios_bundle_id([]) == nil end end + + describe "build_outcome/2 — MOB-150" do + # The observed bug: `mix mob.deploy --android --native` with no `sdk.dir` + # in android/local.properties printed a skip warning, built nothing, and + # exited 0. The old rule was `ok_count == length(results)`, which is + # `0 == 0` for a run that produced no results at all. + + test "everything asked for was built" do + assert NativeBuild.build_outcome([{:ok, "Android"}], [:android]) == :ok + end + + test "a build that failed is a failure" do + results = [{:error, "Android", "gradle exited 1"}] + assert {:error, message} = NativeBuild.build_outcome(results, [:android]) + assert message =~ "1 native build(s) failed" + end + + test "building nothing when nothing was asked for is fine" do + # A default `mix mob.deploy --native` on a machine with only the iOS + # toolchain: Android is skipped and nobody asked for it. + assert NativeBuild.build_outcome([], []) == :ok + assert NativeBuild.build_outcome([{:ok, "iOS"}], []) == :ok + end + + test "building nothing for a platform you named is a failure" do + assert {:error, message} = NativeBuild.build_outcome([], [:android]) + assert message =~ "nothing was built for --android" + end + + test "building only the other platform you named is a failure" do + # `--android --ios` where the Android toolchain is missing: iOS + # succeeding must not mask the half that was skipped. + assert {:error, message} = + NativeBuild.build_outcome([{:ok, "iOS"}], [:android, :ios]) + + assert message =~ "--android" + refute message =~ "--ios" + end + + test "the device iOS label counts as iOS" do + # The physical-device chain reports "iOS (device)", not "iOS", and a + # prefix match is what keeps that from reading as an unserved request. + assert NativeBuild.build_outcome([{:ok, "iOS (device)"}], [:ios]) == :ok + end + + test "a failure outranks an unserved request" do + results = [{:error, "iOS", "zig exited 2"}] + assert {:error, message} = NativeBuild.build_outcome(results, [:android, :ios]) + assert message =~ "failed" + end + end end diff --git a/test/mob_dev/ios_bundle_id_wiring_test.exs b/test/mob_dev/wiring_test.exs similarity index 56% rename from test/mob_dev/ios_bundle_id_wiring_test.exs rename to test/mob_dev/wiring_test.exs index 6e5a740..d2690e7 100644 --- a/test/mob_dev/ios_bundle_id_wiring_test.exs +++ b/test/mob_dev/wiring_test.exs @@ -4,8 +4,18 @@ # what the check is designed to catch. It is the right shape here: the paths # guarded need a keychain, a provisioning profile or a physical device to run, # and every one of these fixes was silently revertible before they existed. -defmodule MobDev.IosBundleIdWiringTest do +defmodule MobDev.WiringTest do @moduledoc """ + Assertions that a decision is actually WIRED to the code path it governs. + + Every finding that put a test in here had the same shape: a well-covered + pure function, and a call site nothing checked — so deleting the call left + the suite green and fully restored the bug. That has now happened four + times (the two bundle-id resolvers, the `Mix.raise` on a failed deploy, and + the `requested:` flag on the native build). + + ## iOS bundle ids + Every iOS path must resolve its bundle id through the `:ios_bundle_id || :bundle_id` rule. @@ -24,6 +34,14 @@ defmodule MobDev.IosBundleIdWiringTest do @release File.read!(Path.expand("../../lib/mob_dev/release.ex", __DIR__)) @deployer File.read!(Path.expand("../../lib/mob_dev/deployer.ex", __DIR__)) @deploy_task File.read!(Path.expand("../../lib/mix/tasks/mob.deploy.ex", __DIR__)) + @native_build_src File.read!(Path.expand("../../lib/mob_dev/native_build.ex", __DIR__)) + + defp index_of(hay, needle) do + case :binary.match(hay, needle) do + {i, _} -> i + :nomatch -> flunk("expected to find #{inspect(needle)}") + end + end defp region(source, from, to) do unless String.contains?(source, from), do: flunk("marker not found: #{inspect(from)}") @@ -95,14 +113,70 @@ defmodule MobDev.IosBundleIdWiringTest do # decides it still perfectly covered. body = region(@deploy_task, "Enum.each(format_summary(", "\n end") - assert body =~ "case failure_message(deployed, failed, skipped) do" + assert body =~ "failure_message(" + assert body =~ "missing_device_message(device_id, deployed, failed, skipped)" assert body =~ "message -> Mix.raise(message)" # Order matters: raising before the summary loses the per-device detail - # for every device in the run, which is what an operator reads. - summary_at = 0 - raise_at = :binary.match(body, "Mix.raise(message)") |> elem(0) - assert summary_at < raise_at + # for every device in the run, which is what an operator reads. Measured + # against the whole file — anchoring the region ON the summary made this + # `assert 0 < raise_at`, which cannot fail. + assert index_of(@deploy_task, "Enum.each(format_summary(") < + index_of(@deploy_task, "Mix.raise(message)") + end + end + + describe "the native build's honest-exit wiring (MOB-150)" do + # `build_outcome/2` is well covered as a pure function, but nothing in the + # suite calls `build_all/1` — so deleting either half of the wiring left + # 2244 tests green and fully restored the bug: `mix mob.deploy --android + # --native` with no sdk.dir builds nothing and exits 0. Same shape as the + # `Mix.raise` gap above: the decision was covered, the call was not. + test "the task tells the build which platforms were asked for" do + body = region(@deploy_task, "MobDev.NativeBuild.build_all(", "\n )") + + assert body =~ "requested: requested_platforms(opts)" + end + + test "the build reads that back and runs it through build_outcome/2" do + body = region(@native_build_src, "requested =", "\n end") + + assert body =~ "Keyword.get(opts, :requested, [])" + assert body =~ "case build_outcome(results, requested) do" + end + + test "an unserved request is intersected with the narrowed platform list" do + # An auto-detected iPhone narrows Android out of the build. Counting that + # as an unserved `--android` fails a run that did what was asked. + body = region(@native_build_src, "requested =", "case build_outcome") + + assert body =~ "Enum.filter(Keyword.get(opts, :requested, []), &(&1 in platforms))" + end + end + + describe "--json is declared and pipeable" do + # Deleting `json: :boolean` from @switches makes OptionParser drop the flag + # into `invalid`, `opts[:json]` becomes nil, and no document is emitted — + # with every json_result/4 test still green, because none of them went + # through the switch. + test "the switch exists" do + assert @deploy_task =~ "json: :boolean" + end + + test "progress is redirected so stdout carries one document" do + # The comment used to claim progress went to stderr. It did not: every + # line in this task, the deployer and the native build is a plain + # IO.puts/1 to :stdio, so `--json | jq` got ANSI prose and died. + assert @deploy_task =~ "Process.group_leader(self(), Process.whereis(:standard_error))" + assert @deploy_task =~ "Process.put(:mob_deploy_stdout, Process.group_leader())" + assert @deploy_task =~ "IO.puts(Process.get(:mob_deploy_stdout, :standard_io), json)" + end + + test "a native-build failure still emits a document" do + # The one path where a caller most needs the result was the one that + # produced none, because the emit sat after the early raise. + assert @deploy_task =~ + ~r/emit_json\(opts, \[\], \[\], \[\], "Native build failed"\)\s*Mix\.raise/ end end end