Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`assert_equal` should not allow `nil` for "expected" #626

Closed
wants to merge 2 commits into from

Conversation

@tenderlove
Copy link
Member

tenderlove commented May 13, 2016

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.

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.
@@ -170,6 +170,7 @@ def assert_empty obj, msg = nil
# See also: Minitest::Assertions.diff

def assert_equal exp, act, msg = nil
raise ArgumentError, "use assert_nil if expecting nil" if exp.nil?

This comment has been minimized.

Copy link
@zenspider

zenspider May 17, 2016

Member

Any reason not to use refute_nil ?

This comment has been minimized.

Copy link
@tenderlove

tenderlove May 17, 2016

Author Member

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.

This comment has been minimized.

Copy link
@zenspider

zenspider May 19, 2016

Member

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).

This comment has been minimized.

Copy link
@zenspider

zenspider May 20, 2016

Member

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
Copy link

jeremyevans commented May 19, 2016

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
Copy link
Member

zenspider commented May 19, 2016

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 zenspider self-assigned this May 19, 2016
@@ -20,6 +20,13 @@ class TestMinitestUnit < MetaMetaMetaTestCase
"#{MINITEST_BASE_DIR}/test.rb:139:in `run'",
"#{MINITEST_BASE_DIR}/test.rb:106:in `run'"]

def test_assert_equal_does_not_allow_lhs_nil
exp = assert_raises(ArgumentError) do

This comment has been minimized.

Copy link
@zenspider

zenspider May 19, 2016

Member

@tenderlove ever hear of "seattle style"??

This comment has been minimized.

Copy link
@tenderlove

tenderlove May 20, 2016

Author Member

Fixed.

@jeremyevans
Copy link

jeremyevans commented May 19, 2016

@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
Copy link
Member Author

tenderlove commented May 20, 2016

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.

I am an embarrassment. Why did I add parenthesis?  This commit is my
penance.
@zenspider
Copy link
Member

zenspider commented May 20, 2016

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
Copy link
Member

zenspider commented May 20, 2016

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
Copy link

chrisarcand commented May 21, 2016

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
Copy link

blowmage commented May 21, 2016

👍 for making this the change in minitest 6.

@zenspider
Copy link
Member

zenspider commented Oct 8, 2016

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

@britishtea
Copy link

britishtea commented Feb 22, 2017

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
Copy link

jeremyevans commented Feb 22, 2017

@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
Copy link

britishtea commented Feb 27, 2017

That is.. unfortunate. Oh well.

@GregMacdonald
Copy link

GregMacdonald commented Mar 17, 2017

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
Copy link
Member

zenspider commented Mar 17, 2017

Requires? Really? Those are the two alternatives?

@GregMacdonald
Copy link

GregMacdonald commented Mar 17, 2017

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
Copy link

britishtea commented Mar 17, 2017

@GregMacdonald
Copy link

GregMacdonald commented Mar 17, 2017

@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.

@seattlerb seattlerb locked and limited conversation to collaborators Mar 18, 2017
@zenspider
Copy link
Member

zenspider commented Mar 18, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.