diff --git a/README.md b/README.md index fee2b24c..614a8ad3 100644 --- a/README.md +++ b/README.md @@ -57,7 +57,7 @@ As a demonstration, once iex loads with `iex -S mix` you can type `ElixirAnalyze ### `ElixirAnalyzer` - The is the main application module. A function call to `start_analyze/3` begins the analysis (either through IEX or the CLI escript [if generated]). -- A configuation in `config/config.exs` holds data for each exercise supported +- A configuration in `config/config.exs` holds data for each exercise supported ```elixir config :elixir_analyzer, @@ -110,7 +110,7 @@ Contains macro to generate a function returning the comment path on the `exercis ### `ElixirAnalyzer.CLI` -This module is a module for the CLI escript to parse the command line arguements and start the processing +This module is a module for the CLI escript to parse the command line arguments and start the processing ### `ElixirAnalyzer.ExerciseTest.________` diff --git a/lib/elixir_analyzer.ex b/lib/elixir_analyzer.ex index e3ac545c..d548bf40 100644 --- a/lib/elixir_analyzer.ex +++ b/lib/elixir_analyzer.ex @@ -134,15 +134,6 @@ defmodule ElixirAnalyzer do analysis_module: analysis_module } rescue - e in ArgumentError -> - Logger.warning("TestSuite halted, ArgumentError", error_message: e.message) - - submission - |> Submission.halt() - |> Submission.set_halt_reason( - "Analysis skipped, no analysis suite exists for this exercise" - ) - e in File.Error -> Logger.warning("Unable to decode 'config.json'", error_message: e.message) @@ -156,6 +147,13 @@ defmodule ElixirAnalyzer do submission |> Submission.halt() |> Submission.set_halt_reason("Analysis skipped, not able to decode solution config.") + + e -> + Logger.warning("TestSuite halted, #{e.__struct__}", error_message: e.message) + + submission + |> Submission.halt() + |> Submission.set_halt_reason("Analysis skipped, unexpected error #{e.__struct__}") end end @@ -168,7 +166,7 @@ defmodule ElixirAnalyzer do code_path = Path.dirname(full_code_path) code_file = Path.basename(full_code_path) - {code_path, code_file, exercise_config[:analyzer_module]} + {code_path, code_file, exercise_config[:analyzer_module] || ElixirAnalyzer.TestSuite.Default} end # Else, use passed in params to analyze diff --git a/lib/elixir_analyzer/comment.ex b/lib/elixir_analyzer/comment.ex index ded1ca5e..c674f68b 100644 --- a/lib/elixir_analyzer/comment.ex +++ b/lib/elixir_analyzer/comment.ex @@ -11,6 +11,7 @@ defmodule ElixirAnalyzer.Comment do comment: String.t(), type: :essential | :actionable | :informative | :celebratory, status: :skip | :test, - suppress_if: false | {String.t(), :pass | :fail} + suppress_if: false | {String.t(), :pass | :fail}, + params: map() } end diff --git a/lib/elixir_analyzer/constants.ex b/lib/elixir_analyzer/constants.ex index f6ea94e3..c3ee4873 100644 --- a/lib/elixir_analyzer/constants.ex +++ b/lib/elixir_analyzer/constants.ex @@ -18,6 +18,8 @@ defmodule ElixirAnalyzer.Constants do solution_use_moduledoc: "elixir.solution.use_module_doc", solution_use_specification: "elixir.solution.use_specification", solution_raise_fn_clause_error: "elixir.solution.raise_fn_clause_error", + solution_module_attribute_name_snake_case: "elixir.solution.module_attribute_name_snake_case", + solution_function_name_snake_case: "elixir.solution.function_name_snake_case", # Concept exercises diff --git a/lib/elixir_analyzer/exercise_test.ex b/lib/elixir_analyzer/exercise_test.ex index c0a7bca7..db4d0b66 100644 --- a/lib/elixir_analyzer/exercise_test.ex +++ b/lib/elixir_analyzer/exercise_test.ex @@ -3,6 +3,7 @@ defmodule ElixirAnalyzer.ExerciseTest do alias ElixirAnalyzer.ExerciseTest.Feature.Compiler, as: FeatureCompiler alias ElixirAnalyzer.ExerciseTest.AssertCall.Compiler, as: AssertCallCompiler + alias ElixirAnalyzer.ExerciseTest.CommonChecks alias ElixirAnalyzer.Submission alias ElixirAnalyzer.Constants @@ -45,10 +46,12 @@ defmodule ElixirAnalyzer.ExerciseTest do {:ok, code_ast} -> feature_results = unquote(feature_tests) |> filter_suppressed_results() assert_call_results = unquote(assert_call_tests) |> filter_suppressed_results() + common_checks_results = CommonChecks.run(code_ast, code_as_string) submission |> append_test_comments(feature_results) |> append_test_comments(assert_call_results) + |> append_test_comments(common_checks_results) {:error, e} -> append_analysis_failure(submission, e) diff --git a/lib/elixir_analyzer/exercise_test/common_checks.ex b/lib/elixir_analyzer/exercise_test/common_checks.ex new file mode 100644 index 00000000..01017b8a --- /dev/null +++ b/lib/elixir_analyzer/exercise_test/common_checks.ex @@ -0,0 +1,18 @@ +defmodule ElixirAnalyzer.ExerciseTest.CommonChecks do + @moduledoc """ + This module aggregates all common checks that should be run on every single solution. + """ + + alias ElixirAnalyzer.ExerciseTest.CommonChecks.FunctionNames + alias ElixirAnalyzer.ExerciseTest.CommonChecks.ModuleAttributeNames + alias ElixirAnalyzer.Comment + + @spec run(Macro.t(), String.t()) :: [{:pass | :fail | :skip, %Comment{}}] + def run(code_ast, code_as_string) when is_binary(code_as_string) do + [ + FunctionNames.run(code_ast), + ModuleAttributeNames.run(code_ast) + ] + |> List.flatten() + end +end diff --git a/lib/elixir_analyzer/exercise_test/common_checks/function_names.ex b/lib/elixir_analyzer/exercise_test/common_checks/function_names.ex new file mode 100644 index 00000000..15a37616 --- /dev/null +++ b/lib/elixir_analyzer/exercise_test/common_checks/function_names.ex @@ -0,0 +1,63 @@ +defmodule ElixirAnalyzer.ExerciseTest.CommonChecks.FunctionNames do + @moduledoc """ + Reports the first function/macro/guard with a name that's not snake_case. + + Doesn't report more if there are more. + A single comment should be enough for the student to know what to fix. + + Common check to be run on every single solution. + """ + + alias ElixirAnalyzer.Constants + alias ElixirAnalyzer.Comment + + @def_ops [:def, :defp, :defmacro, :defmacrop, :defguard, :defguardp] + + @spec run(Macro.t()) :: [{:pass | :fail | :skip, %Comment{}}] + def run(ast) do + {_, names} = Macro.prewalk(ast, [], &traverse/2) + wrong_name = List.last(names) + + if wrong_name do + wrong_name = to_string(wrong_name) + correct_name = to_snake_case(wrong_name) + + [ + {:fail, + %Comment{ + type: :actionable, + comment: Constants.solution_function_name_snake_case(), + params: %{ + expected: correct_name, + actual: wrong_name + } + }} + ] + else + [] + end + end + + defp traverse({op, _meta, [{name, _meta2, _arguments} | _]} = ast, names) when op in @def_ops do + if snake_case?(name) do + {ast, names} + else + {ast, [name | names]} + end + end + + defp traverse(ast, names) do + {ast, names} + end + + defp snake_case?(name) do + # the code had to compile and pass all the tests to get to the analyzer + # so we can assume the name is otherwise valid + to_snake_case(name) == to_string(name) + end + + defp to_snake_case(name) do + # Macro.underscore is good enough because a module attribute name must be a valid Elixir identifier anyway + Macro.underscore(to_string(name)) + end +end diff --git a/lib/elixir_analyzer/exercise_test/common_checks/module_attribute_names.ex b/lib/elixir_analyzer/exercise_test/common_checks/module_attribute_names.ex new file mode 100644 index 00000000..3cb7904a --- /dev/null +++ b/lib/elixir_analyzer/exercise_test/common_checks/module_attribute_names.ex @@ -0,0 +1,61 @@ +defmodule ElixirAnalyzer.ExerciseTest.CommonChecks.ModuleAttributeNames do + @moduledoc """ + Reports the first module attribute with a name that's not snake_case. + + Doesn't report more if there are more. + A single comment should be enough for the student to know what to fix. + + Common check to be run on every single solution. + """ + + alias ElixirAnalyzer.Constants + alias ElixirAnalyzer.Comment + + @spec run(Macro.t()) :: [{:pass | :fail | :skip, %Comment{}}] + def run(ast) do + {_, names} = Macro.prewalk(ast, [], &traverse/2) + wrong_name = List.last(names) + + if wrong_name do + wrong_name = to_string(wrong_name) + correct_name = to_snake_case(wrong_name) + + [ + {:fail, + %Comment{ + type: :actionable, + comment: Constants.solution_module_attribute_name_snake_case(), + params: %{ + expected: correct_name, + actual: wrong_name + } + }} + ] + else + [] + end + end + + defp traverse({:@, _meta, [{name, _meta2, _arguments}]} = ast, names) do + if snake_case?(name) do + {ast, names} + else + {ast, [name | names]} + end + end + + defp traverse(ast, names) do + {ast, names} + end + + defp snake_case?(name) do + # the code had to compile and pass all the tests to get to the analyzer + # so we can assume the name is otherwise valid + to_snake_case(name) == to_string(name) + end + + defp to_snake_case(name) do + # Macro.underscore is good enough because a module attribute name must be a valid Elixir identifier anyway + Macro.underscore(to_string(name)) + end +end diff --git a/lib/elixir_analyzer/test_suite/default.ex b/lib/elixir_analyzer/test_suite/default.ex new file mode 100644 index 00000000..eb5a7186 --- /dev/null +++ b/lib/elixir_analyzer/test_suite/default.ex @@ -0,0 +1,10 @@ +defmodule ElixirAnalyzer.TestSuite.Default do + @moduledoc """ + This is the default exercise analyzer extension module. + + It will be run for any exercise submission that doesn't have its own extension module. + It's intentionally empty, which means it will only run the common checks. + """ + + use ElixirAnalyzer.ExerciseTest +end diff --git a/test/elixir_analyzer/exercise_test/common_checks/function_names_test.exs b/test/elixir_analyzer/exercise_test/common_checks/function_names_test.exs new file mode 100644 index 00000000..6283ef0c --- /dev/null +++ b/test/elixir_analyzer/exercise_test/common_checks/function_names_test.exs @@ -0,0 +1,177 @@ +# credo:disable-for-this-file Credo.Check.Readability.FunctionNames + +defmodule ElixirAnalyzer.ExerciseTest.CommonChecks.FunctionNamesTest do + use ExUnit.Case + alias ElixirAnalyzer.Comment + alias ElixirAnalyzer.Constants + alias ElixirAnalyzer.ExerciseTest.CommonChecks.FunctionNames + + test "returns empty list if there are functions, macros, or guards" do + code = + quote do + defmodule User do + defstruct(:name, :email) + end + end + + assert FunctionNames.run(code) == [] + end + + test "doesn't crash when a variable is named def" do + code = + quote do + defmodule User do + def name(user) do + def = user.first_name + defmacro = user.last_name + def <> " " <> defmacro + end + end + end + + assert FunctionNames.run(code) == [] + end + + test "returns empty list if all names are correct" do + code = + quote do + defmodule User do + def first_name(user), do: user.first_name + defp registered_in_last_quarter?(user), do: true + defmacro validate_user!, do: true + defmacrop do_validate_user, do: true + defguard(is_user, do: true) + defguardp(is_registered_user, do: true) + end + end + + assert FunctionNames.run(code) == [] + end + + test "returns an actionable comment with params" do + code = + quote do + defmodule User do + def firstName(user), do: user.first_name + end + end + + assert FunctionNames.run(code) == [ + {:fail, + %Comment{ + type: :actionable, + comment: Constants.solution_function_name_snake_case(), + params: %{ + expected: "first_name", + actual: "firstName" + } + }} + ] + end + + test "works for def, defp, defmacro, defmacrop, defguard, and defguardp" do + get_result = fn expected, actual -> + [ + {:fail, + %Comment{ + type: :actionable, + comment: Constants.solution_function_name_snake_case(), + params: %{ + expected: expected, + actual: actual + } + }} + ] + end + + code = + quote do + defmodule User do + defp registeredInLastQuarter?(user), do: true + end + end + + assert FunctionNames.run(code) == + get_result.("registered_in_last_quarter?", "registeredInLastQuarter?") + + code = + quote do + defmodule User do + defmacro validateUser!, do: true + end + end + + assert FunctionNames.run(code) == get_result.("validate_user!", "validateUser!") + + code = + quote do + defmodule User do + defmacrop doValidateUser, do: true + end + end + + assert FunctionNames.run(code) == get_result.("do_validate_user", "doValidateUser") + + code = + quote do + defmodule User do + defguard(isUser, do: true) + end + end + + assert FunctionNames.run(code) == get_result.("is_user", "isUser") + + code = + quote do + defmodule User do + defguardp(isRegisteredUser, do: true) + end + end + + assert FunctionNames.run(code) == get_result.("is_registered_user", "isRegisteredUser") + end + + test "only reports the first wrong name" do + code = + quote do + defmodule User do + def first_name(user), do: getFirstName(user) + defguard(isRegisteredUser(user), do: true) + defp getFirstName(user), do: user.first_name + end + end + + assert FunctionNames.run(code) == [ + {:fail, + %Comment{ + type: :actionable, + comment: Constants.solution_function_name_snake_case(), + params: %{ + expected: "is_registered_user", + actual: "isRegisteredUser" + } + }} + ] + end + + test "can handle function heads" do + code = + quote do + defmodule User do + def getName(user, fallback \\ "Doe") + end + end + + assert FunctionNames.run(code) == [ + {:fail, + %Comment{ + type: :actionable, + comment: Constants.solution_function_name_snake_case(), + params: %{ + expected: "get_name", + actual: "getName" + } + }} + ] + end +end diff --git a/test/elixir_analyzer/exercise_test/common_checks/module_attribute_names_test.exs b/test/elixir_analyzer/exercise_test/common_checks/module_attribute_names_test.exs new file mode 100644 index 00000000..e8df0635 --- /dev/null +++ b/test/elixir_analyzer/exercise_test/common_checks/module_attribute_names_test.exs @@ -0,0 +1,95 @@ +# credo:disable-for-this-file Credo.Check.Readability.ModuleAttributeNames + +defmodule ElixirAnalyzer.ExerciseTest.CommonChecks.ModuleAttributeNamesTest do + use ExUnit.Case + alias ElixirAnalyzer.Comment + alias ElixirAnalyzer.Constants + alias ElixirAnalyzer.ExerciseTest.CommonChecks.ModuleAttributeNames + + test "returns empty list if there are no module attributes" do + code = + quote do + defmodule Factorial do + def calculate(1), do: 1 + + def calculate(n) do + n * calculate(n - 1) + end + end + end + + assert ModuleAttributeNames.run(code) == [] + end + + test "returns empty list if there are no module attributes with camelCase names" do + code = + quote do + defmodule Factorial do + @initial_value 1 + @spec calculate(integer) :: integer + def calculate(1), do: @initial_value + + def calculate(n) do + n * calculate(n - 1) + end + end + end + + assert ModuleAttributeNames.run(code) == [] + end + + test "returns an actionable comment with params" do + code = + quote do + defmodule Factorial do + @initialValue 1 + @spec calculate(integer) :: integer + def calculate(1), do: @initialValue + + def calculate(n) do + n * calculate(n - 1) + end + end + end + + assert ModuleAttributeNames.run(code) == [ + {:fail, + %Comment{ + type: :actionable, + comment: Constants.solution_module_attribute_name_snake_case(), + params: %{ + expected: "initial_value", + actual: "initialValue" + } + }} + ] + end + + test "only reports the first module attribute" do + code = + quote do + defmodule Factorial do + @somethingElseCamelCase + @initialValue 1 + @spec calculate(integer) :: integer + def calculate(1), do: @initialValue + + def calculate(n) do + n * calculate(n - 1) + end + end + end + + assert ModuleAttributeNames.run(code) == [ + {:fail, + %Comment{ + type: :actionable, + comment: Constants.solution_module_attribute_name_snake_case(), + params: %{ + expected: "something_else_camel_case", + actual: "somethingElseCamelCase" + } + }} + ] + end +end diff --git a/test/elixir_analyzer/exercise_test/common_checks_test.exs b/test/elixir_analyzer/exercise_test/common_checks_test.exs new file mode 100644 index 00000000..ce449d2b --- /dev/null +++ b/test/elixir_analyzer/exercise_test/common_checks_test.exs @@ -0,0 +1,130 @@ +# credo:disable-for-this-file Credo.Check.Readability.ModuleAttributeNames +# credo:disable-for-this-file Credo.Check.Readability.FunctionNames + +defmodule ElixirAnalyzer.ExerciseTestTest.Empty do + use ElixirAnalyzer.ExerciseTest +end + +defmodule ElixirAnalyzer.ExerciseTest.CommonChecksTest do + alias ElixirConstants.Constants + + use ElixirAnalyzer.ExerciseTestCase, + exercise_test_module: ElixirAnalyzer.ExerciseTestTest.Empty + + describe "function, guard, macro names" do + test_exercise_analysis "doesn't report correct names", + comments_exclude: [Constants.solution_function_name_snake_case()] do + [ + defmodule SomeModule do + def foo_bar, do: 1 + + def foo_bar_baz do + foo_bar + 8 + end + end, + defmodule SomeModule do + defp foo_bar, do: 1 + + defp foo_bar_baz do + foo_bar + 8 + end + end, + defmodule SomeModule do + defmacro foo_bar, do: 1 + + defmacro foo_bar_baz do + foo_bar + 8 + end + end, + defmodule SomeModule do + defmacrop foo_bar, do: 1 + + defmacrop foo_bar_baz do + foo_bar + 8 + end + end, + defmodule SomeModule do + defguard(foo_bar, do: 1) + + defguard foo_bar_baz do + foo_bar + 8 + end + end, + defmodule SomeModule do + defguardp(foo_bar, do: 1) + + defguardp foo_bar_baz do + foo_bar + 8 + end + end + ] + end + + test_exercise_analysis "reports incorrect names", + comments_include: [Constants.solution_function_name_snake_case()] do + [ + defmodule SomeModule do + def fooBarBaz do + foo_bar + 8 + end + end, + defmodule SomeModule do + defp fooBarBaz do + foo_bar + 8 + end + end, + defmodule SomeModule do + defmacro fooBarBaz do + foo_bar + 8 + end + end, + defmodule SomeModule do + defmacrop fooBarBaz do + foo_bar + 8 + end + end, + defmodule SomeModule do + defguard fooBarBaz do + foo_bar + 8 + end + end, + defmodule SomeModule do + defguardp fooBarBaz do + foo_bar + 8 + end + end + ] + end + end + + describe "module attribute names" do + test_exercise_analysis "doesn't report correct module attribute names", + comments_exclude: [Constants.solution_module_attribute_name_snake_case()] do + [ + defmodule SomeModule do + @foo_bar + end, + defmodule SomeModule do + @foo_bar1 + @foo_bar2 + end, + defmodule SomeModule do + @foo_bar1 5 + @foo_bar2 2 + end + ] + end + + test_exercise_analysis "reports a module attribute that doesn't use snake_case", + comments_include: [Constants.solution_module_attribute_name_snake_case()] do + [ + defmodule SomeModule do + @fooBar + end, + defmodule SomeModule do + @someValue 3 + end + ] + end + end +end diff --git a/test/elixir_analyzer/exercise_test_test.exs b/test/elixir_analyzer/exercise_test_test.exs index ab43f572..c5679df1 100644 --- a/test/elixir_analyzer/exercise_test_test.exs +++ b/test/elixir_analyzer/exercise_test_test.exs @@ -47,6 +47,8 @@ defmodule ElixirAnalyzer.ExerciseTestTest.SameComment do end defmodule ElixirAnalyzer.ExerciseTestTest do + alias ElixirConstants.Constants + use ElixirAnalyzer.ExerciseTestCase, exercise_test_module: ElixirAnalyzer.ExerciseTestTest.SameComment diff --git a/test/elixir_analyzer_test.exs b/test/elixir_analyzer_test.exs index 6e41ca9a..baa09e4c 100644 --- a/test/elixir_analyzer_test.exs +++ b/test/elixir_analyzer_test.exs @@ -29,7 +29,7 @@ defmodule ElixirAnalyzerTest do analyzed_exercise = ElixirAnalyzer.analyze_exercise(exercise, path, path, @options) expected_output = """ - {\"comments\":[{\"comment\":\"elixir.solution.use_module_doc\",\"type\":\"informative\"},{\"comment\":\"elixir.solution.raise_fn_clause_error\",\"type\":\"actionable\"},{\"comment\":\"elixir.two-fer.use_of_function_header\",\"type\":\"actionable\"},{\"comment\":\"elixir.solution.use_specification\",\"type\":\"actionable\"}],\"summary\":\"Check the comments for some code suggestions. 📣\"} + {\"comments\":[{\"comment\":\"elixir.solution.use_module_doc\",\"type\":\"informative\"},{\"comment\":\"elixir.solution.raise_fn_clause_error\",\"type\":\"actionable\"},{\"comment\":\"elixir.two-fer.use_of_function_header\",\"type\":\"actionable\"},{\"comment\":\"elixir.solution.use_specification\",\"type\":\"actionable\"},{\"comment\":\"elixir.solution.module_attribute_name_snake_case\",\"params\":{\"actual\":\"someUnusedModuleAttribute\",\"expected\":\"some_unused_module_attribute\"},\"type\":\"actionable\"}],\"summary\":\"Check the comments for some code suggestions. 📣\"} """ assert Submission.to_json(analyzed_exercise) == String.trim(expected_output) @@ -63,15 +63,15 @@ defmodule ElixirAnalyzerTest do assert Submission.to_json(analyzed_exercise) == String.trim(expected_output) end - test "solution for an exercise with no analyzer module" do + test "solution for an exercise with no analyzer module uses the default module" do exercise = "not-a-real-exercise" - path = "./test_data/two_fer/error_solution/" + path = "./test_data/two_fer/imperfect_solution/" capture_log(fn -> analyzed_exercise = ElixirAnalyzer.analyze_exercise(exercise, path, path, @options) expected_output = """ - {\"comments\":[],\"summary\":\"Analysis was halted. Analysis skipped, no analysis suite exists for this exercise\"} + {\"comments\":[{\"comment\":\"elixir.solution.module_attribute_name_snake_case\",\"params\":{\"actual\":\"someUnusedModuleAttribute\",\"expected\":\"some_unused_module_attribute\"},\"type\":\"actionable\"}],\"summary\":\"Check the comments for some code suggestions. 📣\"} """ assert Submission.to_json(analyzed_exercise) == String.trim(expected_output) @@ -97,6 +97,9 @@ defmodule ElixirAnalyzerTest do unused_available_test_suites = all_available_test_suites -- test_suites_referenced_in_config + unused_available_test_suites = + unused_available_test_suites -- [ElixirAnalyzer.TestSuite.Default] + unavailable_test_suites_referenced_in_config = test_suites_referenced_in_config -- all_available_test_suites diff --git a/test_data/two_fer/imperfect_solution/lib/two_fer.ex b/test_data/two_fer/imperfect_solution/lib/two_fer.ex index bd4dfe6a..d96a8770 100644 --- a/test_data/two_fer/imperfect_solution/lib/two_fer.ex +++ b/test_data/two_fer/imperfect_solution/lib/two_fer.ex @@ -1,4 +1,6 @@ defmodule TwoFer do + @someUnusedModuleAttribute 1 + @doc """ Two-fer or 2-fer is short for two for one. One for you and one for me. """