elixir-lang / elixir

Elixir is a dynamic, functional language for building scalable and maintainable applications
https://elixir-lang.org/
Apache License 2.0
24.34k stars 3.36k forks source link

Unexpected warning in 1.11 #10412

Closed fxn closed 3 years ago

fxn commented 3 years ago

Yo! Elixir 1.11 warns that the variable reason is unused in this test:

test "the failure reason is logged as a warning", %{socket: socket} do
  payload = Map.delete(@default_payload, "longitude")
  reason = ["longitude can't be blank"]

  failed_event = fn ->
    ref = push(socket, @message, payload)
    assert_reply ref, :error, %{reason: reason} # <-- WARNING POINTS TO THIS LINE
  end

  assert_log :warn, failed_event, "... #{inspect(reason)} ..."
end

where assert_log is a function:

def assert_log(assertion_level, function, message) do
  original_level = Logger.level()
  Logger.configure(level: assertion_level)

  try do
    captured = capture_log([level: assertion_level], function)
    ExUnit.Assertions.assert(String.contains?(captured, message))
  after
    Logger.configure(level: original_level)
  end
end

Is that warning legit?

ericmj commented 3 years ago

Yes, it looks like the warning is correct. assert_reply is a macro that binds the variable reason. Since it's bound in an anonymous function it will not escape the scope the function and it is indeed unused.

The assert_reply macro in phoenix could be improved to suppress the warning, like is done for the assert_receive etc. macros in ex_unit.

EDIT: Since reason is not used you can fix the warning by adding an underscore: assert_reply ref, :error, %{reason: _reason}.

fxn commented 3 years ago

Oh, I see, the payload is a pattern of course (it is documented).

In this case, I believe I should pin reason because I want to test the payload:

assert_reply ref, :error, %{reason: ^reason}

So 1.10 did not issue a warning here and the suite passed. 1.11 uncovered this test was not testing what I wanted to test. Excellent!

Thanks @ericmj!