From 019cc18939f81f9d9d182a7bf0ef26346e60be76 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Wed, 17 Jan 2024 10:59:07 -0700 Subject: [PATCH] tests --- .formatter.exs | 6 ++ lib/style/speedo.ex | 79 ++++++++++----------- lib/styler.ex | 2 +- test/speedo/readability_test.exs | 114 +++++++++++++++++++++++++++++++ test/support/speedo_case.ex | 50 ++++++++++++++ 5 files changed, 208 insertions(+), 43 deletions(-) create mode 100644 test/speedo/readability_test.exs create mode 100644 test/support/speedo_case.ex diff --git a/.formatter.exs b/.formatter.exs index 76f376bf..6fd3f819 100644 --- a/.formatter.exs +++ b/.formatter.exs @@ -3,6 +3,12 @@ "{mix,.formatter}.exs", "{config,lib,test}/**/*.{ex,exs}" ], + locals_without_parens: [ + assert_error: 2, + refute_errors: 1, + assert_style: 2, + assert_style: 1 + ], plugins: [Styler], line_length: 122 ] diff --git a/lib/style/speedo.ex b/lib/style/speedo.ex index b741cbbb..700fe69f 100644 --- a/lib/style/speedo.ex +++ b/lib/style/speedo.ex @@ -13,6 +13,7 @@ defmodule Styler.Speedo do @definer ~w(def defp defmacro defmacrop defguard defguardp)a def run({{def, _, [{name, m, args} | _]}, _} = zipper, ctx) when def in @definer and is_atom(name) do + {name, m, args} = if name == :when, do: hd(args), else: {name, m, args} line = m[:line] name = to_string(name) error = %{file: ctx.file, line: line, check: nil, message: nil} @@ -24,9 +25,6 @@ defmodule Styler.Speedo do predicate_error = cond do - String.starts_with?(name, "is_") && String.ends_with?(name, "?") -> - %{predicate_error | message: "`#{def} #{name}` wow choose `?` or `is_`, you monster"} - def in ~w(def defp)a and String.starts_with?(name, "is_") -> %{predicate_error | message: "`#{def} #{name}` is invalid -- use `?` not `is_` for defs"} @@ -39,7 +37,7 @@ defmodule Styler.Speedo do var_errors = Enum.map(args || [], fn arg -> - {_, var_errors} = arg |> Zipper.zip() |> Zipper.traverse([], &find_vars_with_bad_names(&1, &2, ctx.file)) + {_, var_errors} = arg |> Zipper.zip() |> Zipper.traverse([], &readability_variable_names(&1, &2, ctx.file)) var_errors end) @@ -63,6 +61,9 @@ defmodule Styler.Speedo do |> Zipper.down() |> Zipper.down() |> Zipper.right() + # coerce single-child defmodules to have the same shape as multi-child + |> Styler.Style.find_nearest_block() + |> Zipper.up() |> Zipper.children() |> Enum.flat_map(fn {:defexception, _, _} -> @@ -76,7 +77,7 @@ defmodule Styler.Speedo do } ] - {:@, _, [{:impl, m, [true]}]} -> + {:@, m, [{:impl, _, [{:__block__, _, [true]}]}]} -> [%{error | line: m[:line], check: ImplTrue, message: "`@impl true` not allowed"}] {:@, _, [{name, m, _}]} -> @@ -91,13 +92,40 @@ defmodule Styler.Speedo do {zipper, Map.update!(ctx, :errors, &[[pascal | errors] | &1])} end + def run({{:<-, m, [lhs, _] = args}, _} = zipper, ctx) do + # Credo.Check.Readability.WithCustomTaggedTuple + tag_error = + case args do + [{:__block__, _, [{{:__block__, _, [tag]}, _}]}, {:__block__, _, [{{:__block__, _, [tag]}, _}]}] -> + msg = "tagging tuples with things like `#{inspect(tag)}` is known to be evil" + %{file: ctx.file, line: m[:line], check: WithCustomTaggedTuple, message: msg} + + _ -> + [] + end + + {_, assignment_errors} = lhs |> Zipper.zip() |> Zipper.traverse([], &readability_variable_names(&1, &2, ctx.file)) + {zipper, Map.update!(ctx, :errors, &[[tag_error | assignment_errors] | &1])} + end + # Credo.Check.Readability.VariableNames # the `=` here will double report when nested in a case. need to move it to its own clause w/ "in block" - def run({{assignment_op, _, [lhs, _]}, _} = zipper, ctx) when assignment_op in ~w(= <- ->)a do - {_, errors} = lhs |> Zipper.zip() |> Zipper.traverse([], &find_vars_with_bad_names(&1, &2, ctx.file)) + def run({{assignment_op, _, [lhs, _]}, _} = zipper, ctx) when assignment_op in ~w(= ->)a do + {_, errors} = lhs |> Zipper.zip() |> Zipper.traverse([], &readability_variable_names(&1, &2, ctx.file)) {zipper, Map.update!(ctx, :errors, &[errors | &1])} end + # Credo.Check.Readability.StringSigils + def run({{:__block__, [{:delimiter, ~s|"|} | _] = m, [string]}, _} = zipper, ctx) when is_binary(string) do + if string =~ ~r/".*".*".*"/ do + msg = "use a sigil for #{inspect(string)}, it has too many quotes" + error = %{file: ctx.file, line: m[:line], check: StringSigils, message: msg} + {zipper, Map.update!(ctx, :errors, &[error | &1])} + else + {zipper, ctx} + end + end + def run(zipper, context) do case run!(Zipper.node(zipper), context.file) do nil -> {zipper, context} @@ -106,7 +134,7 @@ defmodule Styler.Speedo do end end - defp find_vars_with_bad_names({{name, m, nil}, _} = zipper, errors, file) do + defp readability_variable_names({{name, m, nil}, _} = zipper, errors, file) do if name in [:__CALLER__, :__DIR__, :__ENV__, :__MODULE__] or snake_case?(name) do {zipper, errors} else @@ -115,45 +143,12 @@ defmodule Styler.Speedo do end end - defp find_vars_with_bad_names(zipper, errors, _) do + defp readability_variable_names(zipper, errors, _) do {zipper, errors} end - # Credo.Check.Readability.StringSigils - defp run!({:__block__, [{:delimiter, ~s|"|} | _] = m, [string]}, file) when is_binary(string) do - if string =~ ~r/".*".*"/ do - msg = "use a sigil for #{inspect(string)}, it has too many quotes" - %{file: file, line: m[:line], check: StringSigils, message: msg} - end - end - - # Credo.Check.Readability.WithCustomTaggedTuple - defp run!( - {:<-, m, [{:__block__, _, [{{:__block__, _, [tag]}, _}]}, {:__block__, _, [{{:__block__, _, [tag]}, _}]}]}, - file - ) do - msg = "tagging tuples with things like `#{inspect(tag)}` is known to be evil" - %{file: file, line: m[:line], check: WithCustomTaggedTuple, message: msg} - end - defp run!(_, _), do: [] - @badName :bad - def naughtyFun(naughtyParam) do - IO.inspect(@badName) - - naughtyAssignment = :ok - - with {:ugh, naughtyVar} <- {:ugh, naughtyParam} do - naughtyVar - end - end - - def foo(naughtyParam2, %{bar: :x = naughtyParam3}) do - end - - defexception [:foo] - defp snake_case?(name), do: to_string(name) =~ ~r/^[[:lower:]\d\_\!\?]+$/u defp pascal_case?(name), do: to_string(name) =~ ~r/^[A-Z][a-zA-Z0-9\.]*$/ end diff --git a/lib/styler.ex b/lib/styler.ex index eb6aada3..ad243e02 100644 --- a/lib/styler.ex +++ b/lib/styler.ex @@ -68,7 +68,7 @@ defmodule Styler do quoted_to_string(ast, comments, formatter_opts) end - def lint(input, file) do + def lint(input, file \\ "nofile") do {ast, comments} = string_to_quoted_with_comments(input, file) context = %{comments: comments, file: file, errors: []} diff --git a/test/speedo/readability_test.exs b/test/speedo/readability_test.exs new file mode 100644 index 00000000..d593b8ee --- /dev/null +++ b/test/speedo/readability_test.exs @@ -0,0 +1,114 @@ +defmodule Styler.Speedo.ReadabilityTest do + use Styler.SpeedoCase + + alias Credo.Check.Readability.FunctionNames + alias Credo.Check.Readability.ImplTrue + alias Credo.Check.Readability.ModuleAttributeNames + alias Credo.Check.Readability.ModuleNames + alias Credo.Check.Readability.PredicateFunctionNames + alias Credo.Check.Readability.StringSigils + alias Credo.Check.Readability.VariableNames + alias Credo.Check.Readability.WithCustomTaggedTuple + + describe "Credo.Check.Readability.FunctionNames" do + test "positives" do + for def <- ~w(def defp defmacro defmacrop defguard defguardp) do + assert_error "#{def} ooF", FunctionNames + end + end + + test "negatives" do + for def <- ~w(def defp defmacro defmacrop defguard defguardp) do + refute_errors "#{def} snake(), do: :ok" + refute_errors "#{def} snake_case(), do: :ok" + end + + refute_errors "def unquote(foo)(), do: :ok" + end + end + + describe "Credo.Check.Readability.ImplTrue" do + test "this one's pretty simple ngl" do + assert_error "defmodule Foo do @impl true end", ImplTrue + refute_errors "defmodule Foo do @impl Blue end" + end + end + + describe "Credo.Check.Readability.ModuleAttributeNames" do + test "snake s'il vous plaît" do + assert_error "defmodule Foo do @weEeEe :foo end", ModuleAttributeNames + refute_errors "defmodule Foo do @snake :always_snakey end" + refute_errors "defmodule Foo do bar = @wEeEeE end" + end + end + + describe "Credo.Check.Readability.ModuleNames" do + test "pascal por favor" do + assert_error "defmodule Snake_Kinda do end", ModuleNames + refute_errors "defmodule OkayName do end" + refute_errors "defmodule Okay.Name do end" + end + end + + describe "Credo.Check.Readability.PredicateFunctionNames" do + test "defs dont get `is_` prefix" do + for def <- ~w(def defp) do + assert_error "#{def} is_foo?", PredicateFunctionNames + assert_error "#{def} is_foo?(bar)", PredicateFunctionNames + assert_error "#{def} is_foo?(bar) when bar != :baz, do: :bop", PredicateFunctionNames + refute_errors "#{def} foo?" + refute_errors "#{def} foo?(bar)" + refute_errors "#{def} foo?(bar) when bar != :baz, do: :bop" + end + end + + test "macros and guards dont get `?` suffix" do + for def <- ~w(defmacro defmacrop defguard defguardp) do + assert_error "#{def} is_foo?", PredicateFunctionNames + assert_error "#{def} is_foo?(bar)", PredicateFunctionNames + assert_error "#{def} is_foo?(bar) when bar != :baz, do: :bop", PredicateFunctionNames + refute_errors "#{def} is_foo" + refute_errors "#{def} is_foo(bar)" + refute_errors "#{def} is_foo(bar) when bar != :baz, do: :bop" + end + end + end + + describe "Credo.Check.Readability.StringSigils" do + test "3 escaped quotes tops" do + assert_error ~s|x = "\\"1\\"2\\"3\\"4"|, StringSigils + refute_errors ~s|x = ~s{"1"2"3"4}| + refute_errors ~s|x = "\\"1\\"2\\"3"| + end + end + + describe "Credo.Check.Readability.VariableNames" do + test "reports violations on variable creation only " do + errors = + lint(""" + def foo(badName) do + with {:ok, anotherBadName} <- badName do + [stopCamelCase | camelsTail] = anotherBadName + lovely_var_name = functionNotYourFault() + end + end + """) + + assert Enum.count(errors) == 4 + + errors = errors |> Enum.group_by(& &1.line) |> Map.new() + + assert [%{check: VariableNames, message: "badName"}] = errors[1] + assert [%{check: VariableNames, message: "anotherBadName"}] = errors[2] + assert [one, two] = errors[3] + assert "stopCamelCase" in [one.message, two.message] + assert "camelsTail" in [one.message, two.message] + end + end + + describe "Credo.Check.Readability.WithCustomTaggedTuple" do + test "shames tagged tuples" do + assert_error "with {:ooph, result} <- {:ooph, call()}, do: :ok", WithCustomTaggedTuple + end + end +end diff --git a/test/support/speedo_case.ex b/test/support/speedo_case.ex new file mode 100644 index 00000000..115e4bd1 --- /dev/null +++ b/test/support/speedo_case.ex @@ -0,0 +1,50 @@ +# Copyright 2023 Adobe. All rights reserved. +# This file is licensed to you under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. You may obtain a copy +# of the License at http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +# OF ANY KIND, either express or implied. See the License for the specific language +# governing permissions and limitations under the License. + +defmodule Styler.SpeedoCase do + @moduledoc """ + Helpers around testing Style rules. + """ + use ExUnit.CaseTemplate + + using do + quote do + import unquote(__MODULE__), only: [assert_error: 2, refute_errors: 1, lint: 1] + end + end + + defmacro assert_error(code, check) do + quote location: :keep, bind_quoted: [code: code, check: check] do + errors = lint(code) + + if Enum.empty?(errors) and ExUnit.configuration()[:trace] do + {ast, comments} = Styler.string_to_quoted_with_comments(code) + dbg(ast) + dbg(comments) + end + + assert [%{check: ^check, message: m, line: l, file: f}] = errors + assert m, "message was nil for #{check} in #{inspect(errors)}" + assert l, "line was nil for #{check} in #{inspect(errors)}" + assert f, "file was nil for #{check} in #{inspect(errors)}" + end + end + + def refute_errors(code) do + assert [] = lint(code) + end + + def lint(code) do + code + |> Styler.lint() + |> List.flatten() + |> Enum.reject(&is_nil/1) + end +end