Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

100% test coverage #246

Merged
merged 18 commits into from
Feb 14, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 6 additions & 12 deletions lib/elixir_analyzer.ex
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,14 @@ defmodule ElixirAnalyzer do

* `exercise` is which exercise is submitted to determine proper analysis

* `path` is the path (ending with a '/') to the submitted solution
* `input_path` is the path to the submitted solution

* `output_path` is the path to the output folder
angelikatyborska marked this conversation as resolved.
Show resolved Hide resolved

* `opts` is a Keyword List of options, see **options**

## Options

* `:exercise` - name of the exercise, defaults to the `exercise` parameter

* `:path` - path to the submitted solution, defaults to the `path` parameter

* `:output_path` - path to write file output, defaults to the `path` parameter
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not understand these options here, all they do is overwrite the required arguments. Those options were passed from the CLI module. I got rid of them.


* `:output_file`, - specifies the name of the output_file, defaults to
`@output_file` (`analysis.json`)

Expand All @@ -52,8 +48,6 @@ defmodule ElixirAnalyzer do

* `:puts_summary` - boolean flag if an analysis should print the summary of the
analysis to stdio, defaults to `true`

Any arbitrary keyword-value pair can be passed to `analyze_exercise/3` and these options may be used the other consuming code.
"""
@spec analyze_exercise(String.t(), String.t(), String.t(), keyword()) :: Submission.t()
def analyze_exercise(exercise, input_path, output_path, opts \\ []) do
Expand Down Expand Up @@ -147,14 +141,14 @@ defmodule ElixirAnalyzer do
}
rescue
e in File.Error ->
Logger.warning("Unable to decode 'config.json'", error_message: e.message)
Logger.warning("Unable to read config file #{e.path}", error_message: e.reason)

submission
|> Submission.halt()
|> Submission.set_halt_reason("Analysis skipped, not able to read solution config.")

e in Jason.DecodeError ->
Logger.warning("Unable to decode 'config.json'", error_message: e.message)
Logger.warning("Unable to decode 'config.json'", data: e.data)

submission
|> Submission.halt()
Expand Down Expand Up @@ -256,7 +250,7 @@ defmodule ElixirAnalyzer do

submission =
submission
|> submission.analysis_module.analyze(submission.source)
|> submission.analysis_module.analyze()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice getting rid of a duplicated argument 👍

|> Submission.set_analyzed(true)

Logger.info("Analyzing code complete")
Expand Down
51 changes: 15 additions & 36 deletions lib/elixir_analyzer/cli.ex
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,19 @@ defmodule ElixirAnalyzer.CLI do
@usage """
Usage:

$ elixir_analyzer <exercise-name> <input path> <output path> [options]
$ elixir_analyzer <exercise-name> <input path> <output path> [options]

You may also pass the following options:
--skip-analysis flag skips running the static analysis
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the purpose of skipping analysis except for debugging, so I got rid of that option too. I think it still exists somewhere in feature though, although it's never used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it still exists somewhere in feature though, although it's never used.

Do you mean the ability to skip a single check? Because I don't see anything related to skipping analysis as a whole anywhere else 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I suppose that's what I meant. There's probably no relation beyond the name.
Anyway, I don't see the point of skipping anything, just remove the code :)

--output-file <filename>
jiegillet marked this conversation as resolved.
Show resolved Hide resolved

You may also test only individual files :
(assuming analyzer tests are compiled for the named module)

$ exercism_analyzer --analyze-file <full-path-to-.ex>:<module-name>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an option in the CLI, but could not actually be passed to the analyzer, so I got rid of it

--help see this message
--output-file <filename> output file name (default: analysis.json)
--no-write-results doesn't write to JSON file
--no-puts-summary doesn't print summary to stdio
"""

@options [
{{:skip_analyze, :boolean}, false},
{{:output_file, :string}, "analysis.json"},
{{:analyze_file, :string}, nil},
{{:write_results, :boolean}, true},
{{:puts_summary, :boolean}, true},
{{:help, :boolean}, false}
]

Expand All @@ -30,45 +27,27 @@ defmodule ElixirAnalyzer.CLI do
args |> parse_args() |> process()
end

def parse_args(args) do
options = %{
:output_file => "analysis.json"
}
defp parse_args(args) do
default_ops = for({{key, _}, val} <- @options, do: {key, val}, into: %{})

cmd_opts =
OptionParser.parse(args,
strict: for({o, _} <- @options, do: o)
)
cmd_opts = OptionParser.parse(args, strict: for({o, _} <- @options, do: o))

case cmd_opts do
{[help: true], _, _} ->
:help

{[analyze_file: target], _, _} ->
[full_path, module] = String.split(target, ":", trim: true)
path = Path.dirname(full_path)
file = Path.basename(full_path)
{Enum.into([module: module, file: file], options), "undefined", path}

{opts, [exercise, input_path, output_path], _} ->
{Enum.into(opts, options), exercise, input_path, output_path}
{Enum.into(opts, default_ops), exercise, input_path, output_path}
end
rescue
_ -> :help
end

def process(:help), do: IO.puts(@usage)
defp process(:help), do: IO.puts(@usage)

def process({options, exercise, input_path, output_path}) do
opts = get_default_options(options)
ElixirAnalyzer.analyze_exercise(exercise, input_path, output_path, opts)
end
defp process({options, exercise, input_path, output_path}) do
opts = Map.to_list(options)

defp get_default_options(options) do
@options
|> Enum.reduce(options, fn {{option, _}, default}, acc ->
Map.put_new(acc, option, default)
end)
|> Map.to_list()
ElixirAnalyzer.analyze_exercise(exercise, input_path, output_path, opts)
end
end
11 changes: 6 additions & 5 deletions lib/elixir_analyzer/exercise_test.ex
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ defmodule ElixirAnalyzer.ExerciseTest do

import unquote(__MODULE__)
@before_compile unquote(__MODULE__)
@dialyzer no_match: {:do_analyze, 2}
@dialyzer no_match: {:do_analyze, 1}
end
end

Expand Down Expand Up @@ -51,19 +51,20 @@ defmodule ElixirAnalyzer.ExerciseTest do
check_source_tests = Enum.map(check_source_data, &CheckSourceCompiler.compile(&1, source))

quote do
@spec analyze(Submission.t(), Source.t()) :: Submission.t()
def analyze(%Submission{} = submission, %Source{code_string: code_string} = source) do
@spec analyze(Submission.t()) :: Submission.t()
def analyze(%Submission{source: %Source{code_string: code_string} = source} = submission) do
case Code.string_to_quoted(code_string) do
{:ok, code_ast} ->
source = %{source | code_ast: code_ast}
do_analyze(submission, source)
submission = %{submission | source: source}
do_analyze(submission)

{:error, e} ->
append_analysis_failure(submission, e)
end
end

defp do_analyze(%Submission{} = submission, %Source{code_ast: code_ast} = source) do
defp do_analyze(%Submission{source: %Source{code_ast: code_ast} = source} = submission) do
results =
Enum.concat([
unquote(feature_tests),
Expand Down
26 changes: 2 additions & 24 deletions lib/elixir_analyzer/exercise_test/assert_call/compiler.ex
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,9 @@ defmodule ElixirAnalyzer.ExerciseTest.AssertCall.Compiler do
"""
@spec matching_function_call?(
Macro.t(),
nil | AssertCall.function_signature(),
AssertCall.function_signature(),
%{[atom] => [atom] | keyword()}
) :: boolean()
def matching_function_call?(_node, nil, _), do: false

# For erlang libraries: :math._ or :math.pow
def matching_function_call?(
Expand Down Expand Up @@ -209,22 +208,6 @@ defmodule ElixirAnalyzer.ExerciseTest.AssertCall.Compiler do

def matching_function_call?(_, _, _), do: false

@doc """
compare a node to the function_signature, looking for a match for a called function
"""
@spec matching_function_def?(Macro.t(), AssertCall.function_signature()) :: boolean()
def matching_function_def?(_node, nil), do: false

def matching_function_def?(
{def_type, _, [{name, _, _args}, [do: {:__block__, _, [_ | _]}]]},
{_module_path, name}
)
when def_type in ~w[def defp]a do
true
end

def matching_function_def?(_, _), do: false

@doc """
node is a module definition
"""
Expand All @@ -237,13 +220,10 @@ defmodule ElixirAnalyzer.ExerciseTest.AssertCall.Compiler do
def extract_module_name({:defmodule, _, [{:__aliases__, _, name}, [do: _]]}),
do: name

def extract_module_name(_), do: nil

@doc """
node is a function definition
"""
def function_def?({def_type, _, [{name, _, _}, [do: _]]})
when is_atom(name) and def_type in ~w[def defp]a do
def function_def?({def_type, _, [_, [do: _]]}) when def_type in ~w[def defp]a do
true
end

Expand All @@ -260,8 +240,6 @@ defmodule ElixirAnalyzer.ExerciseTest.AssertCall.Compiler do
when is_atom(name) and def_type in ~w[def defp]a,
do: name

def extract_function_name(_), do: nil

@doc """
compare the name of the function to the function signature, if they match return true
"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ defmodule ElixirAnalyzer.ExerciseTest.CommonChecks.FunctionCapture do
functions
end

{node, %{capture_depth: depth - 1, functions: functions}}
{node, %{capture_depth: depth, functions: functions}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you discover this bug? When I bring it back, no tests fail, so it wasn't by writing tests 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that! I took me a while to remember.
I think it was from this test:

# https://github.com/exercism/elixir/blob/main/exercises/practice/sgf-parsing/.meta/example.ex
defmacrop lazy(parser) do
quote do
fn string -> unquote(parser).(string) end
end
end

At some point I was checking if actual_function? gave the right result, I was printing some values and I was expecting to see that test case pop out, but it never came, so I realized that depth was becoming negative.

I've added a failing test now. Let that be a valuable lesson of never fixing bugs without context.

end

# fn -> foo end
Expand Down
10 changes: 8 additions & 2 deletions lib/elixir_analyzer/exercise_test/feature.ex
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,16 @@ defmodule ElixirAnalyzer.ExerciseTest.Feature do
feature_data = %{feature_data | meta: Map.to_list(feature_data.meta)}
feature_data = Map.to_list(feature_data)

unless Keyword.has_key?(feature_data, :comment) do
raise "Comment must be defined for each feature test"
end
Comment on lines +56 to +58
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome 🎉 I didn't notice before that this one macro is missing the comment check


quote do
# Check if the feature is unique
case Enum.filter(@feature_tests, fn {_data, forms} ->
forms == unquote(Macro.escape(feature_forms))
case Enum.filter(@feature_tests, fn {data, forms} ->
{Keyword.get(data, :find), Keyword.get(data, :depth), forms} ==
{Keyword.get(unquote(feature_data), :find),
Keyword.get(unquote(feature_data), :depth), unquote(Macro.escape(feature_forms))}
end) do
[{data, _forms} | _] ->
raise FeatureError,
Expand Down
27 changes: 0 additions & 27 deletions lib/elixir_analyzer/quote_util.ex
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,6 @@ defmodule ElixirAnalyzer.QuoteUtil do
end)
end

@doc """
Performs a depth-first, pre-order traversal of quoted expressions.
With depth provided to a function
"""
@spec prewalk(Macro.t(), (Macro.t(), non_neg_integer -> Macro.t())) :: Macro.t()
def prewalk(ast, fun) when is_function(fun, 2) do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit hesitant to get rid of this. It seems like it could be useful, but it's not actually used anywhere. I'm assuming at this point we won't need to add another complex analysis tool.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No mercy for dead code! Even if it looks potentially useful, we will be able to recreate it when we need it. Making the code base easier to understand by removing trash is the better option every time, IMO.

elem(prewalk(ast, nil, fn x, nil, d -> {fun.(x, d), nil} end), 0)
end

@doc """
Performs a depth-first, pre-order traversal of quoted expressions
using an accumulator.
Expand All @@ -86,22 +77,4 @@ defmodule ElixirAnalyzer.QuoteUtil do
def prewalk(ast, acc, fun) when is_function(fun, 3) do
traverse_with_depth(ast, acc, fun, fn x, a, _d -> {x, a} end)
end

@doc """
Performs a depth-first, post-order traversal of quoted expressions.
"""
@spec postwalk(Macro.t(), (Macro.t(), non_neg_integer -> Macro.t())) :: Macro.t()
def postwalk(ast, fun) when is_function(fun, 2) do
elem(postwalk(ast, nil, fn x, nil, d -> {fun.(x, d), nil} end), 0)
end

@doc """
Performs a depth-first, post-order traversal of quoted expressions
using an accumulator.
"""
@spec postwalk(Macro.t(), any, (Macro.t(), any, non_neg_integer -> {Macro.t(), any})) ::
{Macro.t(), any}
def postwalk(ast, acc, fun) when is_function(fun, 3) do
traverse_with_depth(ast, acc, fn x, a, _d -> {x, a} end, fun)
end
end
Loading