Implicitly assert no exception is raised in block assertions #37313
Conversation
|
The build is broken but it looks unrelated. Can you double check just it is not related? |
|
When looking into this, we found a number of bugs in Shopify's code base and test suites that were hiding because of this issue. I don't think many people know that assertions use exceptions under the hood, and will not assume that the nesting order of |
It seems like it is. It fails consistently on my branch, even when ran in isolation, but not on master. I'm afraid I won't have time to investigate today. |
casperisfine
force-pushed
the
block-assertions
branch
from
a80c50a
to
e951189
Compare
casperisfine
force-pushed
the
block-assertions
branch
from
e951189
to
ea28a1c
Compare
|
Ok, so I updated other block based assertions to also expect no errors. However for the Railie test failing, I'm having a very hard time investigating it. It seems like the parallel worker crash, but I have no idea how to access logs so figure the root cause. |
|
@kaspth seems like you worked on that failing test before. Do you have any pointers as to how I could debug it? Clearly the worker crash, but I have no idea why. Getting a backtrace or something would help. |
| @@ -30,6 +30,8 @@ def assert_not(object, message = nil) | |||
| # end | |||
| def assert_nothing_raised | |||
| yield | |||
| rescue => error | |||
| raise Minitest::UnexpectedError, error | |||
There was a problem hiding this comment.
I actually wonder about the utility of adding this here, it seems minitest does it automatically: https://github.com/seattlerb/minitest/blob/15ed8e4ce504c8313058a1d6fc4918299be34328/lib/minitest/test.rb#L200-L201
There was a problem hiding this comment.
@kaspth that actually the whole point of the PR. If I take the PR description snippet again:
assert_raises RuntimeError do # rescue StandardError and compare with what is expected
assert_no_difference -> { counter.count } do # made a noop by the RuntimeError
counter.increment! # raises RuntimeError (subclass of StandardError)
end
endSo the idea here is to eagerly wrap the error in Minitest::UnexpectedError which doesn't inherit from StandardError so that assert_raises doesn't catch it and hide the legitimate issue.
|
I've tried to debug this some more. I've run the test file locally from which fails with the error seen on CI. I've added these things to get more insight: diff --git a/activesupport/lib/active_support/testing/assertions.rb b/activesupport/lib/active_support/testing/assertions.rb
index e04ff98dc0..d272551e9f 100644
--- a/activesupport/lib/active_support/testing/assertions.rb
+++ b/activesupport/lib/active_support/testing/assertions.rb
@@ -31,6 +31,7 @@ def assert_not(object, message = nil)
def assert_nothing_raised
yield
rescue => error
+ Rails.logger.info error.message
raise Minitest::UnexpectedError, error
end
diff --git a/activesupport/lib/active_support/testing/parallelization.rb b/activesupport/lib/active_support/testing/parallelization.rb
index 96518a4a58..fb65917a06 100644
--- a/activesupport/lib/active_support/testing/parallelization.rb
+++ b/activesupport/lib/active_support/testing/parallelization.rb
@@ -15,6 +15,10 @@ def initialize
end
def record(reporter, result)
+ Rails.logger.info("buf: #{result.buf}")
+ Rails.logger.info("name: #{result.name}")
+ # Rails.logger.info(result.exception)
+
raise DRb::DRbConnError if result.is_a?(DRb::DRbUnknown)
reporter.synchronize do
diff --git a/railties/test/application/test_runner_test.rb b/railties/test/application/test_runner_test.rb
index 9bc7cfbec7..00ab25bffd 100644
--- a/railties/test/application/test_runner_test.rb
+++ b/railties/test/application/test_runner_test.rb
@@ -640,6 +640,8 @@ def test_run_in_parallel_with_unknown_object
RUBY
output = run_test_command("-n test_should_create_user")
+ puts output.split("\n")
+ Dir.chdir(app_path) { require "byebug"; byebug; `open log/test.log` }
assert_match "ActionController::InvalidAuthenticityToken", output
end(somehow the log file wouldn't open without the byebug in there.) What seems to be going is that In my patch I'm outputting the UsersControllerTest: test_should_create_userStarted POST "/users" for 127.0.0.1 at 2019-10-05 18:40:05 +0200 |
|
A potential other fix for the original issue is to override |
|
Hope this helps, I've never really worked with Drb before, so we'd need either Aaron, Eileen or @y-yagi on the case for better assistance. |
|
The test fails because it passes an exception class that can not be resolved by the DRb server(This is exactly what I wanted to solve with #34476). diff --git a/activesupport/lib/active_support/testing/assertions.rb b/activesupport/lib/active_support/testing/assertions.rb
index e04ff98..f283b98 100644
--- a/activesupport/lib/active_support/testing/assertions.rb
+++ b/activesupport/lib/active_support/testing/assertions.rb
@@ -31,7 +31,7 @@ def assert_not(object, message = nil)
def assert_nothing_raised
yield
rescue => error
- raise Minitest::UnexpectedError, error
+ raise Minitest::UnexpectedError.new(error)
endBut as kaspth already mentioned, I think we don't need |
|
I'm learning more and more towards this not being something that we should solve. It looks like minitest ships with two other block based assertions The latter sounds like it would exhibit the same problem. E.g. what happens when execution gets halted deep in the stack? |
There was a problem hiding this comment.
It looks like minitest ships with two other block based assertions
Indeed, but I have very little hope to get this accepted in Minitest. They're not very receptive to changes aimed at preventing mistakes.
IMHO Rails could make this change, and then I could submit a PR on Minitest, just in case.
casperisfine
force-pushed
the
block-assertions
branch
from
ea28a1c
to
a707072
Compare
|
@y-yagi your diff fixed the CI indeed. I'm not quite sure I understand how, but... |
|
I think this is a problem we should fix here and of course try to get the PR submitted to minitest. |
|
Cool cool, I'm happy to go with this, as long as we do make an attempt at upstreaming it, which sounds like we are so all good 😊✌️ |
|
Grain of salt warning, since I haven't checked this, but I'll just reiterate that I think |
|
As a result of this change, the following no longer works in rails 6.1: assert_raises SomeError do
perform_enqueued_jobs
endBecause it is now always throwing This looks like a bug to me. |
|
@artemave not sure what you mean, do you have a more fully featured example? Are you trying to assert than one of the enqueued job is raising an error when performed? |
Yes. |
|
Wouldn't it make more sense to assert this by calling perform directly? |
|
Sometimes code under test enqueues jobs. |
|
@casperisfine here's another example. given this test: assert_raises(ArgumentError) do
email = MyMailer.send_email(foo)
assert_emails(0) { email.deliver_now }
endThen in the mailer we raise an class MyMailer < ApplicationMailer
def send_email(foo)
raise(ArgumentError, "wrong inputs")
end
endOn Rails 6, the test passes. On 6.1 it fails, but it still prints out the
Is there a better way to write this test? |
|
Executable test case for ^ # frozen_string_literal: true
require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
gem "rails", github: "rails/rails", branch: "main"
end
require "action_controller"
require "active_job"
require "active_job/railtie"
require "action_mailer"
require "action_mailer/railtie"
require "rails/test_help"
require "minitest/autorun"
class TestApp < Rails::Application
config.root = __dir__
routes.append {}
end
TestApp.initialize!
class MyMailer < ActionMailer::Base
def send_email(foo)
raise ArgumentError, "bad email!"
end
end
class BuggyJobTest < ActionDispatch::IntegrationTest
def test_that_fails
assert_raises(ArgumentError) do
email = MyMailer.send_email("foo")
assert_emails(0) { email.deliver_now }
end
end
def test_that_passes
assert_raises(Minitest::UnexpectedError) do
email = MyMailer.send_email("foo")
assert_emails(0) { email.deliver_now }
end
end
endI would expect the first test to pass, and the second test to fail. Instead it's the other way around: |
Swapping the assertions around gets the test passing again: assert_emails(0) do
assert_raise(ArgumentError) { email.deliver_now }
endI think it's pretty confusing / hard to debug though. |
|
Moved the discussion to another issue: #42459 (comment) |
As explained on your draft PR, that I'm open to improve error message or things like that, but not to revert this behavior. |
…aised Follow up to rails#37313 - Adds regression tests - Logs a warning in cases where assertions are nested in a way that's likely to be confusing
| @@ -30,6 +30,8 @@ def assert_not(object, message = nil) | |||
| # end | |||
| def assert_nothing_raised | |||
| yield | |||
| rescue => error | |||
| raise Minitest::UnexpectedError.new(error) | |||
There was a problem hiding this comment.
This made minitest a surprised dependency for 6.1
There was a problem hiding this comment.
Oh good pickup. I will have a go at fixing next week unless someone beats me to it.
There was a problem hiding this comment.
Oh wait I thought this was #42459
That said isn’t minitest already a dependency?
There was a problem hiding this comment.
Yes it is:
rails/activesupport/activesupport.gemspec
Line 40 in 9c77d72
So not sure what @mathieujobin means exactly.
Fix: rails#44397 Ref: rails#37313 Ref: rails#42459 This avoid mistakes such as: ```ruby assert_raise Something do assert_queries(1) do raise Something end end ``` Co-Authored-By: Alex Coomans <alexc@squareup.com>
Fix: rails/rails#44397 Ref: rails/rails#37313 Ref: rails/rails#42459 This avoid mistakes such as: ```ruby assert_raise Something do assert_queries(1) do raise Something end end ``` Co-Authored-By: Alex Coomans <alexc@squareup.com>
Ref: Shopify/statsd-instrument#184 (comment)
This simply a quick proof of concept to showcase a fairly nasty foot gun with some block based assertions. See this comment chain for the initial discussion, but I'll explain it all here.
The problem
See the following test case:
It passes even though
counter.countchanged from0to1, this is becauseassert_differencelike most other block based assertions (e.g.assert_enqueued_*) are totally bypassed when an error happen.Of course the test is arguably broken, the user here should put the
assert_raisesinside theassert_no_difference.But realistically it's not hard to overlook this, so I think it would make sense to consider any raised exception, as an assertion failure.
@rafaelfranca @wvanbergen thoughts?