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

Add new check_source check to check source code as a string #189

Merged
merged 12 commits into from
Oct 14, 2021
1 change: 1 addition & 0 deletions lib/elixir_analyzer/constants.ex
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ defmodule ElixirAnalyzer.Constants do
# German Sysadmin Comments
german_sysadmin_no_string: "elixir.german-sysadmin.no_string",
german_sysadmin_use_case: "elixir.german-sysadmin.use_case",
german_sysadmin_no_integer_literal: "elixir.german-sysadmin.no_integer_literal",

# Guessing Game Comments
guessing_game_use_default_argument: "elixir.guessing-game.use_default_argument",
Expand Down
11 changes: 10 additions & 1 deletion lib/elixir_analyzer/exercise_test.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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.CheckSource.Compiler, as: CheckSourceCompiler
alias ElixirAnalyzer.ExerciseTest.CommonChecks

alias ElixirAnalyzer.Submission
Expand All @@ -14,6 +15,7 @@ defmodule ElixirAnalyzer.ExerciseTest do
quote do
use ElixirAnalyzer.ExerciseTest.Feature
use ElixirAnalyzer.ExerciseTest.AssertCall
use ElixirAnalyzer.ExerciseTest.CheckSource
use ElixirAnalyzer.ExerciseTest.CommonChecks
@suppress_tests unquote(opts)[:suppress_tests]

Expand All @@ -31,17 +33,23 @@ defmodule ElixirAnalyzer.ExerciseTest do
# credo:disable-for-previous-line Credo.Check.Refactor.CyclomaticComplexity
feature_test_data = Module.get_attribute(env.module, :feature_tests)
assert_call_data = Module.get_attribute(env.module, :assert_call_tests)
check_source_data = Module.get_attribute(env.module, :check_source_tests)
suppress_tests = Module.get_attribute(env.module, :suppress_tests, [])

# ast placeholder for the submission code ast
# placeholders for submission code
code_ast = quote do: code_ast
code_as_string = quote do: code_as_string

# compile each feature to a test
feature_tests = Enum.map(feature_test_data, &FeatureCompiler.compile(&1, code_ast))

# compile each assert_call to a test
assert_call_tests = Enum.map(assert_call_data, &AssertCallCompiler.compile(&1, code_ast))

# compile each assert_call to a test
jiegillet marked this conversation as resolved.
Show resolved Hide resolved
jiegillet marked this conversation as resolved.
Show resolved Hide resolved
check_source_tests =
Enum.map(check_source_data, &CheckSourceCompiler.compile(&1, code_as_string))

quote do
@spec analyze(Submission.t(), String.t(), nil | Macro.t()) :: Submission.t()
def analyze(%Submission{} = submission, code_as_string, exemplar_ast) do
Expand All @@ -60,6 +68,7 @@ defmodule ElixirAnalyzer.ExerciseTest do
Enum.concat([
unquote(feature_tests),
unquote(assert_call_tests),
unquote(check_source_tests),
CommonChecks.run(code_ast, code_as_string, exemplar_ast)
])
|> filter_suppressed_results()
Expand Down
101 changes: 101 additions & 0 deletions lib/elixir_analyzer/exercise_test/check_source.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
defmodule ElixirAnalyzer.ExerciseTest.CheckSource do
@moduledoc """
Defines a `check_source` macro that allows checking the source code
"""

@doc false
defmacro __using__(_opts) do
quote do
import unquote(__MODULE__)

@check_source_tests []
end
end

@doc """
Defines a macro which runs a boolean function on the source code.

This macro then collates the block into a map structure resembling:
test_data = %{
description: description,
type: :actionable,
comment: "message",
suppress_if: {"name of other test", :fail}
}
and an AST for the function
"""
defmacro check_source(description, do: block) do
:ok = validate_check_block(block)

test_data =
block
|> walk_check_source_block()
|> Map.put(:description, description)
|> Map.put_new(:type, :informative)

check = test_data.check

test_data =
test_data
|> Map.delete(:check)
# made into a key-val list for better quoting
|> Map.to_list()

unless Keyword.has_key?(test_data, :comment) do
raise "Comment must be defined for each check_source test"
end
neenjaw marked this conversation as resolved.
Show resolved Hide resolved

quote do
@check_source_tests [
{unquote(test_data), unquote(Macro.escape(check))} | @check_source_tests
]
end
end

@supported_expressions [:comment, :type, :suppress_if, :check]
defp validate_check_block({:__block__, _, args}) do
neenjaw marked this conversation as resolved.
Show resolved Hide resolved
Enum.each(args, fn {name, _, _} ->
if name not in @supported_expressions do
raise """
Unsupported expression `#{name}`.
The macro `check_source` supports expressions: #{Enum.join(@supported_expressions, ", ")}.
"""
end
end)
end

defp walk_check_source_block(block, test_data \\ %{}) do
{_, test_data} = Macro.prewalk(block, test_data, &do_walk_check_source_block/2)
test_data
end

defp do_walk_check_source_block({:comment, _, [comment]} = node, test_data) do
{node, Map.put(test_data, :comment, comment)}
end

@supported_types ~w(essential actionable informative celebratory)a
Copy link
Contributor

Choose a reason for hiding this comment

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

Either in this pr or another, this check for the supported check levels is probably something that is repeated. Can we extract this check to another module so we can just do something like this:

# ...
    if not AnalyzerCaseUtil.supported_case_type?(type) do
      raise #...
    end
# ...

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to do this in another PR (which I think is fine and keeps this PR's focus singular), can you open an issue and assign to me? I would like to do this if not already done, thanks.

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've thought about that actually, some code is now duplicated three times, which starts to smell like a refactor. I'd be happy to assign this to you, once this gets merged.

Maybe even

 defp do_walk_check_source_block({:type, _, [type]} = node, test_data) do
    AnalyzerCaseUtil.supported_case_type?(type, :check_source)

If it works well for the other cases.

Copy link
Contributor

@neenjaw neenjaw Oct 11, 2021

Choose a reason for hiding this comment

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

Pros and cons for sure. There is a cost to drying up code too much. If you have the bandwidth, reading 99 bottles of oop is a good discussion on this.

Passing the type of check (:check_source) creates a dependency in that future Util function because it then has to "know" about all of the types of check that exist which perhaps all it really need to know is if the type is one of the standard 4 types.

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 just thinking of inserting :check_source in the string raised, but I do see your point.

defp do_walk_check_source_block({:type, _, [type]} = node, test_data) do
if type not in @supported_types do
raise """
Unsupported type `#{type}`.
The macro `check_source` supports the following types: #{Enum.join(@supported_types, ", ")}.
"""
end

{node, Map.put(test_data, :type, type)}
end

defp do_walk_check_source_block({:suppress_if, _, [name, condition]} = node, test_data) do
{node, Map.put(test_data, :suppress_if, {name, condition})}
end

defp do_walk_check_source_block({:check, _, [source, [do: function]]} = node, test_data) do
function = {:fn, [], [{:->, [], [[source], function]}]}

{node, Map.put(test_data, :check, function)}
end

defp do_walk_check_source_block(node, test_data) do
{node, test_data}
end
end
30 changes: 30 additions & 0 deletions lib/elixir_analyzer/exercise_test/check_source/compiler.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
defmodule ElixirAnalyzer.ExerciseTest.CheckSource.Compiler do
@moduledoc false

alias ElixirAnalyzer.Comment

def compile({check_source_data, check_function}, code_string) do
name = Keyword.fetch!(check_source_data, :description)
comment = Keyword.fetch!(check_source_data, :comment)
type = Keyword.get(check_source_data, :type, :informative)
suppress_if = Keyword.get(check_source_data, :suppress_if, false)

test_description =
Macro.escape(%Comment{
name: name,
comment: comment,
type: type,
suppress_if: suppress_if
})

quote do
(fn string ->
if unquote(check_function).(string) do
{:pass, unquote(test_description)}
else
{:fail, unquote(test_description)}
end
end).(unquote(code_string))
end
end
end
10 changes: 10 additions & 0 deletions lib/elixir_analyzer/test_suite/german_sysadmin.ex
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,14 @@ defmodule ElixirAnalyzer.TestSuite.GermanSysadmin do
called_fn name: :case
comment Constants.german_sysadmin_use_case()
end

check_source "does not use integer literals for code points" do
type :actionable
comment Constants.german_sysadmin_no_integer_literal()

check(source) do
Copy link
Member

Choose a reason for hiding this comment

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

String source only? No AST?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... Originally I had both, so it's not like it's impossible. I removed it because it felt too complex, somehow. And since the AST is built from the string to begin with, it's very simple to use Code.string_to_quoted to get the AST.

I'm on the fence, so if you think it's better design, I will put it back in. The test I would add would probably be the same as the indentation check, that's the best use I can think of.

Copy link
Member

Choose a reason for hiding this comment

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

I can think of many checks that I would want to do on the AST that current macros do not allow, but you're right, we can just call Code.string_to_quoted. As long as it doesn't happen a thousand times in a single analyzer run, there's probably no need to worry about the performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. And if end up using ASTs many times, it won't be difficult to put in back in.

integers = ["252", "246", "228", "223"]
not Enum.any?(integers, &String.contains?(source, &1))
Copy link
Member

Choose a reason for hiding this comment

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

I'm worried that this check is a bit weak. It could be accidentally triggered by something silly in a comment, for example writing ?ü # 252 😁. What if we did the opposite check - ensure that the strings "?ü" etc are all in the source code? Maybe for all the special values from the exercise, so including a, z, and underscore. I just saw a solution that only used the question mark syntax for the special characters, but not the simple ones.

It's not much better, but at least it won't be falsely triggered by comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, it's a more robust check.

end
end
end
124 changes: 124 additions & 0 deletions test/elixir_analyzer/exercise_test/check_source_test.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
defmodule ElixirAnalyzer.ExerciseTest.CheckSourceTest do
use ElixirAnalyzer.ExerciseTestCase,
exercise_test_module: ElixirAnalyzer.Support.AnalyzerVerification.CheckSource

test_exercise_analysis "empty module",
comments: ["always return false", "didn't use multiline"] do
~S"""
neenjaw marked this conversation as resolved.
Show resolved Hide resolved
defmodule CheckSourceVerification do
end
"""
end

test_exercise_analysis "contains integer litterals",
comments: [
"always return false",
"used integer litteral from ?a to ?z",
"didn't use multiline"
] do
~S"""
defmodule CheckSourceVerification do
def foo(x) do
case x do
97 -> "a"
98 -> "b"
_ -> "z"
end
end
end
"""
end

test_exercise_analysis "contains integer but false positive",
comments: [
"always return false",
"used integer litteral from ?a to ?z",
"didn't use multiline"
] do
~S"""
defmodule CheckSourceVerification do
def best_version(game) do
case game do
"fifa" -> "fifa 98"
"tomb raider" -> "tomb raider II (1997)"
_ -> "goat simulator"
Copy link
Member

Choose a reason for hiding this comment

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

Hard to argue with that code 👍

end
end
end
"""
end

test_exercise_analysis "uses multiline strings",
comments: ["always return false"] do
[
~S'''
defmodule CheckSourceVerification do
@moduledoc """
this module doesn't do much
"""
end
''',
~S'''
defmodule CheckSourceVerification do
def foo do
"""
all
you
need
is
love
"""
end
end
''',
~S"""
defmodule CheckSourceVerification do
def foo do
'''
love
is
all
you
need
'''
end
end
"""
]
end

test_exercise_analysis "short module",
comments: ["always return false", "module is too short", "didn't use multiline"] do
~S"""
defmodule C do
end
"""
end

test_exercise_analysis "badly formatted modules",
comments: ["always return false", "didn't use multiline", "module is not formatted"] do
[
~S"""
defmodule CheckSourceVerification do
def foo(), do: :ok
end
""",
~S"""
defmodule CheckSourceVerification do



def foo, do: :ok

end
""",
~S"""
defmodule CheckSourceVerification do
end
""",
~S"""
defmodule CheckSourceVerification do end
"""
]
end
end
Loading