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.

Sign up for GitHub

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

Jump to bottom

Fix keyword arguments in dynamic method dispatch #2509

Closed
wants to merge 5 commits into from
Closed

Fix keyword arguments in dynamic method dispatch #2509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ec244b0
Fix keyword arguments in dynamic matchers
JoshCheek Jul 15, 2021
e4b3953
Update controller_example_group_spec.rb
pirj Jul 15, 2021
e2541f0
Update spec/rspec/rails/example/controller_example_group_spec.rb
pirj Jul 27, 2021
cb7e2d4
Update spec/rspec/rails/example/controller_example_group_spec.rb
pirj Jul 27, 2021
0b74226
Revert the previous spec change
pirj Jul 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/rspec/rails/example/controller_example_group.rb
View file
Open in desktop
Expand Up @@ -176,6 +176,7 @@ def method_missing(method, *args, &block)
super
end
end
ruby2_keywords :method_missing if respond_to?(:ruby2_keywords, true)

included do
subject { controller }
Expand Down
7 changes: 7 additions & 0 deletions spec/rspec/rails/example/controller_example_group_spec.rb
View file
Open in desktop
Expand Up @@ -19,6 +19,13 @@ def group_for(klass)
expect(group.included_modules).to include(RSpec::Rails::Matchers::RoutingMatchers)
end

it "handles methods invoked via `method_missing` that use keywords" do
example = group.new
example.define_singleton_method(:has_val?) { |val:| val == 1 }
JonRowe marked this conversation as resolved.
Show resolved Hide resolved
expect(example).to example.have_val(val: 1)
JonRowe marked this conversation as resolved.
Show resolved Hide resolved
expect(example).to_not example.have_val(val: 2)
Comment on lines +24 to +26
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
example.define_singleton_method(:has_val?) { |val:| val == 1 }
expect(example).to example.have_val(val: 1)
expect(example).to_not example.have_val(val: 2)
example.define_singleton_method(:keyword_value) { |value:| value }
expect(example.keyword_value(value: 42)).to eq 42

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer a clear example method here not something imitating a matcher as thats not what is the issue.

Copy link
Member

Choose a reason for hiding this comment

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

This test is not hitting the method_missing. We defined keyword_value and call it.

The current expect(example).to example.have_val(val: 1) works, but it's incredibly confusing.

expect { example.have_val(val: 1) }.not_to raise_error

failed to catch the regression, it doesn't fail if ruby2_keywords is removed.

I'll figure out a self-explanatory spec to cover this. I intend to rely on dynamic matchers, but will try to avoid the expect(example).to part.

Copy link
Member

Choose a reason for hiding this comment

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

I just don't want a have_ anything style example, its confusing, its the method dispatch thats the bug here, not matchers.

Copy link
Member

Choose a reason for hiding this comment

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

It's a bit of both.

To reproduce the issue:

  • it is not sufficient for us to just call the same method that we define, as method_missing won't be called
  • there should be some code that handles method_missing below ControllerExampleGroup and below Rails
  • this code should accept arguments and pass it to a method that accepts keyword arguments

We could write a spec with something like prepend Module.new { def method_missing ... }, but this might be excessively complicated.

There are dynamic matchers. They do exactly what we want - they have a method_missing, and they can delegate to the method we define.

It's still a brain-teaser how to express the case in a spec. The best I could think of so far:

expect {
  example.have_val(val: 1).matches?(example)
}.not_to raise_error

example.have_val goes through ControllerExampleGroup's method_missing, then through Rails' one, and lands in dynamic matchers. They save the kwarg to @args, and the flag that it's a kwarg is already set on the object.
matches? calls has_val? on the example, and passes *@args to it.

It's not necessary to call have_val on example, and we could:

expect(example).to have_val(val: 1)

but in this case ControllerExampleGroup's method_missing is never called, and it's dynamic matchers' method_missing who sets the flag. I.e. this fix would not be covered by such a test.

Frankly speaking, it seems like it doesn't matter much what exactly the spec will be, we'll have to add a chunk of code comments along. And I have strong doubts that in a month from now it will take me less than half an hour to recall all the details about this tangled case.

I'm out of good ideas.

Copy link
Member

Choose a reason for hiding this comment

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

If my change doesn't trigger method missing neither does the original, they both call the method directly I just skip the expect to. So my original point stands here, please construct an example that doesn't look like a matcher.

Copy link
Member

Choose a reason for hiding this comment

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

@JonRowe There is a significant difference. You call a method that we've just defined, keyword_value, while the original example calls a non-existent method, have_val, while we define has_val?.

Copy link
Member

Choose a reason for hiding this comment

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

Ah tired eyes etc, ok fine, implement a method missing and remove the has / have names. This is a lot of conversation for a simple fix, but it needs to not be confused with matchers. Its method_missing and the method names that are causing this.

Copy link
Member

Choose a reason for hiding this comment

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

Right. Dynamic matchers are a victim here.
And they are the only known victim. And they apparently are the only way to test the fix without involving in_sub_process { ControllerExampleGroup.prepend Module.new { def method_missing ... end } }.

end

context "with implicit subject" do
it "uses the controller as the subject" do
controller = double('controller')
Expand Down