Detect and warn about incorrectly used ignore-comments (#325)

* Detect and warn about incorrectly used ignore-comments

Resolves #197.

* Keep existing ignoring behavior

* Improve formatting

* Test more ignore-related warnings

* Remove warning in the case of ignore-next-line at the EOF

* Test the warning output

* Add a changelog entry

* Adjust test descriptions

---------

Co-authored-by: Roman <205906+RKushnir@users.noreply.github.com>

Author: RKushnir

CHANGELOG.md

@@ -1,3 +1,9 @@
+Unreleased
+------
+#### Enhancements
+- Print warnings about incorrectly used ignore-markers (#325), such as start-marker
+without a corresponding stop-marker, or two start-markers without a stop-marker in-between etc.
+
 0.18.1
 ------
 #### Changes

lib/excoveralls/circle.ex

@@ -3,6 +3,7 @@ defmodule ExCoveralls.Circle do
   Handles circle-ci integration with coveralls.
   """
   alias ExCoveralls.Poster
+  alias ExCoveralls.Stats
 
   def execute(stats, options) do
     json = generate_json(stats, Enum.into(options, %{}))
@@ -20,7 +21,7 @@ defmodule ExCoveralls.Circle do
       service_number: get_number(),
       service_job_id: get_job_id(),
       service_pull_request: get_pull_request(),
-      source_files: stats,
+      source_files: Stats.serialize(stats),
       git: generate_git_info(),
       parallel: options[:parallel],
       flag_name: options[:flagname]

lib/excoveralls/drone.ex

@@ -3,6 +3,7 @@ defmodule ExCoveralls.Drone do
   Handles drone-ci integration with coveralls.
   """
   alias ExCoveralls.Poster
+  alias ExCoveralls.Stats
 
   def execute(stats, options) do
     json = generate_json(stats, Enum.into(options, %{}))
@@ -20,7 +21,7 @@ defmodule ExCoveralls.Drone do
       service_number: get_build_num(),
       service_job_id: get_build_num(),
       service_pull_request: get_pull_request(),
-      source_files: stats,
+      source_files: Stats.serialize(stats),
       git: generate_git_info(),
       parallel: options[:parallel],
       flag_name: options[:flagname]

lib/excoveralls/github.ex

@@ -3,6 +3,7 @@ defmodule ExCoveralls.Github do
   Handles GitHub Actions integration with coveralls.
   """
   alias ExCoveralls.Poster
+  alias ExCoveralls.Stats
 
   def execute(stats, options) do
     json = generate_json(stats, Enum.into(options, %{}))
@@ -20,7 +21,7 @@ defmodule ExCoveralls.Github do
     %{
       repo_token: get_env("GITHUB_TOKEN"),
       service_name: "github",
-      source_files: stats,
+      source_files: Stats.serialize(stats),
       parallel: options[:parallel],
       flag_name: options[:flagname],
       git: git_info()

lib/excoveralls/gitlab.ex

@@ -3,6 +3,7 @@ defmodule ExCoveralls.Gitlab do
   Handles gitlab-ci integration with coveralls.
   """
   alias ExCoveralls.Poster
+  alias ExCoveralls.Stats
 
   def execute(stats, options) do
     json = generate_json(stats, Enum.into(options, %{}))
@@ -23,7 +24,7 @@ defmodule ExCoveralls.Gitlab do
       service_number: get_number(),
       service_job_id: get_job_id(),
       service_pull_request: get_pull_request(),
-      source_files: stats,
+      source_files: Stats.serialize(stats),
       git: generate_git_info(),
       parallel: options[:parallel],
       flag_name: options[:flagname]

lib/excoveralls/ignore.ex

@@ -10,51 +10,167 @@ defmodule ExCoveralls.Ignore do
     Enum.map(info, &do_filter/1)
   end
 
+  defmodule State do
+    defstruct ignore_mode: :no_ignore,
+              coverage: [],
+              coverage_buffer: [],
+              warnings: [],
+              last_marker_index: nil
+  end
+
   defp do_filter(%{name: name, source: source, coverage: coverage}) do
-    lines = String.split(source, "\n")
-    list = Enum.zip(lines, coverage)
-           |> Enum.map_reduce(:no_ignore, &check_and_swap/2)
-           |> elem(0)
-           |> List.zip
-           |> Enum.map(&Tuple.to_list(&1))
+    source_lines = String.split(source, "\n")
 
-    [source, coverage] = parse_filter_list(list)
-    %{name: name, source: source, coverage: coverage}
-  end
+    processing_result =
+      Enum.zip(source_lines, coverage)
+      |> Enum.with_index()
+      |> Enum.reduce(%State{}, &process_line/2)
+      |> process_end_of_file()
 
-  defp check_and_swap({line, coverage}, ignore) do
-    {
-      coverage_for_line({line, coverage}, ignore),
-      ignore_next?(line, ignore)
-    }
+    updated_coverage = processing_result.coverage |> List.flatten() |> Enum.reverse()
+    warnings = Enum.sort_by(processing_result.warnings, &elem(&1, 0))
+    %{name: name, source: source, coverage: updated_coverage, warnings: warnings}
   end
 
-  defp parse_filter_list([]),   do: ["", []]
-  defp parse_filter_list([lines, coverage]), do: [Enum.join(lines, "\n"), coverage]
-
-  defp coverage_for_line({line, coverage}, ignore) do
-    if ignore == :no_ignore do
-      {line, coverage}
-    else
-      {line, nil}
+  defp process_line({{source_line, coverage_line}, index}, state) do
+    case detect_ignore_marker(source_line) do
+      :none -> process_regular_line(coverage_line, index, state)
+      :start -> process_start_marker(coverage_line, index, state)
+      :stop -> process_stop_marker(coverage_line, index, state)
+      :next_line -> process_next_line_marker(coverage_line, index, state)
     end
   end
 
-  defp ignore_next?(line, ignore) do
+  defp detect_ignore_marker(line) do
     case Regex.run(~r/coveralls-ignore-(start|stop|next-line)/, line, capture: :all_but_first) do
-      ["start"] -> :ignore_block
-      ["stop"] -> :no_ignore
-      ["next-line"] ->
-        case ignore do
-          :ignore_block -> ignore
-          _sth -> :ignore_line
-        end
-      _sth ->
-        case ignore do
-          :ignore_line -> :no_ignore
-          _sth -> ignore
-        end
+      ["start"] -> :start
+      ["stop"] -> :stop
+      ["next-line"] -> :next_line
+      _sth -> :none
     end
   end
 
+  defp process_regular_line(
+         coverage_line,
+         _index,
+         state = %{ignore_mode: :no_ignore, coverage_buffer: []}
+       ) do
+    %{state | coverage: [coverage_line | state.coverage]}
+  end
+
+  defp process_regular_line(_coverage_line, _index, state = %{ignore_mode: :ignore_line}) do
+    %{state | ignore_mode: :no_ignore, coverage: [nil | state.coverage]}
+  end
+
+  defp process_regular_line(_coverage_line, _index, state = %{ignore_mode: :ignore_block}) do
+    %{state | coverage: [nil | state.coverage]}
+  end
+
+  defp process_start_marker(
+         _coverage_line,
+         index,
+         state = %{ignore_mode: :no_ignore}
+       ) do
+    %{
+      state
+      | ignore_mode: :ignore_block,
+        coverage: [nil | state.coverage],
+        last_marker_index: index
+    }
+  end
+
+  defp process_start_marker(_coverage_line, index, state = %{ignore_mode: :ignore_block}) do
+    warning = {index, "unexpected ignore-start or missing previous ignore-stop"}
+
+    %{
+      state
+      | coverage: [nil | state.coverage],
+        warnings: [warning | state.warnings],
+        last_marker_index: index
+    }
+  end
+
+  defp process_start_marker(_coverage_line, index, state = %{ignore_mode: :ignore_line}) do
+    warning = {state.last_marker_index, "redundant ignore-next-line right before an ignore-start"}
+
+    %{
+      state
+      | ignore_mode: :ignore_block,
+        coverage: [nil | state.coverage],
+        warnings: [warning | state.warnings],
+        last_marker_index: index
+    }
+  end
+
+  defp process_stop_marker(_coverage_line, index, state = %{ignore_mode: :ignore_block}) do
+    %{
+      state
+      | ignore_mode: :no_ignore,
+        coverage: [nil | state.coverage],
+        last_marker_index: index
+    }
+  end
+
+  defp process_stop_marker(_coverage_line, index, state) do
+    warning = {index, "unexpected ignore-stop or missing previous ignore-start"}
+
+    %{
+      state
+      | ignore_mode: :no_ignore,
+        coverage: [nil | state.coverage],
+        warnings: [warning | state.warnings],
+        last_marker_index: index
+    }
+  end
+
+  defp process_next_line_marker(
+         _coverage_line,
+         index,
+         state = %{ignore_mode: :no_ignore}
+       ) do
+    %{
+      state
+      | ignore_mode: :ignore_line,
+        coverage: [nil | state.coverage],
+        last_marker_index: index
+    }
+  end
+
+  defp process_next_line_marker(
+         _coverage_line,
+         index,
+         state = %{ignore_mode: :ignore_block}
+       ) do
+    warning = {index, "redundant ignore-next-line inside ignore block"}
+
+    %{
+      state
+      | coverage: [nil | state.coverage],
+        warnings: [warning | state.warnings]
+    }
+  end
+
+  defp process_next_line_marker(
+         _coverage_line,
+         index,
+         state = %{ignore_mode: :ignore_line}
+       ) do
+    warning = {index, "duplicated ignore-next-line"}
+
+    %{
+      state
+      | coverage: [nil | state.coverage],
+        warnings: [warning | state.warnings],
+        last_marker_index: index
+    }
+  end
+
+  defp process_end_of_file(state = %{ignore_mode: :ignore_block}) do
+    warning =
+      {state.last_marker_index, "ignore-start without a corresponding ignore-stop"}
+
+    %{state | warnings: [warning | state.warnings]}
+  end
+
+  defp process_end_of_file(state), do: state
 end

lib/excoveralls/json.ex

@@ -2,6 +2,7 @@ defmodule ExCoveralls.Json do
   @moduledoc """
   Generate JSON output for results.
   """
+  alias ExCoveralls.Stats
 
   @file_name "excoveralls.json"
 
@@ -16,7 +17,7 @@ defmodule ExCoveralls.Json do
 
   def generate_json(stats, _options) do
     Jason.encode!(%{
-      source_files: stats
+      source_files: Stats.serialize(stats)
     })
   end
 

lib/excoveralls/local.ex

@@ -46,6 +46,7 @@ defmodule ExCoveralls.Local do
     enabled = ExCoveralls.Settings.get_print_summary
     if enabled and not ExCoveralls.ConfServer.summary_printed?() do
       coverage(stats, options) |> IO.puts()
+      warnings(stats) |> IO.write()
       ExCoveralls.ConfServer.summary_printed()
     end
   end
@@ -92,6 +93,12 @@ defmodule ExCoveralls.Local do
     end
   end
 
+  def warnings(stats) do
+    for stat <- stats, {line_num, message} <- stat[:warnings], into: "" do
+      print_string("\e[33mwarning:\e[m ~s\n  ~s:~b\n", [message, stat[:name], line_num + 1])
+    end
+  end
+
   defp sort(count_info, options) do
     if options[:sort] do
       sort_order = parse_sort_options(options)

lib/excoveralls/post.ex

@@ -3,6 +3,7 @@ defmodule ExCoveralls.Post do
   Handles general-purpose CI integration with coveralls.
   """
   alias ExCoveralls.Poster
+  alias ExCoveralls.Stats
 
   def execute(stats, options) do
     json = generate_json(stats, options)
@@ -17,7 +18,7 @@ defmodule ExCoveralls.Post do
       repo_token: options[:token],
       service_name: options[:service_name],
       service_number: options[:service_number],
-      source_files: source_info,
+      source_files: Stats.serialize(source_info),
       parallel: options[:parallel],
       flag_name: options[:flagname],
       git: generate_git_info(options)

lib/excoveralls/semaphore.ex

@@ -3,6 +3,7 @@ defmodule ExCoveralls.Semaphore do
   Handles semaphore-ci integration with coveralls.
   """
   alias ExCoveralls.Poster
+  alias ExCoveralls.Stats
 
   def execute(stats, options) do
     json = generate_json(stats, Enum.into(options, %{}))
@@ -20,7 +21,7 @@ defmodule ExCoveralls.Semaphore do
       service_number: get_build_num(),
       service_job_id: get_build_num(),
       service_pull_request: get_pull_request(),
-      source_files: stats,
+      source_files: Stats.serialize(stats),
       git: generate_git_info(),
       parallel: options[:parallel],
       flag_name: options[:flagname]