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

Moves KeyError into Shared spec #576

Merged
merged 3 commits into from
Jan 3, 2018
Merged

Conversation

cadwallion
Copy link
Contributor

@cadwallion cadwallion commented Dec 31, 2017

This refactors my work on KeyError (#574) features into a shared spec to reduce the duplication.

There are two oddities that I should point out, both are limitations I've found with the implementation of it_behaves_like:

  1. All it_behaves_like calls must have an identical @method and @object calls within a given scope, or they will affect each others' state. To overcome this, you have to wrap the differing parameter in a context or describe block.
  2. If you have a shared spec that relies upon another shared spec, the above issue gets more complex, because even if you've wrapped it in a describe block, the @method has shared scope. To get around this here, I have saved the outer @method into a new @base_method inside a before and referred to it via @base_method to solve the collision.

I'm not sure if the two above issues are intentional side-effects of mspec's implementation or unintended bugs so I'm not sure if I should file an issue, but I thought I'd bring it up since it was relevant to my work on this.

This refactors my work on KeyError features into a shared spec to reduce the duplication.
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Nice work!

-> {
@method.call(@object, 'foo')
}.should raise_error(KeyError) { |err|
err.receiver.should == @object
Copy link
Member

Choose a reason for hiding this comment

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

This could be should equal(@object) to make sure it's the same object.

it "returns the default value from block" do
@hash.fetch_values(:z) { |key| "`#{key}' is not found" }.should == ["`z' is not found"]
@hash.fetch_values(:a, :z) { |key| "`#{key}' is not found" }.should == [1, "`z' is not found"]
end
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 the group describe "with unmatched keys" do below which should be replace by it_behaves_like.

lambda { {}.fetch(:a) }.should raise_error(KeyError)
lambda { Hash.new(5).fetch(:a) }.should raise_error(KeyError)
lambda { Hash.new { 5 }.fetch(:a) }.should raise_error(KeyError)
end
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to keep these examples which use different kinds of Hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added 3 more it_behaves_like calls so it can test all the shared specs in each of the Hash creation methods.

@eregon
Copy link
Member

eregon commented Jan 2, 2018

I'm not sure if the two above issues are intentional side-effects of mspec's implementation or unintended bugs so I'm not sure if I should file an issue, but I thought I'd bring it up since it was relevant to my work on this.

That seems unintended bugs. Could you file an issue on https://github.com/ruby/mspec ?
The workaround in this PR seems fine in the meanwhile.

@cadwallion
Copy link
Contributor Author

I've updated the code per review comments, and I'll file an issue with ruby/mspec detailing the scoping problems of it_behaves_like!

@eregon eregon merged commit 8c9960a into ruby:master Jan 3, 2018
@eregon
Copy link
Member

eregon commented Jan 3, 2018

Thank you for the refactoring!

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.

None yet

2 participants