Fix --fail-above with --exclude (#11306)

And improve test coverage.

Author: marcandre

lib/mix/lib/mix/tasks/xref.ex

@@ -586,10 +586,16 @@ defmodule Mix.Tasks.Xref do
 
   ## Graph
 
-  defp excluded(opts) do
-    opts
-    |> Keyword.get_values(:exclude)
-    |> Enum.flat_map(&[{&1, nil}, {&1, :compile}, {&1, :export}])
+  defp exclude(file_references, []), do: file_references
+
+  defp exclude(file_references, excluded) do
+    excluded_set = MapSet.new(excluded)
+
+    file_references
+    |> Map.drop(excluded)
+    |> Map.new(fn {key, list} ->
+      {key, Enum.reject(list, fn {ref, _kind} -> MapSet.member?(excluded_set, ref) end)}
+    end)
   end
 
   defp label_filter(nil), do: :all
@@ -671,7 +677,7 @@ defmodule Mix.Tasks.Xref do
   end
 
   defp write_graph(file_references, filter, opts) do
-    excluded = excluded(opts)
+    file_references = exclude(file_references, Keyword.get_values(opts, :exclude))
     sources = get_files(:source, opts, file_references)
     sinks = get_files(:sink, opts, file_references)
 
@@ -683,7 +689,7 @@ defmodule Mix.Tasks.Xref do
       end
 
     # Filter according to non direct label
-    file_references = filter(file_references, excluded, filter)
+    file_references = filter(file_references, filter)
 
     # If a label is given, remove empty root nodes
     file_references =
@@ -700,13 +706,12 @@ defmodule Mix.Tasks.Xref do
         file_references
         |> Map.drop(sinks || [])
         |> Enum.map(&{elem(&1, 0), nil})
-        |> Kernel.--(excluded)
       end
 
     callback = fn {file, type} ->
       children = if opts[:only_nodes], do: [], else: Map.get(file_references, file, [])
       type = type && "(#{type})"
-      {{file, type}, Enum.sort(children -- excluded)}
+      {{file, type}, Enum.sort(children)}
     end
 
     {found, count} =
@@ -755,18 +760,18 @@ defmodule Mix.Tasks.Xref do
     Enum.reduce(file_references, 0, fn {_, refs}, total -> total + length(refs) end)
   end
 
-  defp filter_fn(file_references, excluded, :compile_connected),
+  defp filter_fn(file_references, :compile_connected),
     do: fn {key, type} ->
-      type == :compile and match?([_ | _], (file_references[key] || []) -- excluded)
+      type == :compile and match?([_ | _], file_references[key] || [])
     end
 
-  defp filter_fn(_file_references, _excluded, filter),
+  defp filter_fn(_file_references, filter),
     do: fn {_key, type} -> type == filter end
 
-  defp filter(file_references, _excluded, :all), do: file_references
+  defp filter(file_references, :all), do: file_references
 
-  defp filter(file_references, excluded, filter) do
-    filter_fn = filter_fn(file_references, excluded, filter)
+  defp filter(file_references, filter) do
+    filter_fn = filter_fn(file_references, filter)
 
     for {key, children} <- file_references,
         into: %{},

lib/mix/test/mix/tasks/xref_test.exs

@@ -449,6 +449,33 @@ defmodule Mix.Tasks.XrefTest do
       """)
     end
 
+    @abc_linear_files %{
+      "lib/a.ex" => "defmodule A, do: def a, do: B.b()",
+      "lib/b.ex" => "defmodule B, do: def b, do: C.c()",
+      "lib/c.ex" => "defmodule C, do: def c, do: true"
+    }
+
+    test "exclude one from linear case" do
+      assert_graph(
+        ~w[--exclude lib/b.ex],
+        """
+        lib/a.ex
+        lib/c.ex
+        """,
+        files: @abc_linear_files
+      )
+    end
+
+    test "exclude one with source from linear case" do
+      assert_graph(
+        ~w[--exclude lib/b.ex --source lib/a.ex],
+        """
+        lib/a.ex
+        """,
+        files: @abc_linear_files
+      )
+    end
+
     test "only nodes" do
       assert_graph(~w[--only-nodes], """
       lib/a.ex
@@ -507,6 +534,19 @@ defmodule Mix.Tasks.XrefTest do
       end
     end
 
+    test "exclude many with fail-above" do
+      message = "Too many references (found: 1, permitted: 0)"
+
+      assert_raise Mix.Error, message, fn ->
+        assert_graph(~w[--exclude lib/c.ex --exclude lib/b.ex --fail-above 0], """
+        lib/a.ex
+        lib/d.ex
+        `-- lib/e.ex
+        lib/e.ex
+        """)
+      end
+    end
+
     test "filter by compile direct label" do
       assert_graph(~w[--label compile --only-direct], """
       lib/a.ex
@@ -795,49 +835,54 @@ defmodule Mix.Tasks.XrefTest do
       end)
     end
 
-    defp assert_graph(opts \\ [], expected) do
-      in_fixture("no_mixfile", fn ->
-        File.write!("lib/a.ex", """
-        defmodule A do
-          def a, do: :ok
-          B.b2()
-        end
-        """)
-
-        File.write!("lib/b.ex", """
-        defmodule B do
-          def b1, do: A.a() == C.c()
-          def b2, do: :ok
-          :e.e()
-        end
-        """)
-
-        File.write!("lib/c.ex", """
-        defmodule C do
-          def c, do: :ok
-          :d.d()
-        end
-        """)
-
-        File.write!("lib/d.ex", """
-        defmodule :d do
-          def d, do: :ok
-          def e, do: :e.e()
-        end
-        """)
+    @default_files %{
+      "lib/a.ex" => """
+      defmodule A do
+        def a, do: :ok
+        B.b2()
+      end
+      """,
+      "lib/b.ex" => """
+      defmodule B do
+        def b1, do: A.a() == C.c()
+        def b2, do: :ok
+        :e.e()
+      end
+      """,
+      "lib/c.ex" => """
+      defmodule C do
+        def c, do: :ok
+        :d.d()
+      end
+      """,
+      "lib/d.ex" => """
+      defmodule :d do
+        def d, do: :ok
+        def e, do: :e.e()
+      end
+      """,
+      "lib/e.ex" => """
+      defmodule :e do
+        def e, do: :ok
+      end
+      """
+    }
 
-        File.write!("lib/e.ex", """
-        defmodule :e do
-          def e, do: :ok
-        end
-        """)
+    defp assert_graph(opts \\ [], expected, params \\ []) do
+      in_fixture("no_mixfile", fn ->
+        nb_files =
+          Enum.count(params[:files] || @default_files, fn {path, content} ->
+            File.write!(path, content)
+          end)
 
         assert Mix.Task.run("xref", opts ++ ["graph"]) == :ok
+        first_line = "Compiling #{nb_files} files (.ex)"
 
-        assert "Compiling 5 files (.ex)\nGenerated sample app\n" <> result =
-                 receive_until_no_messages([])
+        assert [
+                 ^first_line | ["Generated sample app" | result]
+               ] = receive_until_no_messages([]) |> String.split("\n")
 
-        assert normalize_graph_output(result) == expected
+        assert normalize_graph_output(result |> Enum.join("\n")) == expected
       end)
     end