thoughtbot
lib/context: fix Ruby 2.7 warning
This fixes the warning for:
/vendor/gems/ruby/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/context.rb:209: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
Hey, thanks for submitting this. What is the warning you're getting? Additionally, I think the reason your test passes now is that a warning isn't the same as an error; if you really wanted to test for that I think you'd have to use something like assert_output or assert_silent.
Hi @mcmire
I wrote it in the PR description but seems like the line wasn't wrapped. This is what I see:
/vendor/gems/ruby/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/context.rb:209:
warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
I think the reason your test passes now is that a warning isn't the same as an error; if you really wanted to test for that I think you'd have to use something like assert_output or assert_silent.
Could you point me to an existing test that tests a method_missing method? I still don't know how to test it. How can I make use of assert_output for example? Any help would be useful.
@fatih We don't currently use assert_output, but here is the method in the Minitest source code: https://github.com/seattlerb/minitest/blob/master/lib/minitest/assertions.rb#L317-L348. It looks like you can use it as follows:
assert_output("", /Some warning goes here/) do
# ... do something that would generate a warning ...
end
As for this warning itself, what are you doing that generates this warning? Are you using context as you normally would or are you doing anything special? For instance, are you trying to use it and pass options or something? You may wish to have the test reflect your actual usage rather than call a random method on the context itself.
There is a method like this
def self.it_behaves_like_timed_route(method: :get, status: 200, path: , &block)
should 'time the route execution' do
response = instance_exec(&block)
expected_tags = [
"method:#{method}",
"route:#{path}",
"status:#{status}",
"status_range:#{status.to_s.first}xx"
]
assert_equal 1, timings('request.time', tags: expected_tags).count
end
end
And then call it like this:
class FooControllerTest < ActiveSupport::TestCase
context 'POST /foo/:id/bar' do
it_behaves_like_timed_route(method: :post, status: 401, path: '/foo/:id/bar') do
post FOO.app_path('/foo/1/bar')
end
end
end
This gives then an error saying
/vendor/gems/ruby/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/context.rb:209:
warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
but when track it down it points to this line:
it_behaves_like_timed_route(method: :post, status: 401, path: '/foo/:id/bar') do
Not sure what's going on there at this point. Is it because I created a method and use that?
This is the complete log:
/vendor/gems/ruby/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/context.rb:209:
warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/vendor/gems/ruby/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/context.rb:209:in `method_missing'
/test/integration/controllers/mytest.rb:2:in `block in <class:FooControllerTest>'
/vendor/gems/ruby/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/context.rb:40:in `instance_exec'
/vendor/gems/ruby/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/context.rb:40:in `merge_block'
/vendor/gems/ruby/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/context.rb:31:in `initialize'
/vendor/gems/ruby/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/dsl.rb:192:in `new'
/vendor/gems/ruby/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/dsl.rb:192:in `context'
@fatih Okay. Is it_behaves_like_timed_route a class method on your FooControllerTest or getting mixed in? If that's the case, then calling it_behaves_like_timed_route inside of your context is what is triggering the method_missing. And thinking about it further, I guess that method doesn't actually need to exist, so what you have written in your test is fine to replicate what is going on here.
@mcmire it's a method of this Class:
class ActiveSupport::TestCase
def self.it_behaves_like_timed_route(method: :get, status: 200, path:, &block)
end
...
end
And then inside FooControllerTest I can start using it. (p.s: I'm totally new to Ruby, so please let me know if you need specific information happy to provide them).
We had to make a similar change in factory_bot (see https://github.com/thoughtbot/factory_bot/pull/1415 and then https://github.com/thoughtbot/factory_bot/pull/1425). While reviewing that change I found this post on argument delegation in Ruby2.7/3 useful.
I think this change should be good to go once we are happy with the test. We didn't use assert_output in factory_bot. We had a test that output the deprecation warning before the change, then stopped outputting the deprecation warning after the change. I was OK with that, since I am pretty vigilant about having nothing besides green dots in the test output.
@fatih
@emilford and I worked on writing a test for this, and we figured out why we weren't seeing the deprecation warnings. It has to do with the WarningsLogger in the test helper. It captures stderr, and is meant to fail with a non-zero exit code if anything is output to stderr. Unfortunately the warnings_logger gem doesn't seem to be working quite as expected (it seems to be writing to /tmp rather than a tmp directory within the shoulda-context project, and it is hitting the at_exit hook at the beginning of the suite before any tests run, but doesn't appear to be triggered at any point after the tests have run).
We temporarily disabled the warnings logger and realized that the original test case wasn't outputting any deprecation warnings because the hash argument was coming in as the first argument to `this_is_missing, rather than as a kwarg.
This is a test that triggers the method_missing warning. We also added a positive assertion to replace assert_nothing_raised, since there may be limited value in asserting that no errors were raised.
def test_should_pass_on_missing_method
delegated_to_missing = false
self.class.define_singleton_method(:this_is_missing) do |bar:|
delegated_to_missing = true
end
context = Shoulda::Context::Context.new("context name", self.class) {}
context.this_is_missing(bar: 42)
assert delegated_to_missing
self.class.singleton_class.undef_method(:this_is_missing)
end
We could also add assert_output(nil, ""), but don't feel that is necessary because once the warnings_logger is working correctly it will have us covered.
cc @mcmire
@composerinteralia Ah, okay, I see what you're talking about with warnings_logger. I originally extracted that gem for shoulda-matchers under RSpec and it seems I didn't properly test it for Minitest (it appears that Minitest hijacks at_exit and exits explicitly so the at_exit in warnings_logger is never called). I'll work on a quick fix for this.
Good find! I didn't think to check Minitest for calls to at_exit.
@composerinteralia @fatih I just fixed warnings_logger so that it writes to tmp in the project root instead of /tmp and also so that that it behaves much better on Minitest, so that if warnings get produced during the test run, the test command will fail with an exit status of 1 (and will thus fail the build on Travis). That should enable you to write a failing test for this.
Is there any plan to merge and release this? Without it I believe shoulda-context is incompatible with Ruby 3.x.
Yikes, sorry for the delay on this PR, it's been a long time.
Right now this PR is blocked because there are code changes, but we don't have any way to confirm that this change really does fix the issue. It looks like @composerinteralia submitted a test that should produce a failure without this fix, so I need to drop that in and see if it really does fail. I can try doing this by end of week.