Don't hide underlying test failure in ActiveRecord::TestCase#assert_sql

[drcapulet]

If a test case raises an error that prevents a query from being executed, the ensure block will override that failure which hampers debugging efforts when writing test cases.

[skipkayhil]

Instead of wrapping in unless $!, couldn't you remove ensure?

[byroot]

Look like this one was missed in #37313

The best solution would be to use _assert_nothing_raised_or_warn, see: 4b42beb

[drcapulet]

Yeah, I can definitely simplify this - just the result of staring at this code too long. I'm a bit confused as to the use of _assert_nothing_raised_or_warn? Or is that just to have the underlying exception warn?

[byroot]

Or is that just to have the underlying exception warn?

Yes.

[drcapulet]

Ah gotcha - the case I was actually originally having issues wasn't necessarily an expectation failure, just a normal exception. Is there a reason the assertions are stuffed into the ensure block and not just:

def assert_sql(*patterns_to_match, &block)
  capture_sql(&block)

  failed_patterns = []

  patterns_to_match.each do |pattern|
     failed_patterns << pattern unless SQLCounter.log_all.any? { |sql| pattern === sql }
  end

  assert failed_patterns.empty?, "Query pattern(s) #{failed_patterns.map(&:inspect).join(', ')} not found.#{SQLCounter.log.size == 0 ? '' : "\nQueries:\n#{SQLCounter.log.join("\n")}"}"
end

[byroot]

the case I was actually originally having issues wasn't necessarily an expectation failure

Yes. _assert_nothing_raised_or_warn is not for assertion failures, but for regular app errors.

Is there a reason the assertions are stuffed into the ensure block and not just:

It was likely for the same reason _assert_nothing_raised_or_warn was introduced, just not well done.

[byroot]

Fixed in #44484. I gave you co-authorship for bringing up the issue.

Thanks!

[drcapulet]

@byroot Thanks!