Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/context: fix Ruby 2.7 warning #64

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

lib/context: fix Ruby 2.7 warning #64

wants to merge 2 commits into from

Conversation

fatih
Copy link

@fatih fatih commented Jul 8, 2020

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

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
```
h = { k: 42 }
context.this_is_missing(h)
end
end
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this test, but even if we remove the ruby2_keywords fix, this tests passes. I'm open to suggestion on how to improve it.

@mcmire
Copy link
Collaborator

mcmire commented Jul 11, 2020

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.

@fatih
Copy link
Author

fatih commented Jul 13, 2020

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.

@mcmire
Copy link
Collaborator

mcmire commented Jul 13, 2020

@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.

@fatih
Copy link
Author

fatih commented Jul 13, 2020

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'

@mcmire
Copy link
Collaborator

mcmire commented Jul 14, 2020

@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.

@fatih
Copy link
Author

fatih commented Jul 14, 2020

@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).

@composerinteralia
Copy link

composerinteralia commented Jul 20, 2020

We had to make a similar change in factory_bot (see thoughtbot/factory_bot#1415 and then thoughtbot/factory_bot#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.

@composerinteralia
Copy link

composerinteralia commented Jul 20, 2020

@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

@mcmire
Copy link
Collaborator

mcmire commented Jul 25, 2020

@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.

@composerinteralia
Copy link

Good find! I didn't think to check Minitest for calls to at_exit.

@mcmire
Copy link
Collaborator

mcmire commented Aug 23, 2020

@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.

@corincerami
Copy link

Is there any plan to merge and release this? Without it I believe shoulda-context is incompatible with Ruby 3.x.

@mcmire
Copy link
Collaborator

mcmire commented Feb 21, 2023

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.

@mcmire mcmire self-assigned this Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants