100% test coverage #246

jiegillet · 2021-12-18T07:05:02Z

In #229, there was a discussion about automated dependency updates being safer with better test coverage, so I took it as a learning opportunity and chipped away at it and reached full coverage.

The type of changes changes I made were:

Adding tests
Removing dead code
Refactoring
Fixing bugs discovered by the extra tests
Duplicating some tests for feature, assert_call and check_source because for some reason the modules with use ElixirAnalyzer.ExerciseTest are not always taken into account if they are not defined in a *_test.exs file even if they are actually ran. This is probably due to the heavy macro use. These are a bit unfortunate.

I will add some comments on some parts where I have questions a bit later.

I used excoveralls to find the details of what was or wasn't covered, but I didn't commit that change not to add a dependency. I could do it though, and we could use https://coveralls.io/ to get a badge. I'd be happy to add that (in a different PR, I guess).

lib/elixir_analyzer.ex

jiegillet · 2021-12-18T13:00:55Z

lib/elixir_analyzer.ex

-
-  * `:path` - path to the submitted solution, defaults to the `path` parameter
-
-  * `:output_path` - path to write file output, defaults to the `path` parameter


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.

jiegillet · 2021-12-18T13:02:32Z

lib/elixir_analyzer/cli.ex

-  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>


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

jiegillet · 2021-12-18T13:04:34Z

lib/elixir_analyzer/cli.ex


  You may also pass the following options:
-    --skip-analysis                       flag skips running the static analysis


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.

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 🤔

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 :)

jiegillet · 2021-12-18T13:10:18Z

lib/elixir_analyzer/quote_util.ex

-  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


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.

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.

jiegillet · 2021-12-18T13:13:29Z

test/elixir_analyzer/exercise_test/check_source_test.exs

@@ -121,4 +121,89 @@ defmodule ElixirAnalyzer.ExerciseTest.CheckSourceTest do
      """
    ]
  end
+
+  test "full check_source definition in a test file for coverage" do
+    defmodule Coverage do


This is one of these cases where even though these things are being used somewhere else, somehow the coverage tool doesn't see it. It's a bit messy, but I suppose it doesn't hurt to have redundant tests.

jiegillet · 2021-12-18T13:19:26Z

test/elixir_analyzer_test.exs

@@ -150,6 +170,111 @@ defmodule ElixirAnalyzerTest do
    end
  end

+  describe "different failures" do
+    test "summary for a submission that did not run" do


This is a bit artificial, because I needed to find a test that gives a report with an incomplete analysis, but could not really find a branching in the code that would lead to this. From the point of view of Summary.summary/2 it makes sense to have that formatting option though...

jiegillet · 2021-12-18T13:23:18Z

test/elixir_analyzer/exercise_test/feature_test.exs

+
+      feature "skip" do
+        comment "skip"
+        status :skip


Do we really need to keep that status field? It might have been useful for debugging, but we don't actually use it anywhere in the code. I think it's commented out in two-fer.

My guess is that it would be safe to remove status. @neenjaw do you have any thoughts about this?

jiegillet · 2021-12-18T13:26:11Z

test/elixir_analyzer/exercise_test/feature_test.exs

+
+      feature "meta" do
+        comment "meta"
+        meta(keep_meta(true))


Same here, do we need this meta field?
I can't imagine an application where we would want to keep the meta information. In fact I'm pretty sure that setting keep_meta(true) is equivalent to "never match this form".

I think we can delete it. Forcing meta to match would mean forcing line numbers to match, which doesn't seem useful to me for anything. Adding an empty new line anywhere would mess up the whole analysis? Nah.

Ok, I'll get rid of it. In a different PR :)

angelikatyborska

Excellent job and very valuable changes 👏. Only one more fix to README.md is necessary before merging.

If you want to keep removing stuff (skip and metadata for example), can you open a new PR for that?

I could do it though, and we could use coveralls.io to get a badge. I'd be happy to add that (in a different PR, I guess).

Totally, go for it!

lib/elixir_analyzer/cli.ex

angelikatyborska · 2022-02-13T11:00:54Z

lib/elixir_analyzer.ex

@@ -256,7 +250,7 @@ defmodule ElixirAnalyzer do

    submission =
      submission
-      |> submission.analysis_module.analyze(submission.source)
+      |> submission.analysis_module.analyze()


Nice getting rid of a duplicated argument 👍

angelikatyborska · 2022-02-13T11:03:35Z

lib/elixir_analyzer/cli.ex


  You may also pass the following options:
-    --skip-analysis                       flag skips running the static analysis


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 🤔

angelikatyborska · 2022-02-13T11:15:21Z

lib/elixir_analyzer/exercise_test/common_checks/function_capture.ex

@@ -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}}


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

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

elixir-analyzer/test/elixir_analyzer/exercise_test/common_checks/function_capture_test.exs

Lines 53 to 58 in 926d69d

# 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.

angelikatyborska · 2022-02-13T11:16:49Z

lib/elixir_analyzer/exercise_test/feature.ex

+    unless Keyword.has_key?(feature_data, :comment) do
+      raise "Comment must be defined for each feature test"
+    end


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

angelikatyborska · 2022-02-13T11:22:45Z

lib/elixir_analyzer/quote_util.ex

-  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


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.

angelikatyborska · 2022-02-13T11:24:55Z

test/elixir_analyzer/exercise_test/feature_test.exs

+
+      feature "skip" do
+        comment "skip"
+        status :skip


My guess is that it would be safe to remove status. @neenjaw do you have any thoughts about this?

angelikatyborska · 2022-02-13T11:26:42Z

test/elixir_analyzer/exercise_test/feature_test.exs

+
+      feature "meta" do
+        comment "meta"
+        meta(keep_meta(true))


I think we can delete it. Forcing meta to match would mean forcing line numbers to match, which doesn't seem useful to me for anything. Adding an empty new line anywhere would mess up the whole analysis? Nah.

jiegillet · 2022-02-13T13:35:50Z

OK, I think I've addressed everything, please check my last 2 commits.
I'll wait for @neenjaw's input before removing :status but it would be in a different PR anyway.

Like always, thank you for your incredible thoroughness in your reviews, especially for this huge one!

Yay 100%!

jiegillet added 16 commits December 11, 2021 09:40

Refactor CLI and add coverage tests

05ac98f

Full coverage for elixir_analyzer.ex

c7464a5

99% coverage for assert_call

f537273

1.13 changed log [warn] to [warning]

541319f

Cover Support.Constants

367afaa

Cover 100% ElixirAnalyzer.ExerciseTestCase

d6d38e6

coverage for ElixirAnalyzer.Summary

2a928a6

Cover ElixirAnalyzer.Submission

c5ff3d9

Coverage for CheckSource

d6fc181

Coverage Feature

08dc424

Remove dead code from assert_call

74063df

Fix depth bug in function_capture.ex

7284304

Cover log formatter

d15d73d

Make duplicate features with different types or depth distinct

ff039e1

Rework feature tests coverage

65bbe0d

Remove QuoteUtil unused functions

ae496ca

jiegillet added x:action/improve Improve existing functionality/content x:module/analyzer Work on Analyzers x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises) x:size/large Large amount of work labels Dec 18, 2021

jiegillet commented Dec 18, 2021

View reviewed changes

angelikatyborska approved these changes Feb 13, 2022

View reviewed changes

jiegillet added 2 commits February 13, 2022 21:42

Remove mention of obsolete CLI options in README

11c53f9

More defensive depth check, add failing test for before 7284304

db5858f

angelikatyborska merged commit 2502844 into exercism:main Feb 14, 2022

jiegillet deleted the jie-coverall branch February 16, 2022 13:28

This was referenced Jul 14, 2022

Document reputation label exercism/docs#347

Merged

Consider correct reputation amounts for different labels exercism/exercism#6440

Open

ErikSchierboom mentioned this pull request Jul 14, 2022

Consider correct reputation amounts for different labels exercism/exercism#6441

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

100% test coverage #246

100% test coverage #246

jiegillet commented Dec 18, 2021 •

edited

Loading

jiegillet Dec 18, 2021

jiegillet Dec 18, 2021

jiegillet Dec 18, 2021

angelikatyborska Feb 13, 2022

jiegillet Feb 13, 2022

jiegillet Dec 18, 2021

angelikatyborska Feb 13, 2022

jiegillet Dec 18, 2021

jiegillet Dec 18, 2021

jiegillet Dec 18, 2021

angelikatyborska Feb 13, 2022

jiegillet Dec 18, 2021

angelikatyborska Feb 13, 2022

jiegillet Feb 13, 2022

angelikatyborska left a comment

angelikatyborska Feb 13, 2022

angelikatyborska Feb 13, 2022

angelikatyborska Feb 13, 2022

jiegillet Feb 13, 2022

angelikatyborska Feb 13, 2022

angelikatyborska Feb 13, 2022

angelikatyborska Feb 13, 2022

angelikatyborska Feb 13, 2022

jiegillet commented Feb 13, 2022


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

		* `:output_path` - path to write file output, defaults to the `path` parameter


		You may also pass the following options:
		--skip-analysis flag skips running the static analysis

	# 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

100% test coverage #246

100% test coverage #246

Conversation

jiegillet commented Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelikatyborska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiegillet commented Feb 13, 2022

jiegillet commented Dec 18, 2021 •

edited

Loading