From 36c7eeedfdb4a38642ff32bec37134efcdb1310c Mon Sep 17 00:00:00 2001 From: GenericJam Date: Sat, 5 Sep 2026 00:59:43 -0600 Subject: [PATCH] Fix the deploy regression strict parsing shipped: --beam-flags no longer works MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A post-hoc review of #63 — which I merged without one — found that switching `mix mob.deploy` to strict parsing broke the documented invocation. `OptionParser` will not consume a dash-prefixed argument as a `:string` value, so `--beam-flags "-S 4:4 -A 4"` parses as two unknown options. BEAM flags essentially all begin with a dash, so this is not an edge case: it is every spelling this repo prints, in seven places — the task's own moduledoc, five README recipes, and the "untuned variant" step of both battery benchmarks. Under the old lenient parsing the value was silently dropped and the deploy carried on with whatever `mob.exs` held, which is its own bug. Strict parsing turned that into a hard abort whose message named `--beam-flags` — a valid option — as unknown. So the fix for one silent failure produced a loud wrong one, and shipped because I skipped the review on a change I had filed mentally as small. `--flag value` is now rewritten to `--flag=value` when the value starts with a dash, for string-valued switches only. Doing it for every switch would swallow the flag after a boolean. Three more from the same review: The error conflated "unrecognised flag" with "recognised flag, unparseable value" and discarded the value, so `--schedulers abc` reported `--schedulers` as unknown and sent the reader to a help page that lists it. `mob.new_plugin` already handled this correctly; this now matches. Positional arguments were still silently dropped. `mix mob.deploy --native ABC123` — a natural fumble of `--device` — parsed cleanly and deployed to every device, which is the identical symptom to the bug #63 set out to fix, in the same task, left in place by it. The four tests #63 shipped were all source-text assertions and none ran the parser, so none could have caught any of this. There is now a table test over every invocation the docs actually print, which fails if a switch is renamed or a documented spelling stops working. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 24 +++++ lib/mix/tasks/mob.deploy.ex | 77 ++++++++++++++-- test/mix/tasks/mob_deploy_beam_flags_test.exs | 2 +- test/mix/tasks/mob_deploy_parsing_test.exs | 92 +++++++++++++++++++ test/mob_dev/wiring_test.exs | 7 +- 5 files changed, 192 insertions(+), 10 deletions(-) create mode 100644 test/mix/tasks/mob_deploy_parsing_test.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c7be33..7a9881a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,30 @@ Full module documentation: [hexdocs.pm/mob_dev](https://hexdocs.pm/mob_dev). ## [Unreleased] +### Changed + +- **`mix mob.deploy` rejects unrecognised options instead of ignoring them.** + A run that passed an extra or misspelled flag used to carry on regardless; + it now fails and names the option. **If you have a wrapper script, CI step + or shell alias passing a flag this task does not accept, it will start + failing.** It also takes no positional arguments — `mix mob.deploy --native + ABC123` deployed to every device and said nothing, and now refuses. + +### Fixed + +- **`--beam-flags "-S 4:4 -A 4"` aborted the deploy.** `OptionParser` will not + consume a dash-prefixed argument as a string value, so the spelling this + repo prints in seven places — the moduledoc, five README recipes and both + battery-bench workflows — parsed as two unknown options. Under the previous + lenient parsing the value was silently dropped and the deploy carried on + with whatever `mob.exs` held; strict parsing turned that into a hard failure + whose message named `--beam-flags` itself as unknown. Both spellings work + now. +- **The error said "Unknown option(s)" for options that are known.** + `--schedulers abc` reported `--schedulers` as unknown and discarded the + value, sending the reader to a help page that lists it. It now distinguishes + an unrecognised flag from a bad value and prints both. + ### Added - **`mix mob.mutate`** — mutation testing for the lines a branch changed. diff --git a/lib/mix/tasks/mob.deploy.ex b/lib/mix/tasks/mob.deploy.ex index 1b79bc2..9d2e2f3 100644 --- a/lib/mix/tasks/mob.deploy.ex +++ b/lib/mix/tasks/mob.deploy.ex @@ -23,7 +23,7 @@ defmodule Mix.Tasks.Mob.Deploy do * `--native` — build native binaries before pushing BEAMs * `--no-restart` — push BEAMs but don't restart the app - * `--device ` — target a specific device; use `mix mob.devices` to find IDs + * `-d`, `--device ` — target a specific device; use `mix mob.devices` to find IDs * `--dist-port ` — pin the BEAM dist listen port (default: auto-allocated per device, `9100 + index`). Use to resolve EPMD collisions when multiple sims/emulators are running the same app concurrently @@ -179,8 +179,10 @@ defmodule Mix.Tasks.Mob.Deploy do @impl Mix.Task def run(args) do - {opts, _argv, invalid} = - OptionParser.parse(args, strict: @switches, aliases: [d: :device]) + {opts, argv, invalid} = + args + |> join_dashed_values() + |> OptionParser.parse(strict: @switches, aliases: [d: :device]) # `switches:` silently discards anything it does not recognise, so # `mix mob.deploy -d ` — `-d` was never aliased here, though @@ -192,6 +194,17 @@ defmodule Mix.Tasks.Mob.Deploy do Mix.raise(invalid_options_message(invalid)) end + # `mix mob.deploy --native ABC123` — a natural fumble of `--device` — parsed + # cleanly, deployed to every device, and said nothing. This task takes no + # positional arguments, so tolerating them is the same silent-ignore the + # strict parsing above was added to end. + unless argv == [] do + Mix.raise( + "mix mob.deploy takes no positional arguments, got: #{Enum.join(argv, ", ")}\n\n" <> + "Did you mean `--device #{hd(argv)}`?" + ) + end + # 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 @@ -421,6 +434,47 @@ defmodule Mix.Tasks.Mob.Deploy do end end + @doc false + @spec switches() :: keyword() + def switches, do: @switches + + @doc """ + Rewrite `--flag value` to `--flag=value` when the value starts with a dash. + + `OptionParser` will not consume a dash-prefixed argument as a `:string` + value, so `--beam-flags "-S 4:4 -A 4"` — the spelling this repo prints in + seven places, including the README and both battery-bench workflows — parsed + as two unknown options. Under the old lenient parsing the value was silently + dropped and the deploy carried on with whatever `mob.exs` held; under strict + parsing it became a hard failure that named a valid option as unknown. + + BEAM flags essentially all start with a dash, so this is not an edge case: + it is the documented invocation. + """ + @spec join_dashed_values([String.t()]) :: [String.t()] + def join_dashed_values(args), do: join_dashed(args, []) + + defp join_dashed([], acc), do: Enum.reverse(acc) + + defp join_dashed(["--" | rest], acc), do: Enum.reverse(acc) ++ ["--" | rest] + + defp join_dashed([flag, value | rest], acc) do + if string_switch?(flag) and String.starts_with?(value, "-") do + join_dashed(rest, ["#{flag}=#{value}" | acc]) + else + join_dashed([value | rest], [flag | acc]) + end + end + + defp join_dashed([last], acc), do: Enum.reverse([last | acc]) + + # Only the switches whose value is free text can legitimately begin with a + # dash. Doing this for every switch would swallow `--android --ios`. + defp string_switch?("--" <> name), + do: Keyword.get(@switches, String.to_atom(String.replace(name, "-", "_"))) == :string + + defp string_switch?(_), do: false + @doc """ The error for options the task does not accept. @@ -429,11 +483,20 @@ defmodule Mix.Tasks.Mob.Deploy do """ @spec invalid_options_message([{String.t(), String.t() | nil}]) :: String.t() def invalid_options_message(invalid) do - names = invalid |> Enum.map(&elem(&1, 0)) |> Enum.join(", ") + # `OptionParser`'s `invalid` list conflates "unrecognised flag" with + # "recognised flag, unparseable value". Reporting both as "unknown" sends + # someone who typed `--schedulers abc` to a help page that lists + # `--schedulers`, with the offending value discarded and nothing to go on. + # `mob.new_plugin` already got this right; this now matches it. + names = + Enum.map_join(invalid, ", ", fn + {flag, nil} -> flag + {flag, value} -> "#{flag} #{value}" + end) - "Unknown option(s): #{names}\n\n" <> - "Run `mix help mob.deploy` for the accepted options. " <> - "Short forms are not aliases except `-d` for `--device`." + "Unrecognized or invalid option(s): #{names}\n\n" <> + "Run `mix help mob.deploy` for the accepted options.\n" <> + "A value beginning with `-` needs the equals form: --beam-flags=\"-S 4:4\"." end @doc """ diff --git a/test/mix/tasks/mob_deploy_beam_flags_test.exs b/test/mix/tasks/mob_deploy_beam_flags_test.exs index be032e5..8dcb8a6 100644 --- a/test/mix/tasks/mob_deploy_beam_flags_test.exs +++ b/test/mix/tasks/mob_deploy_beam_flags_test.exs @@ -475,7 +475,7 @@ defmodule Mix.Tasks.Mob.DeployBeamFlagsTest do assert message =~ "-d" assert message =~ "--devcie" - assert message =~ "Unknown option" + assert message =~ "Unrecognized or invalid option" end end end diff --git a/test/mix/tasks/mob_deploy_parsing_test.exs b/test/mix/tasks/mob_deploy_parsing_test.exs new file mode 100644 index 0000000..ed11d26 --- /dev/null +++ b/test/mix/tasks/mob_deploy_parsing_test.exs @@ -0,0 +1,92 @@ +defmodule Mix.Tasks.Mob.DeployParsingTest do + @moduledoc """ + Every invocation this repo prints at a user must actually parse. + + Switching `mix mob.deploy` to strict parsing turned a silent drop into a hard + failure, which was the point — but it also broke + `--beam-flags "-S 4:4 -A 4"`, the spelling documented in seven places + including the README and both battery-bench workflows. The four tests that + shipped with that change were all source-text assertions and none of them + ran the parser, so none could have caught it. + """ + use ExUnit.Case, async: true + + alias Mix.Tasks.Mob.Deploy + + defp parse(argv) do + argv + |> Deploy.join_dashed_values() + |> OptionParser.parse(strict: Deploy.switches(), aliases: [d: :device]) + end + + describe "the documented invocations" do + # Lifted from the moduledoc, README and the two battery-bench tasks. If a + # switch is renamed or dropped, this fails rather than the docs going stale + # in silence. + @documented [ + ~w(--native), + ~w(--no-restart), + ~w(--slim), + ~w(--android), + ~w(--ios), + ~w(--native --android), + ~w(--device ABC123), + ~w(-d ABC123), + ~w(--dist-port 9200), + ~w(--node-suffix sim1), + ~w(--schedulers 4), + ~w(--json), + ["--beam-flags", ""], + ["--beam-flags", "-S 4:4 -A 4"], + ["--beam-flags", "+S 4:4"], + ["--beam-flags=-S 4:4 -A 4"], + ["--beam-flags", "-S 4:4 -A 8", "--android"] + ] + + for argv <- @documented do + test "parses #{inspect(argv)}" do + assert {_opts, [], []} = parse(unquote(argv)) + end + end + + test "a dash-prefixed BEAM flag keeps its value intact" do + # The regression: OptionParser will not consume a dash-prefixed argument + # as a :string value, so this parsed as two unknown options and aborted a + # run the README tells you to make. + assert {[beam_flags: "-S 4:4 -A 4"], [], []} = parse(["--beam-flags", "-S 4:4 -A 4"]) + end + + test "joining only applies to switches whose value is free text" do + # Doing it for every switch would swallow the flag after a boolean. + assert {[android: true, ios: true], [], []} = parse(~w(--android --ios)) + assert {[native: true, device: "X"], [], []} = parse(~w(--native --device X)) + end + + test "a `--` separator still ends option parsing" do + # OptionParser consumes the separator itself and returns the rest as + # positional. The task rejects positionals, so this is a loud failure + # rather than a silent deploy-to-everything — which is the point. + assert {[native: true], ["-S"], []} = parse(~w(--native -- -S)) + end + end + + describe "what it still refuses" do + test "an unrecognised flag" do + assert {_, _, [{"--devcie", _}]} = parse(~w(--devcie X)) + end + + test "a recognised flag with an unparseable value is named with its value" do + # Reporting `--schedulers abc` as merely "unknown" sends the user to a + # help page that lists --schedulers, with the bad value discarded. + assert {_, _, invalid} = parse(~w(--schedulers abc)) + + message = Deploy.invalid_options_message(invalid) + assert message =~ "--schedulers abc" + assert message =~ "Unrecognized or invalid" + end + + test "the message points at the equals form for dashed values" do + assert Deploy.invalid_options_message([{"--beam-flags", nil}]) =~ "--beam-flags=" + end + end +end diff --git a/test/mob_dev/wiring_test.exs b/test/mob_dev/wiring_test.exs index 7aab467..b9e9f04 100644 --- a/test/mob_dev/wiring_test.exs +++ b/test/mob_dev/wiring_test.exs @@ -197,8 +197,11 @@ defmodule MobDev.WiringTest do # restores the silent-ignore. source = @deploy_task - assert source =~ "OptionParser.parse(args, strict: @switches, aliases: [d: :device])" - refute source =~ "OptionParser.parse(args, switches: @switches)" + # Fragments, not the whole line: this call grew a `join_dashed_values/1` + # step and `mix format` rewrapped it, which would have broken an + # exact-line assertion with no behaviour change. + assert source =~ "strict: @switches, aliases: [d: :device]" + refute source =~ "switches: @switches" end test "the run raises rather than proceeding" do