Newsletter in Elixir track has incorrect automated feedback

[octowombat]

In submitting to the Elixir track my solution to the Newsletter exercise, I received the following automated feedback. I don't understand why it marks as Essential a code suggestion that my submitted code already has.

[jiegillet]

Hello @octowombat, thanks for opening the issue. Yup, it looks like a bug, I think the analyzer did not expect the when is_binary(path), it should be fixable.

On an unrelated note, @angelikatyborska do the comments not get their headers removed anymore? The #open_log uses options :write should not be there.

[octowombat]

Thanks @jiegillet for the fast response. Should I wait or attempt a fix myself?

[jiegillet]

You are very welcome to try it yourself! The main file to change is here, the simple fix would be to add more variants like

    form do
      def open_log(_ignore) when _ignore do
        _block_includes do
          File.open(_ignore, [:write])
        end
      end
    end

We would also need new tests in here.

I'll assign the issue to you, let me know if you need more guidance :)

[octowombat]

@jiegillet Thanks!

[octowombat]

@jiegillet

When running the test suite locally with mix test --include external I have got 6 failing tests not related to this issue. I have made sure I have everything setup I think (submodule updated). The 6 failing tests are all in test/elixir_analyzer_test.exs.

An example is as follows:

1) test ElixirAnalyzer for concept exercise failing solution that uses deprecated modules (ElixirAnalyzerTest)
     test/elixir_analyzer_test.exs:144
     Assertion with == failed
     code:  assert Submission.to_json(analyzed_exercise) == expected_output
     left:  "{\"comments\":[{\"type\":\"actionable\",\"comment\":\"elixir.solution.compiler_warnings\",\"params\":{\"warnings\":\"warning: HashDict.new/0 is deprecated. Use maps and the Map module instead\\n  lib/lasagna.ex:7\\n\\nwarning: HashSet.member?/2 is deprecated. Use the MapSet module instead\\n  lib/lasagna.ex:12\\n\\nwarning: HashSet.new/0 is deprecated. Use the MapSet module instead\\n  lib/lasagna.ex:12\\n\\nwarning: Behaviour.defcallback/1 is deprecated. Use the @callback module attribute instead\\n  lib/lasagna.ex:4\\n\\n\"}},{\"type\":\"informative\",\"comment\":\"elixir.general.feedback_request\"}],\"summary\":\"Check the comments for some suggestions. 📣\"}"
     right: "{\"comments\":[{\"type\":\"actionable\",\"params\":{\"warnings\":\"warning: HashDict.new/0 is deprecated. Use maps and the Map module instead\\n  lib/lasagna.ex:7\\n\\nwarning: HashSet.member?/2 is deprecated. Use the MapSet module instead\\n  lib/lasagna.ex:12\\n\\nwarning: HashSet.new/0 is deprecated. Use the MapSet module instead\\n  lib/lasagna.ex:12\\n\\nwarning: Behaviour.defcallback/1 is deprecated. Use the @callback module attribute instead\\n  lib/lasagna.ex:4\\n\\n\"},\"comment\":\"elixir.solution.compiler_warnings\"},{\"type\":\"informative\",\"comment\":\"elixir.general.feedback_request\"}],\"summary\":\"Check the comments for some suggestions. 📣\"}"
     stacktrace:
       test/elixir_analyzer_test.exs:152: (test)

This seems to imply that in this test that the JSON is not in the expected order when encoded as a string.

Do you have any idea why this is? Has a dependency been updated? Maybe it feels like a Map key ordering issue, I'll have a dig into the code too and take a look.

[octowombat]

Apart from the above query, I have the tests and fix ready to push as a PR.

[octowombat]

Some more details on the test failure: Looking into failing test

test/elixir_analyzer_test.exs:63
     Assertion with == failed
     code:  assert Submission.to_json(analyzed_exercise) == String.trim(expected_output)
     left:  "{\"comments\":[{\"type\":\"essential\",\"comment\":\"elixir.general.file_not_found\",\"params\":{\"file_name\":\"two_fer.ex\",\"path\":\"test_data/two_fer/missing_file_solution/\"}}],\"summary\":\"Check the comments for things to fix. 🛠\"}"
     right: "{\"comments\":[{\"type\":\"essential\",\"params\":{\"file_name\":\"two_fer.ex\",\"path\":\"test_data/two_fer/missing_file_solution/\"},\"comment\":\"elixir.general.file_not_found\"}],\"summary\":\"Check the comments for things to fix. 🛠\"}"
     stacktrace:
       (ex_unit 1.15.2) lib/ex_unit/capture_log.ex:113: ExUnit.CaptureLog.with_log/2
       (ex_unit 1.15.2) lib/ex_unit/capture_log.ex:75: ExUnit.CaptureLog.capture_log/2
       test/elixir_analyzer_test.exs:67: (test)

and grabbing the single comment in the list from the implementation, thus;

comment = %{
  type: :essential,
  comment: "elixir.general.file_not_found",
  params: %{
    "file_name" => "two_fer.ex",
    "path" => "test_data/two_fer/missing_file_solution/"
  }
}

Then encoding that comment to JSON gives;

iex(2)> Jason.encode!(comment)
"{\"type\":\"essential\",\"comment\":\"elixir.general.file_not_found\",\"params\":{\"file_name\":\"two_fer.ex\",\"path\":\"test_data/two_fer/missing_file_solution/\"}}"

which shows the field comment as the second encoded field in the object/map but the expected encoding in the test has this field further on in the object/map.

[angelikatyborska]

On an unrelated note, @angelikatyborska do the comments not get their headers removed anymore? The #open_log uses options :write should not be there.

Markdown syntax error. The comment needs to have a space after #.

[jiegillet]

Mmh, it smells of this change in OTP26, the order of the maps is not guaranteed. It's definitely not related to this issue, I'll see if I can fix it.

[octowombat]

I will hold off pushing the PR then until this is fixed and then I know all tests are passing.

[jiegillet]

I opened a PR #399

[jiegillet]

@octowombat you should be able to resume now.

[octowombat]

@jiegillet I have pulled in the update from the upstream repo but I am still getting 2 failures for the elixir_analyzer_tests.exs tests on line 26 and 160. I will investigate further as it is the same kind of error, i.e. a mismatch of the list of comments.

[octowombat]

The test on line 26 is strange since the expected list of comments has 11 elements whereas the resultant list within the analyzed_exercise Submission struct returned to the test has only 9 elements.

When I checkout at commit 7bb938aabaf9d8efe8aa94313f1a2a9383c5235e the resultant list has 11 elements but in that case the test fails on the sort ordering mismatch!

[jiegillet]

I commented on the PR :)

[octowombat]

Thanks, I reverted the deletion with a comment and also commented upon the question about the other variants.