Resolve exercism/elixir-analyzer#191 : make extract_function_name/1 work when functions use guards

[jlirochon]

I wrote some tests but I don't know where they should be placed...

defmodule CompilerTest do
  use ExUnit.Case

  test "extract the name of simple functions" do
    quoted = quote do: def(foo(x, y), do: :a)
    assert Compiler.extract_function_name(quoted) == :foo
  end

  test "extract the name of functions using one guard" do
    quoted = quote do: def(foo(x, y) when is_atom(x), do: :a)
    assert Compiler.extract_function_name(quoted) == :foo
  end

  test "extract the name of functions using multiple guards" do
    quoted = quote do: def(foo(x, y) when is_atom(x) when is_integer(y), do: :a)
    assert Compiler.extract_function_name(quoted) == :foo
  end

  test "extract the name of functions using one guard with \"and\" operator" do
    quoted = quote do: def(foo(x, y) when is_atom(x) and is_integer(y), do: :a)
    assert Compiler.extract_function_name(quoted) == :foo
  end
end

Mentoring welcome ! Can I do better ?

[angelikatyborska]

I think for the tests, the best option would be to create a new module in the directory test/elixir_analyzer/exercise_test/assert_call, something called for example multiple_clause_functions_test.exs which should have those test cases:

• single clause function, no guards
• single clause function, with a guard
• multiple clause functions with pattern matching in the arguments, no guards, only one of the clauses calls the function we're looking for
• multiple clause functions with guards, only one of the clauses calls the function we're looking for
• multiple clause functions with pattern matching in the arguments, no guards, multiple clauses call the function we're looking for
• multiple clause functions with guards, multiple clauses call the function we're looking for

In each case assert that the function call will be found.

But those wouldn't be unit tests of extract_function_name like you wrote in the PR description. They would be test of the assert_call macro in general. They would need to follow the same pattern as other tests in that directory, e.g. test/elixir_analyzer/exercise_test/assert_call/erlang_modules_test.exs. They all use the custom macro test_exercise_analysis to run the tests on actual code snippets and rely on a support analyzer module for tests only, like for example test/support/analyzer_verification/assert_call/erlang_modules.ex. It's hard to describe, but I hope that when you read an existing test, you'll know what to do 🙂.

Thank you for the PR ❤️ and let me know if you're able to add the tests. The would be more work than adding the fix itself.

[jlirochon]

Hi @angelikatyborska, I added some tests by following your instructions, thank you !

Not sure if it's good enough. Let me know if you want me to improve something 🙂.

[jiegillet]

Very nice! One thing you should add is a few tests where "does not call Map.new/0 in specific function" is triggered, because as of now, as far as we know (with a little bit of bad faith) in this test suite "found a call to Map.new/0 in function/1" could be defective and always get triggered, and "does not call Map.new/0 in specific function" be also defective and never get triggered.

[angelikatyborska]

The tests that you've added are beautiful 💜. I second @jiegillet suggestion, a few more tests for "didn't find a call to Map.new/0 in function/1" and it will be perfect.

[angelikatyborska]

Ah, and the mix credo warnings reported by CI, albeit very trivial, need to be resolved before merging.

[jlirochon]

Fixed credo warnings.

Before adding more tests I would like to have a better understanding of this block copied from sibling files :

  assert_no_call "does not call :rand.normal in specific function" do
    type :informative
    calling_fn module: AssertCallVerification, name: :function
    called_fn module: :rand, name: :normal
    comment "found a call to :rand.normal in function/0"
  end

I dont understand why the comment is the negative of the description. It seems counter-intuitive to me. Conversely if I want this comment to appear when testing some code blocks, why do I need to use assert_no_call instead of assert_call ? I tried to read the code of these macros but I'm unable to clarify what happens with regard to the comment.

[jiegillet]

I know how you feels, sometimes when I think too much about this I get lost too.

When you say assert_call "A calls B", the wanted behavior is A calling B, so it will only trigger a comment when A doesn't call B. assert_no_call is basically not assert_call. When you say assert_no_call "A doesn't call B", you don't want A calling B, so it will only trigger a comment when A does call B.

Basically for both cases, the comment is the opposite of the test description because it is only triggered when the test description isn't true.

[jlirochon]

@jiegillet Thank you for your explanation !

I think what confuses me the most with this implementation is that if I just want to check that "A calls B", the required block is assert_no_call "A doesn't call B" with a comment saying "A calls B". A block with assert_call "A calls B" is useless in this case. Because test_exercise_analysis expects comments to be triggered, we have to assert the opposite of what we want. Feels weird to me, maybe it would be more clear if we were comparing descriptions instead of comments.

Anyway I can process and add the tests 👍

[jiegillet]

Yes, it is true that both of your tests are testing the exact same thing. However it doesn't hurt to do it in that way because you can see the comments of the failing checks. Like I said, I feel the same way you do sometimes, but in some contexts it's very natural so... 🤷

[jlirochon]

Hi !

• [x] Fix mix credo warnings
• [x] Add tests triggering "didn't find a call to Map.new/0 in function/1"
• [x] Use import Map in some tests to prove that a call to new() is detected
• [x] Use alias Map, as: MyMap in some tests to prove that a call to MyMap.new() is detected

Thank you again for your patience and support 🙂 Please tell me if something should be further improved.