Do not leak TestUnit specific methods after Rails 4 #1512
Conversation
| methods + [:build_message] | ||
| # Starting on Rails 4, Minitest is the default testing framework so no | ||
| # need to add TestUnit specific methods. | ||
| if ::Rails::VERSION::STRING >= '4.0.0' |
There was a problem hiding this comment.
As a point of style, it's nice to write method behavior that depends on constants by putting the conditional statement outside of the method definition so that it doesn't need to get calculated every time the method is called.
if ::Rails::VERSION::STRING >= '4.0.0'
def assertion_method_names
[…]
end
else
def assertion_method_names
[…]
end
endIn this case, define_assertion_delegators only gets called when the concern is included, which is just twice AFAICT, so not a big deal, but wanted to mention it since it's a common pattern.
There was a problem hiding this comment.
Thanks for mentioning, I've updated the code accordingly
|
928eddb doesn't seem strictly necessary. RSpec 4.0 is removing 1.8.7 support, so adding changes specifically to address 1.8.7 compatibility in tests that were already working in all rubies is likely to get reverted or backed out soon. In other words, that commit is probably creating some additional work in the near-future. |
* TestUnit requires the build_message method to be defined for its
assertions to work properly. After Rails 4, Minitest is used instead
of TestUnit so the method should not be included anymore. This is
problematic because when using RSpec configured with
config.expect_with :test_unit
the TestUnit assertions try to call build_message on the
MinitestAssertionAdapter which will crash.
|
@soulcutter It's not strictly necessary, although it improves the quality of the current tests. I removed it, although I still use the same principle on the commit left. Otherwise the build breaks for Ruby <= 1.8.7. In the end, since the bugfix is really only worth for Rails 4 and above, Ruby 1.8.7 does not matter. You tell me what to do, I'm fine either way. Thanks for reviewing! |
|
@soulcutter any words on the actual bugfix? Does it make sense? |
|
Yeah, this looks good to me. Sorry for letting it dangle here for a bit, thanks for gently reminding me to revisit it. |
…fter_rails_4 Do not leak TestUnit specific methods after Rails 4
TestUnit requires the build_message method to be defined for its assertions to work properly. After Rails 4, Minitest is used instead of TestUnit so the method should not be included anymore. This is problematic because when using RSpec configured with
the TestUnit assertions try to call build_message on the MinitestAssertionAdapter which will crash.