Skip to content

Allow reading multiple file solutions and exemploids #298

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

Merged
merged 13 commits into from
Jun 12, 2022
Merged
2 changes: 1 addition & 1 deletion elixir
70 changes: 48 additions & 22 deletions lib/elixir_analyzer.ex
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ defmodule ElixirAnalyzer do

defaults = [
{:exercise, exercise},
{:path, input_path},
{:path, String.trim_leading(input_path, "./")},
{:output_path, output_path},
{:output_file, @output_file},
{:exercise_config, default_exercise_config()},
Expand Down Expand Up @@ -107,14 +107,14 @@ defmodule ElixirAnalyzer do
Logger.debug("Getting the exercise config")
exercise_config = params.exercise_config[params.exercise]

{code_path, exercise_type, exemploid_path, analysis_module} =
{submitted_files, exercise_type, exemploid_files, analysis_module} =
do_init(params, exercise_config)

Logger.debug("Initialization successful",
path: params.path,
code_path: code_path,
submitted_files: submitted_files,
exercise_type: exercise_type,
exemploid_path: exemploid_path,
exemploid_files: exemploid_files,
analysis_module: analysis_module
)

Expand All @@ -129,9 +129,9 @@ defmodule ElixirAnalyzer do

source = %{
source
| code_path: code_path,
| submitted_files: submitted_files,
exercise_type: exercise_type,
exemploid_path: exemploid_path
exemploid_files: exemploid_files
}

%{
Expand Down Expand Up @@ -165,16 +165,28 @@ defmodule ElixirAnalyzer do

defp do_init(params, exercise_config) do
meta_config = Path.join(params.path, @meta_config) |> File.read!() |> Jason.decode!()
relative_code_path = meta_config["files"]["solution"] |> hd()
code_path = Path.join(params.path, relative_code_path)
solution_files = meta_config["files"]["solution"] |> Enum.map(&Path.join(params.path, &1))
if Enum.empty?(solution_files), do: raise("No solution files specified")

{exercise_type, exemploid_path} =
submitted_files =
Path.join([params.path, "lib", "**", "*.ex"])
|> Path.wildcard()
|> Enum.concat(solution_files)
|> Enum.uniq()
|> Enum.sort()

editor_files = Map.get(meta_config["files"], "editor", [])

{exercise_type, exemploid_files} =
case meta_config["files"] do
%{"exemplar" => [path | _]} -> {:concept, Path.join(params.path, path)}
%{"example" => [path | _]} -> {:practice, Path.join(params.path, path)}
%{"exemplar" => path} -> {:concept, path}
%{"example" => path} -> {:practice, path}
end

{code_path, exercise_type, exemploid_path,
exemploid_files =
(editor_files ++ exemploid_files) |> Enum.sort() |> Enum.map(&Path.join(params.path, &1))

{submitted_files, exercise_type, exemploid_files,
exercise_config[:analyzer_module] || ElixirAnalyzer.TestSuite.Default}
end

Expand All @@ -190,15 +202,15 @@ defmodule ElixirAnalyzer do
end

defp check(%Submission{source: source} = submission, _params) do
Logger.info("Attempting to read code file", code_file_path: source.code_path)
Logger.info("Attempting to read code files", code_file_path: source.submitted_files)

with {:code_read, {:ok, code_string}} <- {:code_read, File.read(source.code_path)},
with {:code_read, {:ok, code_string}} <- {:code_read, read_files(source.submitted_files)},
source <- %{source | code_string: code_string},
Logger.info("Code file read successfully"),
Logger.info("Attempting to read exemploid", exemploid_path: source.exemploid_path),
Logger.info("Code files read successfully"),
Logger.info("Attempting to read exemploid", exemploid_files: source.exemploid_files),
{:exemploid_read, _, {:ok, exemploid_string}} <-
{:exemploid_read, source, File.read(source.exemploid_path)},
Logger.info("Exemploid file read successfully, attempting to parse"),
{:exemploid_read, source, read_files(source.exemploid_files)},
Logger.info("Exemploid files read successfully, attempting to parse"),
{:exemploid_ast, _, {:ok, exemploid_ast}} <-
{:exemploid_ast, source, Code.string_to_quoted(exemploid_string)} do
Logger.info("Exemploid file parsed successfully")
Expand All @@ -208,36 +220,50 @@ defmodule ElixirAnalyzer do
{:code_read, {:error, reason}} ->
Logger.warning("TestSuite halted: Code file not found. Reason: #{reason}",
path: source.path,
code_path: source.code_path
submitted_files: source.submitted_files
)

submission
|> Submission.halt()
|> Submission.append_comment(%Comment{
comment: Constants.general_file_not_found(),
params: %{
"file_name" => Path.basename(source.code_path),
"file_name" => Path.basename(source.submitted_files),
"path" => source.path
},
type: :essential
})

{:exemploid_read, source, {:error, reason}} ->
Logger.warning("Exemploid file not found. Reason: #{reason}",
exemploid_path: source.exemploid_path
exemploid_files: source.exemploid_files
)

%{submission | source: source}

{:exemploid_ast, source, {:error, reason}} ->
Logger.warning("Exemploid file could not be parsed. Reason: #{inspect(reason)}",
exemploid_path: source.exemploid_path
exemploid_files: source.exemploid_files
)

%{submission | source: source}
end
end

defp read_files(paths) do
Enum.reduce_while(
paths,
{:ok, nil},
fn path, {:ok, code} ->
case File.read(path) do
{:ok, file} when is_nil(code) -> {:cont, {:ok, file}}
{:ok, file} -> {:cont, {:ok, code <> "\n" <> file}}
Comment on lines +256 to +260
Copy link
Member

Choose a reason for hiding this comment

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

I assume that the case is_nil(code) is only there because of the first iteration and initial value of the accumulator. What if we did something like this, appending new lines instead of prepending? We would end up with one extra newline at the very end, but it shouldn't matter, right? (suggested code not tested)

Suggested change
{:ok, nil},
fn path, {:ok, code} ->
case File.read(path) do
{:ok, file} when is_nil(code) -> {:cont, {:ok, file}}
{:ok, file} -> {:cont, {:ok, code <> "\n" <> file}}
{:ok, ""},
fn path, {:ok, code} ->
case File.read(path) do
{:ok, file} -> {:cont, {:ok, code <> file <> "\n"}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It almost doesn't matter, but in some unlikely cases it makes a line in an error message off-by-one (see CI fail before reverted commit). Line number kind of loses meaning when you have multiple files, but after reflection, I'd still like to keep the nil.

{:error, err} -> {:halt, {:error, err}}
end
end
)
end

# Analyze
# - Start the static analysis
defp analyze(%Submission{halted: true} = submission, _params) do
Expand Down
4 changes: 2 additions & 2 deletions lib/elixir_analyzer/exercise_test/common_checks.ex
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ defmodule ElixirAnalyzer.ExerciseTest.CommonChecks do

@spec run(Source.t()) :: [{:pass | :fail, Comment.t()}]
def run(%Source{
code_path: code_path,
submitted_files: submitted_files,
code_ast: code_ast,
code_string: code_string,
exercise_type: type,
Expand All @@ -46,7 +46,7 @@ defmodule ElixirAnalyzer.ExerciseTest.CommonChecks do
VariableNames.run(code_ast),
ModuleAttributeNames.run(code_ast),
ModulePascalCase.run(code_ast),
CompilerWarnings.run(code_path, code_ast),
CompilerWarnings.run(submitted_files),
BooleanFunctions.run(code_ast),
FunctionAnnotationOrder.run(code_ast),
ExemplarComparison.run(code_ast, type, exemploid_ast),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,30 @@ defmodule ElixirAnalyzer.ExerciseTest.CommonChecks.CompilerWarnings do
alias ElixirAnalyzer.Constants
alias ElixirAnalyzer.Comment

def run(code_path, _code_ast) do
import ExUnit.CaptureIO
Application.put_env(:elixir, :ansi_enabled, false)
def run(code_path) do
Logger.configure(level: :critical)

warnings =
capture_io(:stderr, fn ->
try do
Code.compile_file(code_path)
|> Enum.each(fn {module, _binary} ->
case Kernel.ParallelCompiler.compile(code_path) do
{:ok, modules, warnings} ->
Enum.each(modules, fn module ->
:code.delete(module)
:code.purge(module)
end)
rescue
# There are too many compile errors for tests, so we filter them out
# We assume that real code passed the tests and therefore compiles
_ -> nil
end
end)

warnings

{:error, _errors, _warnings} ->
# This should not happen, as real code is assumed to have compiled and
# passed the tests
[]
end

Logger.configure(level: :warn)

Application.put_env(:elixir, :ansi_enabled, true)

if warnings == "" do
if Enum.empty?(warnings) do
[]
else
[
Expand All @@ -35,9 +37,20 @@ defmodule ElixirAnalyzer.ExerciseTest.CommonChecks.CompilerWarnings do
type: :actionable,
name: Constants.solution_compiler_warnings(),
comment: Constants.solution_compiler_warnings(),
params: %{warnings: warnings}
params: %{warnings: Enum.map_join(warnings, &format_warning/1)}
}}
]
end
end

defp format_warning({filepath, line, warning}) do
[_ | after_lib] = String.split(filepath, "/lib/")
filepath = "lib/" <> Enum.join(after_lib)

"""
warning: #{warning}
#{filepath}:#{line}

"""
end
end
8 changes: 4 additions & 4 deletions lib/elixir_analyzer/source.ex
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,23 @@ defmodule ElixirAnalyzer.Source do
defstruct [
:slug,
:path,
:code_path,
:submitted_files,
:code_string,
:code_ast,
:exercise_type,
:exemploid_path,
:exemploid_files,
:exemploid_string,
:exemploid_ast
]

@type t() :: %__MODULE__{
slug: String.t(),
path: String.t(),
code_path: String.t(),
submitted_files: [String.t()],
code_string: String.t(),
code_ast: Macro.t(),
exercise_type: :concept | :practice,
exemploid_path: String.t(),
exemploid_files: [String.t()],
exemploid_string: String.t(),
exemploid_ast: Macro.t()
}
Expand Down
4 changes: 1 addition & 3 deletions lib/elixir_analyzer/test_suite/take_a_number_deluxe.ex
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ defmodule ElixirAnalyzer.TestSuite.TakeANumberDeluxe do
alias ElixirAnalyzer.Constants
alias ElixirAnalyzer.Source

use ElixirAnalyzer.ExerciseTest,
# this is temporary until we include editor files in compilation
suppress_tests: [Constants.solution_compiler_warnings()]
use ElixirAnalyzer.ExerciseTest

feature "uses GenServer" do
type :actionable
Expand Down
4 changes: 2 additions & 2 deletions test/elixir_analyzer/cli_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ defmodule ElixirAnalyzer.CLITest do
source: %Source{
code_ast: {:defmodule, _, _},
code_string: "defmodule Lasagna" <> _,
code_path: @lasagna_path <> "/lib/lasagna.ex",
submitted_files: [@lasagna_path <> "/lib/lasagna.ex"],
exemploid_ast: {:defmodule, _, _},
exemploid_string: "defmodule Lasagna" <> _,
exemploid_path: @lasagna_path <> "/.meta/exemplar.ex",
exemploid_files: [@lasagna_path <> "/.meta/exemplar.ex"],
exercise_type: :concept,
path: @lasagna_path,
slug: "lasagna"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ defmodule ElixirAnalyzer.ExerciseTest.CommonChecks.CompilerWarningsTest do
alias ElixirAnalyzer.ExerciseTest.CommonChecks.CompilerWarnings

test "Implementing a protocol doesn't trigger a compiler warning" do
filepath = "test_data/clock/lib/clock.ex"
assert CompilerWarnings.run(filepath, nil) == []
filepath = "test_data/clock/perfect_solution/lib/clock.ex"
assert CompilerWarnings.run([filepath]) == []
end
end
Loading