assert_equal should not allow nil for "expected"

[tenderlove]

If you are expecting the value to be nil, you should be using
assert_nil rather than passing nil in for the expected value. This
helps shorten your code, and also helps prevent situations where the
expected value becomes nil accidentally.

I wasn't sure what exception this should raise, so I just went with ArgumentError.

[zenspider]

Any reason not to use refute_nil ?

[tenderlove]

I tried that, but it messed up all the tests that count "number of assertions". Also, I'm not sure if this is an ArgumentError or an actual test failure.

[zenspider]

I'm fine with messing up the counts. I can fix those easily enough. I don't think this is an ArgumentError so much as it is a shitty way to test (slight semantic difference, but worth noting).

[zenspider]

I'm no longer fine with messing up the counts. Ugh I use assert_equal a LOT. It's even worse on the spec tests because they're not sandboxed the way the assertion tests are. If I switch to refute_nil, I'll jimmy the assertion count to make up for that.

[jeremyevans]

I don't think it makes sense to add arbitrary restrictions to assert_equal that break backwards compatibility. Also, consider:

assert_equal some_method(arg), other_method(arg)

[zenspider]

I don't think this is arbitrary and it makes sense and promotes better testing practices.

The example you give is an example of bad testing. Intentionality should be revealing in testing, not masked via indirection. If you know that some_method(arg) returns nil, then you can use assert_nil to say the exact same thing more clearly.

[zenspider]

@tenderlove ever hear of "seattle style"??

[tenderlove]

Fixed.

[jeremyevans]

@zenspider If you know in advance that some_method(arg) returns nil, then of course you would hard code it. However, you may not know the value in advance. I think it's valid to be able to test that two values are equal without knowing either value a priori. For example, when you are writing a replacement for an existing library, and you want to make sure that you return the same output as the existing library for arbitrary inputs.

At the very least, if you want to do this, you should bump the major version, cause this will definitely break backwards compatibility.

[tenderlove]

If you know in advance that some_method(arg) returns nil, then of course you would hard code it. However, you may not know the value in advance.

You will definitely know the value when it gets to assert_equal and that complains about nil (at which point you can switch to assert_nil).

The itch that this patch fixes for me is that any time I have assert_equal var1, var2, if those variables become nil, it's 100% a mistake, yet the test still passes. I've never hit a situation where I wanted that to happen.

As a compromise, maybe we could introduce a Minitest::Strict module. Then you could opt-in to the behavior by including the module. We use this behavior at work, so I'd like to upstream it.

[zenspider]

I just ran this across 50+ projects of mine and found 3-4 places I needed to change static nils and one place where my overly clever custom assertion needed to handle NilClass. Most could be solved with a quick sed if I wanted. That took me about 5-10 minutes total... but then I found a real bug that took me another 10-20 minutes to figure out how to solve. This was definitely hidden in the test:

    assert_equal site.layout("post"), page.layout

I think it is fair to say that any inconvenience caused by this change is far outweighed by the bugs it might uncover by inadvertent errors hidden by this assertion. I'm currently toying with having the behavior versioned so MT5 will warn and MT6 will raise. I'm not against Aaron's opt-in idea for MT5 (even tho nobody will use it), but I think this should be the standard behavior for MT6.

[zenspider]

It has been suggested that the nil check is not enforced unless they match... which basically means pushing the nil check to be after the equality check. This would mean if they don't match, you'd see regular assert_equal failure output first and had a better chance to debug it. Once you made it work, you'd potentially get the "Use assert_nil" failure.

Thoughts?

[chrisarcand]

It has been suggested that the nil check is not enforced unless they match..

"Makes sense" but really weird, unexpected behavior unless you happen to know about it.

I think the logic here is sound and your suggestion of just being opt in for one version and eventually the default in a later version is great. Straightforward, simple. 👍

[blowmage]

👍 for making this the change in minitest 6.

[zenspider]

This has been massaged heavily but is in. Thanks for the dialog!

[britishtea]

I guess I'm a little late to the party. I was surprised to see a screen full of warnings when I ran some tests today.

We have a few unit tests where we loop over a long list of expected inputs and outputs. Some of those expected outputs happen to be nil, triggering the warning.

An example:

  def test_it
    cases = [
      { in: true, out: true },
      { in: 123, out: 123 },
      { in: "", out: nil },
      { in: [], out: nil },
      ...
    ]

    cases.each do |test_case|
      actual = some_operation test_case[:in]
      assert_equal test_case[:out], actual
    end
  end

Any advice on keeping these kinds of tests free of warnings?

[jeremyevans]

@britishtea I believe you basically have to do:

if actual.nil?
  assert_nil test_case[:out]
else
  assert_equal test_case[:out], actual
end

[britishtea]

That is.. unfortunate. Oh well.

[GregMacdonald]

I see the value of both assert_equal and assert_equal_not_nil, but I find changing the semantics of assert_equal to be assert_equal_except.... very odd. In particular, I find breaking an existing contract rather abhorrent. But, I think the use cases you are thinking of are far too narrow.

Consider this. You have a custom cloning\deriving mechanism that copies selected properties. You can't compare the objects, you must compare individual properties. For some properties, it doesn't matter if they are nil or not, as long as those properties in the derived object are equal. Now you write a good test and then generate a bunch of objects to run through that test, which of course we do because we test multiple patterns, boundary conditions and the standard nil, zero, 1 and 2 conditions where appropriate.

With this change, what used to be reasonably neat now requires some really messy conditionals all over the place; or, of course, monkey-patching minitest.

[zenspider]

Requires? Really? Those are the two alternatives?

[GregMacdonald]

OK, you're right, requires is an excessive word in that sentence. Still, let's not distract ourselves from the point that you didn't add functionality, you replaced existing functionality, redefined the word and concept of equality, and left no way to simply test equality of two values. Equal is equal. Not equal is not equal. Equal with conditions is equal with conditions. We now have only a specialized case of the latter.

I recognize the value of defaulting the special definition of equal. So, while I think it is semantically confused, I would acquiesce to the argument if an equality method was provided. But this change doesn't just redefine equality, it removes the basic test for equality. So, it's not just a simple refactor because there is no method to refactor to. I don't get why it must be an absolute either-or question. We have lots of ones and zeros. Go ahead, take a few more.

[britishtea]

We're now using a helper method, `#assert_equal!`, implementing the old behaviour in tests where needed (see above) without muddying them up with conditionals. The implementation is trivial, but perhaps it'd be nice if Minitest 6 provided something like this?

…

Op 17 mrt. 2017 om 19:23 heeft Greg Macdonald ***@***.***> het volgende geschreven: OK, you're right, requires is an excessive word in that sentence. Still, let's not distract ourselves from the point that you didn't add functionality, you replaced existing functionality, redefined the word and concept of equality, and left no way to simply test equality of two values. Equal is equal. Not equal is not equal. Equal with conditions is equal with conditions. We now have only a specialized case of the latter. I recognize the value of defaulting the special definition of equal. So, while I think it is semantically confused, I would acquiesce to the argument if an equality method was provided. But this change doesn't just redefine equality, it removes the basic test for equality. So, it's not just a simple refactor because there is no method to refactor to. I don't get why it must be an absolute either-or question. We have lots of ones and zeros. Go ahead, take a few more. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[GregMacdonald]

@britishtea Great naming solution! Way better than my assert_f*king_equal ;) And it makes good sense from an API perspective. Thanks for the suggestion.

[zenspider]

Hyperbole, false dichotomy, and immaturity are three things I don't need in my life. Locking this thread down.