Fix the deploy regression strict parsing shipped: --beam-flags no longer works

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.

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 <id>` — target a specific device; use `mix mob.devices` to find IDs
+ * `-d`, `--device <id>` — target a specific device; use `mix mob.devices` to find IDs
  * `--dist-port <N>` — 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 <udid>` — `-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 """

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

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

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