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

If a callable mock includes kwargs, Ruby 2.7 warns #824

Closed
wants to merge 1 commit into from
Closed

If a callable mock includes kwargs, Ruby 2.7 warns #824

wants to merge 1 commit into from

Conversation

kddnewton
Copy link

@kddnewton kddnewton commented Dec 31, 2019

This commit explicitly forwards kwargs to remove the warning, and adds a test to assert that it was silent.

@zenspider zenspider changed the title If a callable mock includes kwargs, Ruby warns If a callable mock includes kwargs, Ruby 2.7 warns Jan 9, 2020
@zenspider zenspider self-assigned this Jan 9, 2020
@zenspider
Copy link
Collaborator

This patch wrecks havoc on the tests if ruby < 2.7:

680 % STFU p4 gh https://github.com/seattlerb/minitest/pull/824
681 % rake26
Run options: --seed 24769

# Running:

.............................................................................................................................................SEE....S.E.SS..S..S.E.E.SSS..S.E.................................................................................................................................................................................................................

Finished in 0.133836s, 2854.2395 runs/s, 8144.2960 assertions/s.

  1) Error:
TestMinitestStub#test_stub_lambda_block_5:
ArgumentError: wrong number of arguments (given 1, expected 0)
    /Users/ryan/Work/p4/zss/src/minitest/dev/test/minitest/test_minitest_mock.rb:631:in `block in test_stub_lambda_block_5'

  2) Error:
TestMinitestStub#test_stub_lambda:
ArgumentError: wrong number of arguments (given 1, expected 0)
    /Users/ryan/Work/p4/zss/src/minitest/dev/test/minitest/test_minitest_mock.rb:619:in `block in test_stub_lambda'

  3) Error:
TestMinitestStub#test_stub_block_args:
ArgumentError: wrong number of arguments (given 2, expected 1)
    /Users/ryan/Work/p4/zss/src/minitest/dev/test/minitest/test_minitest_mock.rb:443:in `block in test_stub_block_args'

  4) Error:
TestMinitestStub#test_stub_lambda_args:
ArgumentError: wrong number of arguments (given 1, expected 0)
    /Users/ryan/Work/p4/zss/src/minitest/dev/test/minitest/test_minitest_mock.rb:625:in `block in test_stub_lambda_args'

  5) Error:
TestMinitestStub#test_stub_block:
ArgumentError: wrong number of arguments (given 1, expected 0)
    /Users/ryan/Work/p4/zss/src/minitest/dev/test/minitest/test_minitest_mock.rb:435:in `block in test_stub_block'

  6) Error:
TestMinitestStub#test_stub_lambda_block_args_5:
ArgumentError: wrong number of arguments (given 2, expected 1)
    /Users/ryan/Work/p4/zss/src/minitest/dev/test/minitest/test_minitest_mock.rb:652:in `block in test_stub_lambda_block_args_5'

382 runs, 1090 assertions, 0 failures, 6 errors, 10 skips

You have skipped tests. Run with --verbose for details.
rake aborted!
Command failed with status (1): [/Users/ryan/.rubies/ruby-2.6.5/bin/ruby -I...]

Tasks: TOP => default => test
(See full trace by running task with --trace)

@zenspider
Copy link
Collaborator

and presumably you'd want **kwargs on the other side of that else?

@kddnewton
Copy link
Author

Ahh sorry I didn't realize that would break things from earlier Ruby versions. I've added a switch on RUBY_VERSION to fix that.

I'm not sure what you mean about **kwargs being on the wrong side of the else though, can you clarify?

@kddnewton
Copy link
Author

@zenspider anything I can do to help get this in?

@jeremyevans
Copy link

The implementation is wrong, it should use ruby2_keywords if defined: ruby2_keywords(name) if defined?(:ruby2_keywords, true), and you shouldn't need other non-test changes. This type of issue is the reason ruby2_keywords was added to Ruby.

@kddnewton
Copy link
Author

kddnewton commented Aug 31, 2020

@jeremyevans we're modifying the way in which we call the callable proc here, not the method that surrounds it. I don't think ruby2_keywords(method) will help us here.

That being said if you can find a way to make it work with ruby2_keywords I'd love to see it!

@jeremyevans
Copy link

The arguments passed to the proc come from *args in the method definition, and using ruby2_keywords on the method will correctly flag the keyword args hash in *args, so that when calling the proc, the keywords args hash will be converted back to keywords. Have you actually tried using ruby2_keywords here, or are you just guessing?

@kddnewton
Copy link
Author

Wow dude. Yes I have tried multiple times to get this to work with ruby2_keywords and have so far been unable to. I would love to learn from you if you could put a code snippet on this PR which makes it work.

@jeremyevans
Copy link

Apologies. I'm not trying to attack you, I just did not see anywhere earlier in this where it states that ruby2_keywords was tried. I can certainly work on an example.

@jeremyevans
Copy link

This appears to work with your test:

diff --git a/lib/minitest/mock.rb b/lib/minitest/mock.rb
index 39cfc24..6fc97e3 100644
--- a/lib/minitest/mock.rb
+++ b/lib/minitest/mock.rb
@@ -230,6 +230,7 @@ class Object
         val_or_callable
       end
     end
+    metaclass.send(:ruby2_keywords, name) if metaclass.respond_to?(:ruby2_keywords, true)

     yield self
   ensure

This commit handles kwargs using ruby2_keywords, and adds a test to assert that the warning is gone.
@kddnewton
Copy link
Author

@jeremyevans thank you for the help. I've updated the pull request per your suggestion.

@zenspider
Copy link
Collaborator

Thanks everyone. I’m currently celebrating an anniversary but I’ll try to get on this shortly. I have a pressing hoe release that’ll be a forcing function on this as well.

@Nakilon
Copy link

Nakilon commented Feb 2, 2021

May be related: Nakilon@3fc22e7#diff-2a72d0121a0eb22e9f32a1ce654e5132391273007bf643b5ae68fbbc2800dea4
Mocking the method with both args and kwargs mixed them both into args on Ruby 3.0

@TSMMark
Copy link

TSMMark commented Feb 10, 2021

May be related: Nakilon@3fc22e7#diff-2a72d0121a0eb22e9f32a1ce654e5132391273007bf643b5ae68fbbc2800dea4
Mocking the method with both args and kwargs mixed them both into args on Ruby 3.0

I agree, this is what we need to get merged here and released in a new gem version.

I imagine this is currently a problem for anyone stubbing methods with kwargs.

@zenspider
Copy link
Collaborator

GAH... I totally forgot about this PR and basically did the same thing myself yesterday... I had clicked through on the email from @Nakilon's comment and ignored the wider context. Sorry! I'll add you to the changelog.

@zenspider zenspider closed this Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants