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

Custom assert message ignored when using '=' operator #10503

Closed azizk closed 3 years ago

azizk commented 3 years ago

Environment

Current behavior

The assertion fails but doesn't display the provided message.

assert 1 = 2, "1 is not 2"

# ** (MatchError) no match of right hand side value: 2

It's displayed if you use "==" for example.

assert 1 == 2, "1 != 2"

# ** (ExUnit.AssertionError) 
#
# 1 != 2

Comparison with assert without message.

assert 1 = 2

# ** (ExUnit.AssertionError)

# match (=) failed
# code:  assert 1 = 2
# left:  1
# right: 2

Expected behavior

assert 1 = 2, "1 is not 2"

# ** (ExUnit.AssertionError)

# 1 is not 2
# code:  assert 1 = 2
# left:  1
# right: 2
vinibrsl commented 3 years ago

Shouldn't match assertions raise ExUnit.AssertionError instead of a regular MatchError?

v0idpwn commented 3 years ago

The same happens with refute/2. It works this way because they're just functions, not macros as assert. I ain't sure if this is a bug, but if it is, it should be easy to fix making those functions macros. If this is indeed a bug and nobody wants it, I can take it :)

josevalim commented 3 years ago

After checking the APIs, this behaviour is correct. assert/1 is macro based and assert/2 simply executes the first argument, without doing any macro manipulation. You can do assert match?(...), "message" in your case. I will add an example to the docs.

azizk commented 3 years ago

Okay, but why not do the same macro manipulation in assert/2? I was hoping to get the comparison between the left and right-hand side and the highlighting of keys if it's a map, but with match?(...) you won't see that...

josevalim commented 3 years ago

@azizk you will see that if match?(...), you won't see that with assert/2. Once you pass your own message, then it means you are replacing the message ExUnit generates (which is the one with the diff).

azizk commented 3 years ago

@jose Yeah, I meant match? with assert/2. Now I see that you intend the message to be a complete replacement. In that sense it's working correctly. But maybe you'd like to reconsider because it would seem more consistent with assert/1, if assert/2 also printed a detailed AssertionError where the custom message is used. Moreover there's no easy way to print the left and right-hand side expressions in your custom message and if there was it would be tedious and impractical to do so.

I just noticed some code in the module that looks like this case was planned, or maybe it was left in unintentionally:

            _ ->
              left = unquote(Macro.escape(left))
              message =
                (message || "match (=) failed") <>
                  ExUnit.Assertions.__pins__(unquote(pins))
azizk commented 3 years ago

@josevalim Never mind the code I posted above. That was just an attempt I made some time ago. :)