From 660b2889be6a823bd184b5713b98a94a68455123 Mon Sep 17 00:00:00 2001 From: Dave Lucia Date: Wed, 27 May 2026 18:57:49 -0400 Subject: [PATCH 1/2] feat(suite): line-range skipping for Lua 5.3 official tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace whole-file `@tag :skip` with per-file line ranges. Each suite file can now run partially, with only the failing lines skipped, so every assertion outside the skipped range contributes to the green signal. The skip list itself becomes the conformance backlog ahead of 1.0.0. - `Lua.SuiteRunner.run_file/2` accepts `:skip_ranges`, replacing each skipped line with a one-line comment to preserve line numbers (load-bearing for `Lua.VM.AssertionError`). - `test/lua53_skips.exs` holds the per-file skip map. Each entry pairs a line range with a category, one-line reason, and optional GitHub issue number. `lines: :all` marks files awaiting initial triage. - `test/lua53_suite_test.exs` generates one ExUnit test per suite file from the skip map. Test names carry the conformance debt count (e.g. `pm.lua (47 lines skipped, 3 ranges)`). - `mix lua.suite --status` summarises the conformance backlog (totals by category and tracking issue). - `mix lua.suite --audit` re-runs each file with each entry omitted to surface stale ranges and promotion candidates. - Triage skill rewritten so §6.C produces a skip-range entry instead of a whole-file skip. Documents range edge cases (multi-line strings, local-decl coupling, if/then/end). Zero behavior change for the existing suite: same 6 ready files pass, the 19 previously-skipped files keep `lines: :all` (still tagged `:skip`), and the 4 permanently-deferred files (filesystem I/O, subprocess) stay in their own `@deferred_permanent` attribute. --- .agents/skills/triage-suite-failure/SKILL.md | 77 +++++-- tasks/lua.suite.ex | 199 ++++++++++++++++++- tasks/suite_runner.ex | 36 +++- test/lua53_skips.exs | 109 ++++++++++ test/lua53_suite_test.exs | 51 +++-- test/mix/tasks/lua.suite_test.exs | 83 ++++++++ test/suite_runner_test.exs | 100 ++++++++++ test/support/lua_test_case.ex | 4 +- 8 files changed, 617 insertions(+), 42 deletions(-) create mode 100644 test/lua53_skips.exs create mode 100644 test/suite_runner_test.exs diff --git a/.agents/skills/triage-suite-failure/SKILL.md b/.agents/skills/triage-suite-failure/SKILL.md index 62c1c67..4917c92 100644 --- a/.agents/skills/triage-suite-failure/SKILL.md +++ b/.agents/skills/triage-suite-failure/SKILL.md @@ -27,8 +27,8 @@ turning that understanding into a shippable plan. - Reducing it to a 5–20 line repro that lives in `test/lua/vm/`. - Producing one of three outputs: 1. A new plan file under `.agents/plans/` if the fix is shippable. - 2. An `@tag :skip` annotation in `test/lua53_suite_test.exs` with a clear - comment explaining the deferral, plus a deferred-issue label. + 2. A line-range skip entry in `test/lua53_skips.exs` with a one-line + reason and (optionally) a tracking issue. See §6.C. 3. An update to an existing plan if this failure is part of an ongoing fix. ## What this skill is NOT for @@ -159,26 +159,68 @@ Required sections: Then open a corresponding GitHub issue and link them via `issue:` frontmatter field. -#### C. Defer (out of scope for current direction) +#### C. Defer with a line-range skip -The bug is real but fixing it is weeks of work (coroutines, GC, full goto -CFG, file I/O, etc.). - -In `test/lua53_suite_test.exs`: +The bug is real but fixing it is out of scope for the current cycle +(coroutines, GC, full goto CFG, etc.). Don't tag the whole suite file +`@tag :skip` — that throws away signal from every other assertion in +the file. Instead, add a *line-range* entry to `test/lua53_skips.exs`: ```elixir -# Deferred: backward goto requires CFG pass in compiler. -# See .agents/plans/A-goto-cfg.md (when written) or ROADMAP.md "Deferred". -@tag :skip -test "goto.lua" do ... end +%{ + "pm.lua" => [ + %{lines: 245..289, category: :stdlib, reason: "string.gmatch frontier %f not implemented", issue: 312} + ] +} +``` + +Entry fields: + +- `:lines` — a `Range` covering the failing statement and any + dependent state. Use `:all` only for whole-file deferrals awaiting + initial triage (existing seed entries do this; you should replace + one with a real range). +- `:category` — pick one from the table in §5 of this skill + (`:lexer | :parser | :codegen | :executor | :stdlib | :unimplemented | :semantic`). +- `:reason` — one line, stands alone. Do **not** reference plan ids + (plans are ephemeral per CLAUDE.md repo conventions). +- `:issue` — optional GitHub issue number, or `nil`. Issues are + durable; use them when there is a tracked fix. + +**Pick the smallest range that contains the failure.** The whole +failing `assert(...)` line if it stands alone; the enclosing `do ... +end` block if assertions depend on earlier `local`s. Then scan the +range for other `assert(...)` calls — if there are more, either name +them in `:reason` or split into multiple entries. Broad ranges +silently hide bugs. + +**Edge cases that make a range fail to parse:** + +- Multi-line strings `[[...]]` and long comments `--[[...]]` — both + delimiters must be inside or outside the range; never split. +- `local x = ...` referenced later in the file — skipping the + declaration breaks the downstream `x`; widen the range to cover + those uses too. +- `if/then/.../end` — `then` block is skippable but `if`, `then`, + and `end` must survive. (Lua allows empty `then`.) + +If your range crosses a syntactic boundary, the suite test fails +with a parse error pointing near the boundary; widen and re-run. + +**Re-run the file after adding the range:** + +``` +mix test test/lua53_suite_test.exs --only lua53 ``` -Leave the unit test from step 4 in place — even if the suite file is -skipped, the unit test documents the bug and will catch a regression if a -future change accidentally fixes it. +Confirm: the file either passes (test name now reads +`pm.lua (47 lines skipped, 3 ranges)`) or progresses to a new, +well-located failure. If a parse error appeared, the range crossed a +boundary — widen. -If there's no deferred-tracking issue yet, open one with label `defer` and -reference it from the comment. +Leave the unit test from step 4 in place — even if the suite range is +skipped, the unit test documents the bug and will catch a regression +if a future change accidentally fixes it. ### 7. Output a triage report @@ -195,7 +237,8 @@ Classification: Decision: fix-in-plan | defer | follow-up If fix-in-plan: created .agents/plans/-.md -If defer: tagged @skip in test/lua53_suite_test.exs with reason +If defer: added range to test/lua53_skips.exs (issue #) +Lines skipped: ``` ## Conventions diff --git a/tasks/lua.suite.ex b/tasks/lua.suite.ex index 91c6d11..b7238f4 100644 --- a/tasks/lua.suite.ex +++ b/tasks/lua.suite.ex @@ -24,6 +24,8 @@ defmodule Mix.Tasks.Lua.Suite do mix lua.suite --filter math mix lua.suite --dir test/lua53_tests mix lua.suite --verbose + mix lua.suite --status + mix lua.suite --audit ## Options @@ -35,6 +37,11 @@ defmodule Mix.Tasks.Lua.Suite do exceed it are reported as `timeout` (default: `30000`). * `--verbose` — Print the full error message for each failing file, not just the first line. + * `--status` — Print a per-file conformance summary from + `test/lua53_skips.exs`. Fast, no tests run. + * `--audit` — For each skip entry, re-run the file with that + entry removed and report whether it is stale (file passes + without it). Slow. ## Output @@ -59,15 +66,33 @@ defmodule Mix.Tasks.Lua.Suite do use Mix.Task + @skip_file "test/lua53_skips.exs" + @impl Mix.Task def run(args) do Mix.Task.run("app.start") {opts, _positional} = OptionParser.parse!(args, - strict: [dir: :string, filter: :string, timeout: :integer, verbose: :boolean] + strict: [ + dir: :string, + filter: :string, + timeout: :integer, + verbose: :boolean, + status: :boolean, + audit: :boolean, + skip_file: :string + ] ) + cond do + Keyword.get(opts, :status, false) -> run_status(opts) + Keyword.get(opts, :audit, false) -> run_audit(opts) + true -> run_suite(opts) + end + end + + defp run_suite(opts) do dir = Keyword.get(opts, :dir, "test/lua53_tests") filter = Keyword.get(opts, :filter) timeout = Keyword.get(opts, :timeout, 30_000) @@ -181,4 +206,176 @@ defmodule Mix.Tasks.Lua.Suite do |> String.split("\n", parts: 2) |> List.first() end + + # --- --status --------------------------------------------------------- + + defp run_status(opts) do + skip_map = load_skip_map!(Keyword.get(opts, :skip_file, @skip_file)) + files = Map.keys(skip_map) |> Enum.sort() + + if files == [] do + Mix.shell().info("No skip entries — suite is fully ranged.") + else + width = files |> Enum.map(&String.length/1) |> Enum.max() + + total_lines = + skip_map + |> Map.values() + |> List.flatten() + |> Enum.reject(&(&1.lines == :all)) + |> Enum.reduce(0, fn e, acc -> Enum.count(e.lines) + acc end) + + total_ranges = + skip_map + |> Map.values() + |> List.flatten() + |> Enum.reject(&(&1.lines == :all)) + |> length() + + all_count = + skip_map + |> Map.values() + |> List.flatten() + |> Enum.filter(&(&1.lines == :all)) + |> length() + + Enum.each(files, fn file -> + entries = Map.get(skip_map, file) + all? = Enum.any?(entries, &(&1.lines == :all)) + + if all? do + Mix.shell().info(" #{String.pad_trailing(file, width + 2)}:all pending triage") + else + lines = Enum.reduce(entries, 0, fn e, acc -> Enum.count(e.lines) + acc end) + cats = entries |> Enum.map(& &1.category) |> Enum.frequencies() + cats_str = cats |> Enum.map(fn {c, n} -> "#{c}×#{n}" end) |> Enum.join(", ") + Mix.shell().info(" #{String.pad_trailing(file, width + 2)}#{lines} lines, #{length(entries)} ranges (#{cats_str})") + end + end) + + Mix.shell().info("") + Mix.shell().info("Total: #{total_lines} skipped lines across #{total_ranges} ranges in #{length(files) - all_count} files. #{all_count} files pending initial triage.") + + categories = + skip_map + |> Map.values() + |> List.flatten() + |> Enum.reject(&(&1.lines == :all)) + |> Enum.map(& &1.category) + |> Enum.frequencies() + + if categories != %{} do + cats_str = categories |> Enum.sort_by(fn {_, n} -> -n end) |> Enum.map(fn {c, n} -> "#{c} #{n}" end) |> Enum.join(", ") + Mix.shell().info("By category: #{cats_str}") + end + + issues = + skip_map + |> Map.values() + |> List.flatten() + |> Enum.reject(&(&1.lines == :all)) + |> Enum.map(& &1.issue) + + with_issue = Enum.count(issues, &(&1 != nil)) + Mix.shell().info("By issue: #{with_issue} ranges linked, #{length(issues) - with_issue} unassigned.") + end + end + + # --- --audit ---------------------------------------------------------- + + defp run_audit(opts) do + dir = Keyword.get(opts, :dir, "test/lua53_tests") + timeout = Keyword.get(opts, :timeout, 30_000) + skip_map = load_skip_map!(Keyword.get(opts, :skip_file, @skip_file)) + + files = Map.keys(skip_map) |> Enum.sort() + total_entries = + skip_map |> Map.values() |> List.flatten() |> length() + + Mix.shell().info("Auditing #{total_entries} skip entries across #{length(files)} files...") + Mix.shell().info("") + + {stale, candidates} = + Enum.reduce(files, {0, 0}, fn file, {stale_acc, cand_acc} -> + entries = Map.get(skip_map, file) + path = Path.join(dir, file) + cond do + Enum.any?(entries, &(&1.lines == :all)) -> + case run_with_ranges(path, [], timeout) do + :ok -> + report(file, nil, "CANDIDATE", "file passes with no ranges — try promoting") + {stale_acc, cand_acc + 1} + + {:error, e} -> + line = error_line(e) || "?" + report(file, nil, "ACTIVE", ":all entry, first failure at line #{line}") + {stale_acc, cand_acc} + + :timeout -> + report(file, nil, "TIMEOUT", "exceeded #{timeout}ms with no ranges") + {stale_acc, cand_acc} + end + + true -> + Enum.reduce(entries, {stale_acc, cand_acc}, fn entry, {s, c} -> + others = Enum.reject(entries, &(&1 == entry)) |> Enum.map(& &1.lines) + + case run_with_ranges(path, others, timeout) do + :ok -> + report(file, entry.lines, "STALE", "file passes without this range — try removing") + {s + 1, c} + + {:error, e} -> + new_line = error_line(e) + if new_line && new_line not in entry.lines do + report(file, entry.lines, "MOVED", "failure now at line #{new_line} — consider narrowing") + {s, c} + else + {s, c} + end + + :timeout -> + report(file, entry.lines, "TIMEOUT", "exceeded #{timeout}ms without this range") + {s, c} + end + end) + end + end) + + Mix.shell().info("") + Mix.shell().info("#{stale} stale entries, #{candidates} promotion candidates.") + end + + defp run_with_ranges(path, ranges, timeout) do + task = Task.async(fn -> Lua.SuiteRunner.run_file(path, skip_ranges: ranges) end) + + case Task.yield(task, timeout) || Task.shutdown(task, :brutal_kill) do + {:ok, :ok} -> :ok + {:ok, {:error, e}} -> {:error, e} + _ -> :timeout + end + end + + defp error_line(e) when is_exception(e), do: Map.get(e, :line) + defp error_line(_), do: nil + + defp report(file, nil, status, msg) do + Mix.shell().info(" #{String.pad_trailing(file, 24)}#{String.pad_trailing(status, 12)}#{msg}") + end + + defp report(file, range, status, msg) do + label = "#{file}:#{range_to_string(range)}" + Mix.shell().info(" #{String.pad_trailing(label, 24)}#{String.pad_trailing(status, 12)}#{msg}") + end + + defp range_to_string(%Range{first: a, last: a}), do: "#{a}" + defp range_to_string(%Range{first: a, last: b}), do: "#{a}..#{b}" + + defp load_skip_map!(path) do + if !File.exists?(path) do + Mix.raise("#{path} not found — nothing to report.") + end + + path |> Code.eval_file() |> elem(0) + end end diff --git a/tasks/suite_runner.ex b/tasks/suite_runner.ex index ec05905..3f9e84a 100644 --- a/tasks/suite_runner.ex +++ b/tasks/suite_runner.ex @@ -36,12 +36,23 @@ defmodule Lua.SuiteRunner do Returns `:ok` on success, `{:error, exception}` if the file raises. No exceptions escape this function. + ## Options + + * `:skip_ranges` — a list of `Range` values. Each line in any of + the ranges is replaced with a one-line comment before the source + is evaluated. Line numbers are preserved, so assertion errors + still report the original source position. + Use `prepare/1` if you need to inspect or modify the VM before running. """ - @spec run_file(Path.t()) :: :ok | {:error, Exception.t()} - def run_file(path) do - source = File.read!(path) + @spec run_file(Path.t(), keyword) :: :ok | {:error, Exception.t()} + def run_file(path, opts \\ []) do + source = + path + |> File.read!() + |> apply_skip_ranges(Keyword.get(opts, :skip_ranges, [])) + lua = prepare(Path.dirname(path)) try do @@ -52,6 +63,25 @@ defmodule Lua.SuiteRunner do end end + @doc """ + Replace each line covered by `ranges` with a one-line `--` comment, + preserving total line count. Used by `run_file/2` and the audit + helpers in `Mix.Tasks.Lua.Suite`. + """ + @spec apply_skip_ranges(String.t(), [Range.t()]) :: String.t() + def apply_skip_ranges(source, []), do: source + + def apply_skip_ranges(source, ranges) do + skip = ranges |> Enum.flat_map(&Enum.to_list/1) |> MapSet.new() + + source + |> String.split("\n") + |> Enum.with_index(1) + |> Enum.map_join("\n", fn {line, n} -> + if MapSet.member?(skip, n), do: "-- skipped (suite triage): was line #{n}", else: line + end) + end + defp add_test_paths(lua, test_dir) do Lua.set_lua_paths(lua, [ Path.join(test_dir, "?.lua"), diff --git a/test/lua53_skips.exs b/test/lua53_skips.exs new file mode 100644 index 0000000..db4c516 --- /dev/null +++ b/test/lua53_skips.exs @@ -0,0 +1,109 @@ +# Lua 5.3 official suite skip list. +# +# Loaded by `test/lua53_suite_test.exs` via `Code.eval_file/1`. Each +# key is a suite filename in `test/lua53_tests/`; each value is a list +# of skip entries. A file absent from this map runs unmodified. +# +# Entry shape: +# +# %{ +# lines: 12..58, # a Range, or the atom :all +# category: :parser, # :lexer | :parser | :codegen | :executor +# # | :stdlib | :unimplemented | :semantic +# reason: "one-line cause", # stands alone, no plan-id references +# issue: 287 # optional GitHub issue number, or nil +# } +# +# Use `lines: :all` for files awaiting initial triage. The test driver +# tags those `@tag :skip`. Files with specific ranges run with those +# lines commented out (line numbers preserved for error reporting). +# +# Run `mix lua.suite --status` for a quick conformance snapshot, or +# `mix lua.suite --audit` to find stale entries that no longer need +# to exist. +# +# The four permanently-deferred files (main.lua, files.lua, +# attrib.lua, verybig.lua) do NOT appear here — they live in +# `@deferred_permanent` in `test/lua53_suite_test.exs` because they +# exercise filesystem I/O and subprocess invocation that we will +# never support in an embedded sandboxed VM. + +%{ + "all.lua" => [ + %{lines: :all, category: :unimplemented, reason: "upstream harness file, pending initial triage", issue: nil} + ], + "big.lua" => [ + %{ + lines: :all, + category: :unimplemented, + reason: "pending initial triage (suspected timeout per ROADMAP A10)", + issue: nil + } + ], + "calls.lua" => [ + %{lines: :all, category: :unimplemented, reason: "pending initial triage", issue: nil} + ], + "closure.lua" => [ + %{ + lines: :all, + category: :unimplemented, + reason: "pending initial triage (suspected timeout per ROADMAP A10)", + issue: nil + } + ], + "constructs.lua" => [ + %{lines: :all, category: :unimplemented, reason: "pending initial triage", issue: nil} + ], + "coroutine.lua" => [ + %{lines: :all, category: :unimplemented, reason: "coroutines not implemented", issue: nil} + ], + "db.lua" => [ + %{lines: :all, category: :unimplemented, reason: "full debug library not implemented", issue: nil} + ], + "errors.lua" => [ + %{lines: :all, category: :unimplemented, reason: "pending initial triage", issue: nil} + ], + "events.lua" => [ + %{lines: :all, category: :unimplemented, reason: "pending initial triage", issue: nil} + ], + "gc.lua" => [ + %{lines: :all, category: :unimplemented, reason: "garbage collection / weak tables not implemented", issue: nil} + ], + "goto.lua" => [ + %{ + lines: :all, + category: :unimplemented, + reason: "backward goto / goto-out-of-conditional not implemented", + issue: nil + } + ], + "literals.lua" => [ + %{lines: :all, category: :unimplemented, reason: "pending initial triage", issue: nil} + ], + "locals.lua" => [ + %{lines: :all, category: :unimplemented, reason: "pending initial triage", issue: nil} + ], + "math.lua" => [ + %{lines: :all, category: :unimplemented, reason: "pending initial triage", issue: nil} + ], + "nextvar.lua" => [ + %{lines: :all, category: :unimplemented, reason: "pending initial triage", issue: nil} + ], + "pm.lua" => [ + %{lines: :all, category: :unimplemented, reason: "pending initial triage (pattern engine work in A9)", issue: nil} + ], + "sort.lua" => [ + %{lines: :all, category: :unimplemented, reason: "pending initial triage", issue: nil} + ], + "strings.lua" => [ + %{lines: :all, category: :unimplemented, reason: "pending initial triage", issue: nil} + ], + "utf8.lua" => [ + %{ + lines: :all, + category: :unimplemented, + reason: "pending initial triage (suspected timeout per ROADMAP A10)", + issue: nil + } + ] +} diff --git a/test/lua53_suite_test.exs b/test/lua53_suite_test.exs index 58d1b2e..40e8793 100644 --- a/test/lua53_suite_test.exs +++ b/test/lua53_suite_test.exs @@ -6,6 +6,7 @@ defmodule Lua.Lua53SuiteTest do @moduletag :lua53 @test_dir "test/lua53_tests" + @skip_file "test/lua53_skips.exs" # Dynamically generate test cases for all .lua files in the test directory. # This ensures we don't miss any tests as the suite evolves. @@ -14,8 +15,9 @@ defmodule Lua.Lua53SuiteTest do |> Enum.filter(&String.ends_with?(&1, ".lua")) |> Enum.sort() - # Tests that are ready to run (not skipped). - @ready_tests ["simple_test.lua", "api.lua", "bitwise.lua", "code.lua", "tpack.lua", "vararg.lua"] + # Per-file skip ranges. See test/lua53_skips.exs for shape and conventions. + @external_resource @skip_file + @skip_map @skip_file |> Code.eval_file() |> elem(0) # Suite files that we have deliberately decided not to support. # @@ -50,25 +52,36 @@ defmodule Lua.Lua53SuiteTest do "verybig.lua" => "tests dofile()-of-tmpfile harness for >64K constants" } - # Tests that require features not yet implemented. As we implement - # features, move tests from here to @ready_tests. - @skipped_tests (@lua_files -- @ready_tests) -- Map.keys(@deferred_permanent) + @runnable_files @lua_files -- Map.keys(@deferred_permanent) - describe "Lua 5.3 Test Suite - Ready Tests" do - for test_file <- @ready_tests do - @test_file test_file - test test_file do - run_lua_file(Path.join(@test_dir, @test_file)) - end - end - end + describe "Lua 5.3 Test Suite" do + for file <- @runnable_files do + entries = Map.get(@skip_map, file, []) + whole_file? = Enum.any?(entries, &(&1.lines == :all)) + ranges = entries |> Enum.reject(&(&1.lines == :all)) |> Enum.map(& &1.lines) - describe "Lua 5.3 Test Suite - Skipped Tests (Missing Features)" do - for test_file <- @skipped_tests do - @test_file test_file - @tag :skip - test test_file do - run_lua_file(Path.join(@test_dir, @test_file)) + @test_file file + @ranges ranges + + cond do + whole_file? -> + @tag :skip + test "#{file} (pending initial triage)" do + run_lua_file(Path.join(@test_dir, @test_file)) + end + + ranges == [] -> + test file do + run_lua_file(Path.join(@test_dir, @test_file)) + end + + true -> + skipped = + Enum.reduce(ranges, 0, fn r, acc -> Enum.count(r) + acc end) + + test "#{file} (#{skipped} lines skipped, #{length(ranges)} ranges)" do + run_lua_file(Path.join(@test_dir, @test_file), skip_ranges: @ranges) + end end end end diff --git a/test/mix/tasks/lua.suite_test.exs b/test/mix/tasks/lua.suite_test.exs index 40f07c1..de77c50 100644 --- a/test/mix/tasks/lua.suite_test.exs +++ b/test/mix/tasks/lua.suite_test.exs @@ -83,4 +83,87 @@ defmodule Mix.Tasks.Lua.SuiteTest do end end end + + describe "--status" do + test "summarises a fixture skip file with totals and categories" do + skip_file = Path.join(@tmpdir, "skips.exs") + + File.write!(skip_file, """ + %{ + "a.lua" => [%{lines: :all, category: :unimplemented, reason: "t", issue: nil}], + "b.lua" => [ + %{lines: 1..3, category: :stdlib, reason: "t", issue: 42}, + %{lines: 10..10, category: :executor, reason: "t", issue: nil} + ] + } + """) + + output = capture_io(fn -> Suite.run(["--status", "--skip-file", skip_file]) end) + + assert output =~ "a.lua" + assert output =~ ":all pending triage" + assert output =~ "b.lua" + assert output =~ "stdlib×1" + assert output =~ "executor×1" + assert output =~ "Total: 4 skipped lines across 2 ranges in 1 files. 1 files pending initial triage." + assert output =~ "By category: stdlib 1, executor 1" + assert output =~ "By issue: 1 ranges linked, 1 unassigned." + end + end + + describe "--audit" do + test "reports CANDIDATE when a :all entry's file passes outright" do + write_lua("passing.lua", "return 1\n") + skip_file = Path.join(@tmpdir, "skips.exs") + + File.write!(skip_file, """ + %{"passing.lua" => [%{lines: :all, category: :unimplemented, reason: "t", issue: nil}]} + """) + + output = + capture_io(fn -> + Suite.run(["--audit", "--dir", @tmpdir, "--skip-file", skip_file, "--timeout", "5000"]) + end) + + assert output =~ "passing.lua" + assert output =~ "CANDIDATE" + assert output =~ "1 promotion candidates" + end + + test "reports STALE when a specific range is no longer needed" do + write_lua("a.lua", "local x = 1\nreturn x\n") + skip_file = Path.join(@tmpdir, "skips.exs") + + File.write!(skip_file, """ + %{"a.lua" => [%{lines: 1..1, category: :executor, reason: "t", issue: nil}]} + """) + + output = + capture_io(fn -> + Suite.run(["--audit", "--dir", @tmpdir, "--skip-file", skip_file, "--timeout", "5000"]) + end) + + assert output =~ "a.lua:1" + assert output =~ "STALE" + assert output =~ "1 stale entries" + end + + test "reports ACTIVE when a :all entry's file still fails without ranges" do + write_lua("broken.lua", "assert(false, 'still bad')\n") + skip_file = Path.join(@tmpdir, "skips.exs") + + File.write!(skip_file, """ + %{"broken.lua" => [%{lines: :all, category: :unimplemented, reason: "t", issue: nil}]} + """) + + output = + capture_io(fn -> + Suite.run(["--audit", "--dir", @tmpdir, "--skip-file", skip_file, "--timeout", "5000"]) + end) + + assert output =~ "broken.lua" + assert output =~ "ACTIVE" + assert output =~ "first failure at line 1" + end + end end diff --git a/test/suite_runner_test.exs b/test/suite_runner_test.exs new file mode 100644 index 0000000..130d3ac --- /dev/null +++ b/test/suite_runner_test.exs @@ -0,0 +1,100 @@ +defmodule Lua.SuiteRunnerTest do + use ExUnit.Case, async: true + + alias Lua.SuiteRunner + + @tmpdir Path.join(System.tmp_dir!(), "lua_suite_runner_test") + + setup do + File.rm_rf!(@tmpdir) + File.mkdir_p!(@tmpdir) + on_exit(fn -> File.rm_rf!(@tmpdir) end) + :ok + end + + defp write_lua(name, body) do + path = Path.join(@tmpdir, name) + File.write!(path, body) + path + end + + describe "apply_skip_ranges/2" do + test "returns source unchanged when no ranges are given" do + source = "line one\nline two\nline three\n" + assert SuiteRunner.apply_skip_ranges(source, []) == source + end + + test "comments out lines in a single range, preserving total line count" do + source = "a\nb\nc\nd\ne" + out = SuiteRunner.apply_skip_ranges(source, [2..3]) + + lines = String.split(out, "\n") + assert length(lines) == 5 + assert Enum.at(lines, 0) == "a" + assert String.starts_with?(Enum.at(lines, 1), "--") + assert String.starts_with?(Enum.at(lines, 2), "--") + assert Enum.at(lines, 3) == "d" + assert Enum.at(lines, 4) == "e" + end + + test "supports multiple disjoint ranges" do + source = Enum.map_join(1..10, "\n", &"line#{&1}") + out = SuiteRunner.apply_skip_ranges(source, [2..3, 7..8]) + + lines = String.split(out, "\n") + assert Enum.at(lines, 0) == "line1" + assert String.starts_with?(Enum.at(lines, 1), "--") + assert String.starts_with?(Enum.at(lines, 2), "--") + assert Enum.at(lines, 3) == "line4" + assert Enum.at(lines, 5) == "line6" + assert String.starts_with?(Enum.at(lines, 6), "--") + assert String.starts_with?(Enum.at(lines, 7), "--") + assert Enum.at(lines, 8) == "line9" + end + end + + describe "run_file/2 — line preservation invariant" do + # If apply_skip_ranges ever stops preserving line numbers, this test + # catches it. The whole triage workflow depends on the line in the + # error message matching the line in the source file. + test "assertion error reports the original line number, with and without skip ranges" do + # Eleven lines total; the failing assert is on line 11. + source = """ + -- line 1 (filler) + -- line 2 + -- line 3 + -- line 4 + -- line 5 + -- line 6 + -- line 7 + -- line 8 + -- line 9 + -- line 10 + assert(false, "boom") + """ + + path = write_lua("line_check.lua", source) + + {:error, err_no_skip} = SuiteRunner.run_file(path) + {:error, err_with_skip} = SuiteRunner.run_file(path, skip_ranges: [2..4]) + + assert err_no_skip.line == 11, + "expected failing line to be 11 without skips, got #{inspect(err_no_skip.line)} (#{err_no_skip.__struct__})" + + assert err_with_skip.line == 11, + "expected failing line to remain 11 after skipping 2..4, got #{inspect(err_with_skip.line)} (#{err_with_skip.__struct__})" + end + + test "skipping the failing statement makes the file pass" do + source = """ + local x = 1 + assert(false, "would fail") + """ + + path = write_lua("skipme.lua", source) + + assert {:error, _} = SuiteRunner.run_file(path) + assert :ok = SuiteRunner.run_file(path, skip_ranges: [2..2]) + end + end +end diff --git a/test/support/lua_test_case.ex b/test/support/lua_test_case.ex index cc961a8..dd36d47 100644 --- a/test/support/lua_test_case.ex +++ b/test/support/lua_test_case.ex @@ -10,8 +10,8 @@ defmodule Lua.TestCase do use ExUnit.CaseTemplate @doc false - def run_lua_file(path, _opts \\ []) do - case Lua.SuiteRunner.run_file(path) do + def run_lua_file(path, opts \\ []) do + case Lua.SuiteRunner.run_file(path, opts) do :ok -> :ok {:error, e} -> raise e end From c2cddafb6b75a9f83bf8042ad67214127cd50ab4 Mon Sep 17 00:00:00 2001 From: Dave Lucia Date: Wed, 27 May 2026 19:11:18 -0400 Subject: [PATCH 2/2] refactor(suite): address PR #249 review feedback - Validate skip file: raise when a file mixes `lines: :all` with specific ranges. The test driver treats `:all` as a hard skip and would silently drop the ranges. Loader lives on `Lua.SuiteRunner` so both the Mix task and the test module enforce it. - Audit labels exceptions without a `:line` field (e.g. `Lua.VM.InternalError`, `Lua.CompilerException`) as `unknown line (StructName)` instead of bare `?`. - Collapse four passes over the skip map in `--status` into one reducer; extract per-file printing for clarity. - `range_to_string/1` handles non-default step ranges. --- tasks/lua.suite.ex | 195 +++++++++++++++++------------- tasks/suite_runner.ex | 28 +++++ test/lua53_suite_test.exs | 2 +- test/mix/tasks/lua.suite_test.exs | 40 ++++++ 4 files changed, 177 insertions(+), 88 deletions(-) diff --git a/tasks/lua.suite.ex b/tasks/lua.suite.ex index b7238f4..e8590f9 100644 --- a/tasks/lua.suite.ex +++ b/tasks/lua.suite.ex @@ -211,73 +211,84 @@ defmodule Mix.Tasks.Lua.Suite do defp run_status(opts) do skip_map = load_skip_map!(Keyword.get(opts, :skip_file, @skip_file)) - files = Map.keys(skip_map) |> Enum.sort() + files = skip_map |> Map.keys() |> Enum.sort() if files == [] do Mix.shell().info("No skip entries — suite is fully ranged.") else width = files |> Enum.map(&String.length/1) |> Enum.max() - - total_lines = - skip_map - |> Map.values() - |> List.flatten() - |> Enum.reject(&(&1.lines == :all)) - |> Enum.reduce(0, fn e, acc -> Enum.count(e.lines) + acc end) - - total_ranges = - skip_map - |> Map.values() - |> List.flatten() - |> Enum.reject(&(&1.lines == :all)) - |> length() - - all_count = - skip_map - |> Map.values() - |> List.flatten() - |> Enum.filter(&(&1.lines == :all)) - |> length() + stats = aggregate_skip_stats(skip_map) Enum.each(files, fn file -> entries = Map.get(skip_map, file) - all? = Enum.any?(entries, &(&1.lines == :all)) - - if all? do - Mix.shell().info(" #{String.pad_trailing(file, width + 2)}:all pending triage") - else - lines = Enum.reduce(entries, 0, fn e, acc -> Enum.count(e.lines) + acc end) - cats = entries |> Enum.map(& &1.category) |> Enum.frequencies() - cats_str = cats |> Enum.map(fn {c, n} -> "#{c}×#{n}" end) |> Enum.join(", ") - Mix.shell().info(" #{String.pad_trailing(file, width + 2)}#{lines} lines, #{length(entries)} ranges (#{cats_str})") - end + print_file_status(file, entries, width) end) + file_count = length(files) - stats.all_count + Mix.shell().info("") - Mix.shell().info("Total: #{total_lines} skipped lines across #{total_ranges} ranges in #{length(files) - all_count} files. #{all_count} files pending initial triage.") - categories = - skip_map - |> Map.values() - |> List.flatten() - |> Enum.reject(&(&1.lines == :all)) - |> Enum.map(& &1.category) - |> Enum.frequencies() + Mix.shell().info( + "Total: #{stats.total_lines} skipped lines across #{stats.total_ranges} ranges " <> + "in #{file_count} files. #{stats.all_count} files pending initial triage." + ) + + if stats.categories != %{} do + cats_str = + stats.categories + |> Enum.sort_by(fn {_, n} -> -n end) + |> Enum.map_join(", ", fn {c, n} -> "#{c} #{n}" end) - if categories != %{} do - cats_str = categories |> Enum.sort_by(fn {_, n} -> -n end) |> Enum.map(fn {c, n} -> "#{c} #{n}" end) |> Enum.join(", ") Mix.shell().info("By category: #{cats_str}") end - issues = - skip_map - |> Map.values() - |> List.flatten() - |> Enum.reject(&(&1.lines == :all)) - |> Enum.map(& &1.issue) + unassigned = stats.total_ranges - stats.issues_linked + Mix.shell().info("By issue: #{stats.issues_linked} ranges linked, #{unassigned} unassigned.") + end + end + + defp aggregate_skip_stats(skip_map) do + initial = %{ + total_lines: 0, + total_ranges: 0, + all_count: 0, + categories: %{}, + issues_linked: 0 + } + + skip_map + |> Map.values() + |> List.flatten() + |> Enum.reduce(initial, fn entry, acc -> + if entry.lines == :all do + %{acc | all_count: acc.all_count + 1} + else + %{ + acc + | total_lines: acc.total_lines + Enum.count(entry.lines), + total_ranges: acc.total_ranges + 1, + categories: Map.update(acc.categories, entry.category, 1, &(&1 + 1)), + issues_linked: acc.issues_linked + if(entry.issue, do: 1, else: 0) + } + end + end) + end - with_issue = Enum.count(issues, &(&1 != nil)) - Mix.shell().info("By issue: #{with_issue} ranges linked, #{length(issues) - with_issue} unassigned.") + defp print_file_status(file, entries, width) do + label = String.pad_trailing(file, width + 2) + + if Enum.any?(entries, &(&1.lines == :all)) do + Mix.shell().info(" #{label}:all pending triage") + else + lines = Enum.reduce(entries, 0, fn e, acc -> Enum.count(e.lines) + acc end) + + cats_str = + entries + |> Enum.map(& &1.category) + |> Enum.frequencies() + |> Enum.map_join(", ", fn {c, n} -> "#{c}×#{n}" end) + + Mix.shell().info(" #{label}#{lines} lines, #{length(entries)} ranges (#{cats_str})") end end @@ -288,7 +299,8 @@ defmodule Mix.Tasks.Lua.Suite do timeout = Keyword.get(opts, :timeout, 30_000) skip_map = load_skip_map!(Keyword.get(opts, :skip_file, @skip_file)) - files = Map.keys(skip_map) |> Enum.sort() + files = skip_map |> Map.keys() |> Enum.sort() + total_entries = skip_map |> Map.values() |> List.flatten() |> length() @@ -299,46 +311,45 @@ defmodule Mix.Tasks.Lua.Suite do Enum.reduce(files, {0, 0}, fn file, {stale_acc, cand_acc} -> entries = Map.get(skip_map, file) path = Path.join(dir, file) - cond do - Enum.any?(entries, &(&1.lines == :all)) -> - case run_with_ranges(path, [], timeout) do + + if Enum.any?(entries, &(&1.lines == :all)) do + case run_with_ranges(path, [], timeout) do + :ok -> + report(file, nil, "CANDIDATE", "file passes with no ranges — try promoting") + {stale_acc, cand_acc + 1} + + {:error, e} -> + report(file, nil, "ACTIVE", ":all entry, first failure at #{error_line_label(e)}") + {stale_acc, cand_acc} + + :timeout -> + report(file, nil, "TIMEOUT", "exceeded #{timeout}ms with no ranges") + {stale_acc, cand_acc} + end + else + Enum.reduce(entries, {stale_acc, cand_acc}, fn entry, {s, c} -> + others = entries |> Enum.reject(&(&1 == entry)) |> Enum.map(& &1.lines) + + case run_with_ranges(path, others, timeout) do :ok -> - report(file, nil, "CANDIDATE", "file passes with no ranges — try promoting") - {stale_acc, cand_acc + 1} + report(file, entry.lines, "STALE", "file passes without this range — try removing") + {s + 1, c} {:error, e} -> - line = error_line(e) || "?" - report(file, nil, "ACTIVE", ":all entry, first failure at line #{line}") - {stale_acc, cand_acc} + new_line = error_line(e) + + if new_line && new_line not in entry.lines do + report(file, entry.lines, "MOVED", "failure now at line #{new_line} — consider narrowing") + {s, c} + else + {s, c} + end :timeout -> - report(file, nil, "TIMEOUT", "exceeded #{timeout}ms with no ranges") - {stale_acc, cand_acc} + report(file, entry.lines, "TIMEOUT", "exceeded #{timeout}ms without this range") + {s, c} end - - true -> - Enum.reduce(entries, {stale_acc, cand_acc}, fn entry, {s, c} -> - others = Enum.reject(entries, &(&1 == entry)) |> Enum.map(& &1.lines) - - case run_with_ranges(path, others, timeout) do - :ok -> - report(file, entry.lines, "STALE", "file passes without this range — try removing") - {s + 1, c} - - {:error, e} -> - new_line = error_line(e) - if new_line && new_line not in entry.lines do - report(file, entry.lines, "MOVED", "failure now at line #{new_line} — consider narrowing") - {s, c} - else - {s, c} - end - - :timeout -> - report(file, entry.lines, "TIMEOUT", "exceeded #{timeout}ms without this range") - {s, c} - end - end) + end) end end) @@ -359,6 +370,15 @@ defmodule Mix.Tasks.Lua.Suite do defp error_line(e) when is_exception(e), do: Map.get(e, :line) defp error_line(_), do: nil + defp error_line_label(e) when is_exception(e) do + case Map.get(e, :line) do + nil -> "unknown line (#{e.__struct__ |> Module.split() |> List.last()})" + n -> "line #{n}" + end + end + + defp error_line_label(_), do: "unknown line" + defp report(file, nil, status, msg) do Mix.shell().info(" #{String.pad_trailing(file, 24)}#{String.pad_trailing(status, 12)}#{msg}") end @@ -369,13 +389,14 @@ defmodule Mix.Tasks.Lua.Suite do end defp range_to_string(%Range{first: a, last: a}), do: "#{a}" - defp range_to_string(%Range{first: a, last: b}), do: "#{a}..#{b}" + defp range_to_string(%Range{first: a, last: b, step: 1}), do: "#{a}..#{b}" + defp range_to_string(%Range{first: a, last: b, step: s}), do: "#{a}..#{b}//#{s}" defp load_skip_map!(path) do if !File.exists?(path) do Mix.raise("#{path} not found — nothing to report.") end - path |> Code.eval_file() |> elem(0) + Lua.SuiteRunner.load_skip_map!(path) end end diff --git a/tasks/suite_runner.ex b/tasks/suite_runner.ex index 3f9e84a..46ee17e 100644 --- a/tasks/suite_runner.ex +++ b/tasks/suite_runner.ex @@ -63,6 +63,34 @@ defmodule Lua.SuiteRunner do end end + @doc """ + Load and validate the suite skip map from an `.exs` file. + + Raises if a file mixes `lines: :all` with specific ranges, since the + test driver treats `:all` as a hard skip and silently drops any + ranges alongside it. + """ + @spec load_skip_map!(Path.t()) :: %{optional(String.t()) => [map()]} + def load_skip_map!(path) do + map = path |> Code.eval_file() |> elem(0) + validate_skip_map!(map, path) + map + end + + defp validate_skip_map!(map, path) do + Enum.each(map, fn {file, entries} -> + has_all? = Enum.any?(entries, &(&1.lines == :all)) + has_range? = Enum.any?(entries, &(&1.lines != :all)) + + if has_all? and has_range? do + raise ArgumentError, + "#{path}: #{file} mixes `lines: :all` with specific ranges. " <> + "Use one or the other — `:all` skips the whole file, so any " <> + "ranges alongside it would be silently ignored." + end + end) + end + @doc """ Replace each line covered by `ranges` with a one-line `--` comment, preserving total line count. Used by `run_file/2` and the audit diff --git a/test/lua53_suite_test.exs b/test/lua53_suite_test.exs index 40e8793..9789c9c 100644 --- a/test/lua53_suite_test.exs +++ b/test/lua53_suite_test.exs @@ -17,7 +17,7 @@ defmodule Lua.Lua53SuiteTest do # Per-file skip ranges. See test/lua53_skips.exs for shape and conventions. @external_resource @skip_file - @skip_map @skip_file |> Code.eval_file() |> elem(0) + @skip_map Lua.SuiteRunner.load_skip_map!(@skip_file) # Suite files that we have deliberately decided not to support. # diff --git a/test/mix/tasks/lua.suite_test.exs b/test/mix/tasks/lua.suite_test.exs index de77c50..dbc6945 100644 --- a/test/mix/tasks/lua.suite_test.exs +++ b/test/mix/tasks/lua.suite_test.exs @@ -165,5 +165,45 @@ defmodule Mix.Tasks.Lua.SuiteTest do assert output =~ "ACTIVE" assert output =~ "first failure at line 1" end + + test "labels failures that lack a :line field with the exception name" do + # Syntax garbage triggers Lua.CompilerException, which carries + # formatted error messages but no top-level :line field. Audit + # should fall back to a struct-name label instead of bare "?". + write_lua("bad_syntax.lua", "@@@ not lua @@@\n") + skip_file = Path.join(@tmpdir, "skips.exs") + + File.write!(skip_file, """ + %{"bad_syntax.lua" => [%{lines: :all, category: :unimplemented, reason: "t", issue: nil}]} + """) + + output = + capture_io(fn -> + Suite.run(["--audit", "--dir", @tmpdir, "--skip-file", skip_file, "--timeout", "5000"]) + end) + + assert output =~ "bad_syntax.lua" + assert output =~ "ACTIVE" + assert output =~ "unknown line (CompilerException)" + end + end + + describe "skip-file validation" do + test "raises when a file mixes lines: :all with specific ranges" do + skip_file = Path.join(@tmpdir, "skips.exs") + + File.write!(skip_file, """ + %{ + "mixed.lua" => [ + %{lines: :all, category: :unimplemented, reason: "t", issue: nil}, + %{lines: 1..3, category: :stdlib, reason: "t", issue: nil} + ] + } + """) + + assert_raise ArgumentError, ~r/mixed\.lua mixes `lines: :all`/, fn -> + Suite.run(["--status", "--skip-file", skip_file]) + end + end end end