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