MT 6 roadmap

[pathawks]

Use assert_nil if expecting nil from jekyll/test/test_page.rb:99:in `block (4 levels) in <class:TestPage>'. This will fail in MT6.

Where can I find a roadmap of expected changes in MT6?

[tamia]

can we please not do this change ... this will break so many existing tests ... and it will make it less intuitive to use ... every other test framework I know of supports equal nil :(
@zenspider

[zenspider]

It doesn't break anything, yet.

[tamia]

that's why I said "it will" ;)
... please don't do this change or at least ask for feedback from more people before doing it.

[zenspider]

I'm going to do this change. It will go out on a major rev. You are not being forced to update.

Yes, this will cause some work for some tests, but it also exposes tautological assertions and is worth it in the long run. It has exposed real bugs both across my many projects as well as in github's tests. It is worth it.

[zenspider]

@pathawks there is no official roadmap. It's in my head.

[utilum]

@zenspider it would be sweet of you to share what's in your head a little more, even be a bit structured about it.

[zenspider]

It would be sweet... it'd also be sweet to finish the minitest book and stay on top of my 90+ projects as well as do all my client work. OH! And have a social life and people-time to stay sane... Alas, I can't do all the things and I must prioritize what I actually do and when.

If someone wants to sponsor some OSS time and prioritize minitest, then sure... I'd be happy to get right on that. Until then, it'll get done when it gets done (or not at all, maybe (probably)).

[utilum]

I can't answer for budget allocation, as I don't have any to allocate, but I'd be happy to allocate time regularly, and I suspect there's a couple other folk who would. Any way to help you? Perhaps with #637 ?

[lazyatom]

I'm pretty happy changing my assert_equal nil, x to assert_nil x, but it becomes a little harder when the assertions' expected values are themselves using variables, i.e. assert_equal y, x where y might be nil.

In concrete terms, our test suites sometimes contain methods that wrap assertions to better capture our domain, e.g.

def assert_domain_concept(a, b, c, object)
  assert_equal a, object.method_relating_to_a, "error message referencing domain concept"
  assert_in_delta b, c, object.method_relating_to_b_and_c, "another descriptive error"
  # ...
end

I can't see a way of avoiding this deprecation message (or indeed this breaking upon MT6) without adding conditionals into the method, e.g.

def assert_domain_concept(a, b, c, object)
  if a.nil?
    assert_nil object.method_relating_to_a, "message"
  else
    assert_equal a, object.method_relating_to_a, "..."
  end
  # ...
end

... but given enough of these cases, I'd be sorely tempted to add a new method to our suites along the lines of:

def assert_equal_or_nil(expected, *args)
  if expected.nil?
    assert_nil(*args)
  else
    assert_equal(expected, *args)
  end
end

... which is just reintroducing something that MT seems to believe is a smell.

Do you have any advice about how we should proceed to best fit in with the MT philosophy?

[thesmart]

The warning doesn't make sense when actually comparing two things that happen to be nil. For example:

assert_equal expected_thing, actual_thing

assert_equal is correctly verifying that two things are equal. assert_nil means something different.

Is this change in MT6 purely philosophical or based on a breaking change?

[canoeberry]

Yes. This change is a mistake if you cannot

assert_equal a, b

where a might be nil.

[jrochkind]

Can you provide any background for:

it also exposes tautological assertions and is worth it in the long run. It has exposed real bugs both across my many projects as well as in github's tests. It is worth it.

An example? I think many of us don't understand the intent here.

[mtgrosser]

It is up to the language and its methods to define the notion of "equality", not up to the testing framework:

foo = nil
nil == foo => true
nil.eql?(foo) => true
nil.equal?(foo) => true
nil === foo => true

Therefore, assert_nil should be used whenever nil is expected, and assert_equal should be used when we expect equality. The testing framework should not try to nudge people towards becoming better programmers, there are other tools for that.

Also, when making assertions against variables, it is good practice to ensure they actually contain what you think they contain. Again, this is of no concern to the assertion of equality.

I suggest introducing a new assertion for this behaviour, with an appropriate name which reflects the expectation of equality to a non-nil value. This would be more logical, more expressive and would not be breaking things.

[donv]

I regularly test methods using a set of expected return values based on a set of input values. The return value can be nil for some input values. For this assert_equal works well now, but will break with MiniTest 6, and I have to add more logic even if assert_equal alone perfectly conveys the intent of the test.

Please revert this change or suggest another way of writing these tests. I would like to keep up to date with MiniTest since it it awesome.

[zenspider]

I will not be reverting this change.

I regularly test methods using a set of expected return values based on a set of input values.

Yes, this is a basic description of testing. We all do it. This is good.

The return value can be nil for some input values.

Here's where the problem starts to reveal itself.

You're using known input values and have known expected output values. So when you know and expect it to be nil, you use assert_nil instead. It's that simple. This isn't a risky change, and can be done with a simple command like (untested):

sed -i -e 's/assert_equal nil,/assert_nil/' test/**/*.rb

If you do not know when it is supposed to be nil, that's a separate and very important issue. It means you are not in control of your tests and you might have nils when you don't realize it. That is what this change is trying to expose. Tautological assertions provide a false sense of security and cover up potential bugs. I found a few across my projects when I was trying out this change. @tenderlove found bugs in his projects too (he proposed this change). Real bugs. In production.

This is a good thing. Use this opportunity to look at why you're resisting the change to see if maybe you're making assumptions that cover up bugs in your code or thinking.

@randycoulman wrote a good blog post about this (and maybe he wants to chime in here?): http://randycoulman.com/blog/2016/12/20/tautological-tests/

[jrochkind]

Some of us find it frequently convenient to do things like:

{ "input" => "expected_output",
  "input_with_nil_expected" => nil }.each do |input, expected|
    assert_equal expected, something.something(input)
end

That's the pattern/use this breaks. But I get it, you think other considerations override this and won't be changing your mind, I'm not gonna try to argue it anymore, just wanted to clarify what people were talking about when they say things like "I regularly test methods using a set of expected return values based on a set of input values" -- I think this sort of thing is what several people in the thread are talking about, and how this change makes it that particular way of doing things no good anymore.

[donv]

@zenspider Thank you for your response. I am indeed looking looking to improve my code and thinking. I am changing all my assert_equal nil, ... to assert_nil .... No problem there. I like it. It conveys the meaning of the test better.

The situations where I have a problem is typically in test helpers, not in individual tests. Since I did not include an example in my previous post, I went through my concrete examples, and I found that they are all in test helpers comparing multiple values. So, in all my cases I switch from individual assert_equal e, a calls to assert_equal [e1, e2, ...], [a1, a2, ...] calls where some of the values can be nil depending on the context in which the test helper is used.

I think it is slightly less readable, but better than having a check if the expected value is nil and use assert_nil in that special case.

All in all, it is good enough for me, although it feels like a coincidence that all my cases where comparing multiple values.

[mshappe]

@jrochkind There's also quite a lot of us working with inherited code bases where the factories are just lazy 😄 After replacing all the explicit nil tests, it looks like the other places we're seeing this deprecation are places where the source data is also supplying nil (because lazy factories that only include data they must have), so the output is correctly returning nil as well, and that's been "good enough" for us until now. @zenspider is saying it's not good enough if what you really mean to do is test that for given values in, you'll get those values out, because nil could also represent a failure somewhere along the way. Philosophically, I have to admit I agree with him, even though updating our tests will be a bit of a pain.

[hovissimo]

I understand where you're trying to go with assert_nil, and I really agree overall, but please don't break equality testing with nil. Using assert_nil makes perfect sense when the programmer knows at write-time that the output of his test article is going to be nil. However, the programmer doesn't always know that! Your stance is going to force the programmer to be defensive about his tests, which will hurt readability, maintainability, and programmer satisfaction.

Here's an example pattern that's extremely common in my code. This is clean, efficient, easy to read and update, and your proposal will break that.

    samples = { ... }
    samples.each do |input, expected|
        actual = frob(input)
        assert_equal(expected, actual, "'#{input}' -> #{actual}, but should have been #{expected}")
    end

nil is a valid output in many cases, and you're planning on forcing me to code around MT6. Purity is nice, but practicality has to beat purity in the real world of real code. Your plan feels like Java, not Ruby.

[mshappe]

Using assert_nil makes perfect sense when the programmer knows at write-time that the output of his test article is going to be nil. However, the programmer doesn't always know that!

OK. I almost buy your second argument -- that if I'm testing a range of inputs for their expected outputs and want to do so efficiently in a loop, not allowing an test for equality that happens to include nil is a bit of a pain.

However, I reject the validity of the quoted statement above. If you don't know what your output will be for a given input, you're doing something very, very, very, very wrong.

[hovissimo]

To me, it's the same argument. The loop that iterates over and asserts test cases is agnostic to what the test cases actually are.

Let's say I have some test cases:

samples = {
    "foo" => "bar",
    "Foo" => "Bar",
    "baz" => "qux",
}
# assertion loop not included for brevity

All is well and good here, but requirements changed or I forgot a corner case and I need to add a new test case:

samples = {
    "foo" => "bar",
    "Foo" => "Bar",
    "baz" => "qux",
    ".{-+" => nil,
}
# assertion loop not included for brevity

BOOM! Tests have failed.

Now I'm left with either completely refactoring this test (and making it VERY repetitive), or changing my assertion loop to match what @lazyatom mentioned above:

samples.each do |input, expected|
    actual = frob(input)
    if expected.nil?
        assert_nil(actual, "'#{input}' -> #{actual}, but should have been nil")
    else
        assert_equal(expected, actual, "'#{input}' -> #{actual}, but should have been #{expected}")
    end
end

This new assertion block is just complicated enough to fail "immediate recognition", which is really important when reading/writing code. Also, it violates DRY.

If there's a better alternative than what I'm doing here, I'm all ears. As I touched on before, I'm a big fan of semantic assertions. They're a huge win in nearly all cases - I just think that forcing them has undesirable side effects that outweigh the benefit of encouraging adoption.

[mshappe]

I guess that I would argue that there's very little lost by having two entirely separate test cases, one that takes a hash of 'input' vs 'expected, and one that takes an array of inputs where 'expected' is always nil. That is, those things that are known to generate nil results get tested separately.

Point is: much of the time, nil is a perfectly valid answer to get back from a function, but you're should always know exactly when those times are, and they're often going to be edge cases where the input you got is actually not valid. Not-valid inputs ought to be tested for separately, explicitly, anyway, in their own test case (e.g. def test_invalid_inputs_return_nils), both so it's clear to another coder that you've thought about it and so that your invalid inputs aren't all mixed in with your valid ones!

[hovissimo]

@mshappe I like what you've suggested, and I think I'm going to adopt that for when I'm testing invalid -> nil - but invalid inputs aren't the only case when mixed (nil, not nil) outputs are equally valid. In reality, I don't think it's possible to declare "nil should always be tested separately" as a general rule. We could keep throwing examples and counter examples at each other, but I doubt either of us would be satisfied.

I'll just reiterate: I think the current plan is going to hurt MT and MT's community, and I urge you to abandon or modify that plan.

[joshuasiler]

I agree that assert_equal(foo, nil) should still be supported where foo is nil. It makes for an inconsistent API if it's not supported. Nil should be treated like a valid state.

[mshappe]

I'll just reiterate: I think the current plan is going to hurt MT and MT's community, and I urge you to abandon or modify that plan.

For the most part, as should be apparent here, I agree with @zenspider 's decision on philosophical grounds. nil is not just another value. nil is the absence of value and should be treated separately.

That said, minitest has been around a long time with its current semantics, and this is a pretty significant change. At the very least, I might make an argument for a longer deprecation cycle.

[canoeberry]

Man this is bordering on ridiculous. nil is a value. You can return it like any other value. Who are you to determine what nil means to me and my program?

I suppose the fact that if you return NOTHING it's also nil, but that's ruby for you.

[zenspider]

@canoeberry reign it in on the tone, please.

To answer your question: I am the author of minitest and I determine what nil means to minitest. What nil means to you and your program is between you and nil.

[zenspider]

@hovissimo:

Using assert_nil makes perfect sense when the programmer knows at write-time that the output of his test article is going to be nil. However, the programmer doesn't always know that!

I can't disagree more... If you don't know the value at the time you're writing the test, you're not testing, you're hoping.

[zenspider]

@joshuasiler it's assert_equal(expected, actual)... so there's that. And assert_nil has existed since the beginning, so there's nothing inconsistent... just exactly one less way to do something that already had valid alternatives. It also means there is one less way for an error to sneak in.

[zenspider]

To everyone: I actually appreciate the spiritedness of this thread, but I have to say that the objections to this seem really really overblown. (eta: try harder?) I was able to find and fix every case across many (> 75) projects in less than 10 minutes... (you might not believe that claim, but it's true. Also helps that I have my own CI locally) I even found a couple legit bugs in the process!

[zenspider]

@hovissimo btw... I think writing tests like that is terrible:

    samples = { ... }
    samples.each do |input, expected|
        actual = frob(input)
        assert_equal(expected, actual, "'#{input}' -> #{actual}, but should have been #{expected}")
    end

That means you only ever get ONE failure at a time... No pattern matching, no signaling that something deeper has happened. Just a single breadcrumb. That sucks!

Much better to use that loop to generate a bunch of test methods. Then you can see when multiple break at once and know that something bad happened deeper.

[tenderlove]

We've been monkey patching Minitest to support this feature at GitHub for years. The place where it saves us is assert_equal(some_complex_function, other_complex_function). If a developer changes app state (say fixtures or factories or whatever) or functionality such that both functions return nil, then the test continues to pass. However, at the time of writing the test the author most likely did not expect some_complex_function to return nil. And why would you? It's a complex function.

Of course this is an example of a tautological test, and yes, a tautological test is bad. But without the exception on nil in the "expected" side, there is no way to detect it's tautological in the first place.

This has been so helpful that the small effort it takes to explicitly call out the "I expect the return value to be nil" cases makes it worth while.

If there were a way to tell the difference between the return value of some_complex_function and just a literal nil being passed to assert_equal, I think that would be better. But since we don't have that, this seems like the next best thing.

That said, minitest has been around a long time with its current semantics, and this is a pretty significant change. At the very least, I might make an argument for a longer deprecation cycle.

I could get behind this. OTOH it's so easy to add an assert_equal that conditionally calls super based on the expected value that the escape valve is pretty easy to write.

[tenderlove]

Also I just wanted to say, I totally understand the "iterating over samples" case; I use it too. Just specifically calling out the "return value is nil" cases has been worth it to me for the safety this buys. Not to mention that, to me, returning nil is a Big Deal™ and I want those cases called out explicitly.

[asomers]

@tenderlove yours is the only example given in support of this change. But I feel that you're solving this problem at the wrong level. Instead of fundamentally changing the semantics of assert_equal, all you have to do is:

x = other_complex_function
assert_not_nil x
assert_equal some_complex_function, x

Alternatively, you yourself have pointed out that it's easy to subclass MiniTest::Test to override assert_equal. That's probably what you should've done for projects that require it. Instead, you've changed the semantics of assert_equal itself, completing violating POLA. Now that function should really be called assert_equal_but_not_nil.

And @zenspider, you're being so obtuse that I feel that you're deliberately misconstruing our criticisms.

I can't disagree more... If you don't know the value at the time you're writing the test, you're not testing, you're hoping.

What @hovissimo said made perfect sense. It's perfectly valid to expect that a == b without knowing at write-time whether they happen to be nil. He already gave a good example, so I'll not repeat it here. Instead, he and every other MiniTest user will have to use @lazyatom's antipattern, completely defeating the purpose of this change.

And what's so special about nil anyway? If you're going to refuse to test the equality of nil, why not do the same for other common flag values, like 0, -1, [], or NilClass? All of them can be valid, depending on the context. It's highly presumptuous of the test framework to declare that no normal code should ever return nil.

But, as you've said multiple times, it's your project, and you can do as you wish. The rest of us are all getting what we paid for. You won't hear from me again on this subject.

[tenderlove]

@tenderlove yours is the only example given in support of this change. But I feel that you're solving this problem at the wrong level. Instead of fundamentally changing the semantics of assert_equal, all you have to do is:

If I could see the future, yes. But as I said, at the time of writing the test the author did not expect the return value to be nil. For example a function that couldn't possibly return nil and someone later changed it to return nil.

[lazyatom]

It means you are not in control of your tests and you might have nils when you don't realize it.

For what it's worth, I do understand the tautological tests argument, and basically everything above about not being in control of when you expect nil, but that doesn't cover the situation that I described in my original comment, which is when one trying to use assert_equal as part of user-defined assertion method.

If you don't know the value at the time you're writing the test, you're not testing, you're hoping.

In the context of my example, when I write assert_domain_concept(a, b, nil, object), that nil is very much intentional and I'm entirely in control of it. It's just that assert_equal within another method doesn't know the value it's going to be passed.

I am the author of minitest and I determine what nil means to minitest. What nil means to you and your program is between you and nil.

Perhaps my example sits in a kind of grey area between these two zones of control...

I understand if the position you're taking is that test authors who want to build up a set of "higher-level assertions" like this are on their own, but I felt it was worth being clear that there is a case where you might be simultaneously in control of your nils, and still also want to pass arbitrary values to the same underlying assert_equal method.

[zenspider]

From @lazyatom:

For what it's worth, I do understand the tautological tests argument, and basically everything above about not being in control of when you expect nil, but that doesn't cover the situation that I described in my original comment, which is when one trying to use assert_equal as part of user-defined assertion method.

... actually it does. You're already writing your own user defined assertion. You've already refactored your tests into custom assertions. Adding an if expression to cover this has a cost of roughly zero. As an assertion writer, onus is on you to anticipate your edge cases and cover them. I still think that there are places where you can get hidden nil values that your assertion might cover up, but you're already operating on a level higher than 80-90% of the tests out there. I don't understand the opposition given how low the cost is for you.

The tautological test argument is for the cases where someone is just using assert_equal straight up and don't even realize that they're getting back a nil. It is hidden from them and the way they're thinking about tests. Minitest tries not only to be small and fast, but also responsible. It tries to make everything possible, but bad ideas harder. In this case, our perspective has changed on assert_equal and how it can make bad ideas (hidden nils) easy, so we're moving the goal posts.

This isn't for everyone. And for some, it'll be some work. We think that work is worth it in the long run. If you're not ready or willing for this change, then don't use unbounded dependencies on minitest on your gemfile. "~> 5.9.0" will prevent any deprecation warnings at all. "~> 5.0" will allow the warnings, but won't upgrade to the version that fails on nil.

[zenspider]

@lazyatom ... thank you for constructive dialog. Hyperbolic shit makes me want to lock down threads like this but people like you make it worth the noise.

[drush]

I'm going to add one more thought to this thread: both options are terrible.

Wrong (link to gem) is the right way to do this. Creating entirely new DSLs and method libraries for something that can so easily be handled from within the language itself is silly.

assert { left <operator> right } should do the right thing and provide contextually accurate suggestions when errors are detected. That's exactly what the Wrong gem provides.

assert_<operator> and assert_<value> (where equals is implicit) is far from a minimalist ideal. The permutations explode until one day you find yourself crying in a corner because you have reinvented rspec. Flame on.

[lighttroupe]

This change did uncover a few incorrect tests for me.

For those upgrading, three easy ways to upgrade your loop:
hash.each { |key, value| assert_equal value, my_model.send(key) }

1. Simplest hack:
   hash.each { |key, value| assert_equal [value], [my_model.send(key)] }

2. Bonus: also includes key in assertion failures for easier debugging:
   hash.each { |key, value| assert_equal [key, value], [key, my_model.send(key)] }

3. Shows you the whole story in one go:
   assert_equal hash.values, hash.keys.map { |key| my_model.send(key) }

[yourtallness]

I do understand that tests need to be explicit and nil warrants its own assertion.

That being said, for tests that evaluate whether e.g. a new object was copied properly from a fixture, one might like to test that fields will end up with the same respective values, whichever those are. I get that this has a notion of "not knowing what you are expecting" (hoping) and may mask errors, yet may also be a reasonable risk if you know that you're not changing the fixture itself.

Explicitless also becomes kind of a pain if you'd like to factor out some common assertions to a function which will work OK 9/10 times but throw a deprecation warning that 1 out of 10 times where a nil is passed in as the expected value.

[zenspider]

I don't think anyone is going to add anything new to this thread, as evidenced above... Locking and closing...