Better assertion messages when StatsD expectation fails, but an exception occurred as well

[wvanbergen]

Consider this example:

assert_raises(RuntimeError) do
  assert_statsd_increment("blabla") do
    raise "expected"
  end
end

This used to pass, even though no blabla metric was emitted. This was fixed in #166 by checking the emitted packets in an ensure block.

But, now consider this example:

assert_statsd_increment("blabla") do
  raise "unexpected"
  StatsD.increment('blabla')
end

This test now gives us a less than useful assertion message ("expected 'blabla', but that didn't happen"). In most cases, the user needs to know that the exception occurred instead to debug the problem.

Unfortunately we cannot know if the exception that happened inside a block will be handled by assert_raises or not, and act accordingly. So instead, we include the exception information in our assertion message, which should help the user debug the problem.

[casperisfine]

This seems weird to me. IMO assert_statsd_increment shouldn't handle exceptions at all.

If you want to both assert that your code raises and that it emitted some metrics, then the assert_raises should be inside:

assert_statsd_increment("blabla") do
  assert_raises(RuntimeError) do
    raise "expected"
  end
end

Or am I missing something?

[wvanbergen]

Swapping the assert_ calls works, but I am not sure how obvious it is that there is a problem.

When looking at this and not knowing how the assertion methods are implemented, I would expect the first example to fail because it obviously does not emit the metric that we are asserting for. Worse, if you make this wrong assumption about how these methods work, it will not blow up in your face but silently pass and hide potential bugs.

As evidence that this is not obvious to people: we found a couple of test bugs after trying to upgrade statsd-instrument to a version that includes #166: test started to fail because the assert_statsd_increment call was silently not actually doing anything before.

[casperisfine]

@wvanbergen I'm not denying that this is an obvious footgun. However most block based assertions are the same way, see assert_difference for instance, same thing for the various assert_job_*.

But if we really want to make it bullet proof, then I think a better way would be to simply to re-raise a Minitest::Assertion or similar with a proper message explaining how to handle it.

[wvanbergen]

But if we really want to make it bullet proof, then I think a better way would be to simply to re-raise a Minitest::Assertion or similar with a proper message explaining how to handle it.

I don't think I understand what you mean with this. Isn't that exactly what we are doing? We assert, which raises a Minitest::Assertion. However, assert_raises ignores (re-raises) those exceptions, so minitest will report on the original assertion failure rather than reporting that Minitest::Assertion was not the expected exception. This PR adds "a proper message explaining how to handle it".

[casperisfine]

I don't think I understand what you mean with this. Isn't that exactly what we are doing?

Not really no.

My point is that block based assertions (not just this one, but also the Rails core ones) should implicitly expect that nothing is raised.

I think I should start an upstream discussion on this, but not sure I'll have time today.

In the meantime 👍

[wvanbergen]

Oh wait, now I think I get what you meant: Something like

begin
  yield
  assert stuff
rescue
  flunk("An exception occurred - if this is expected please use `assert_raises`")
end

[casperisfine]

Something like

Pretty much yeah. And I think all core assertions should do the same. Because:

assert_raises FooError do
  assert_difference -> { Foo.count }, +1 do
    Foo.insert!
  end
end

Have the exact same footgun you are trying to prevent. So it's a much more general minitest (or AS::TestCase because I don't want to report that to minitest) issue.

[wvanbergen]

I will merge this PR for now because this is better than what we currently have, but I like that idea - even if we only do it for our own exceptions.

[wvanbergen]

In this particular case, silently passing is quite bad, because "check whether a StatsD counter gets incremented when an exception occurs" turns out to be a quite common pattern.

Your solution will work fine as well: the assertion message can just tell people to swap the assert_raises with the assert_statsd_increment around.

[wvanbergen]

The reason I built this is when turning on strict mode, rather than getting useful ArgumentError exceptions telling me what was broken, I was getting assertion failures telling me that some counter wasn't incremented.

Given that we are going to ship the next version with strict mode so people can prepare for the backwards incompatible changes in the future, I think we have to include this.

For the next major release, I will prepare a change that uses the alternative approach.