Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix generated docstring for BeAnInstanceOf when Class#inspect changed #184

Merged
merged 5 commits into from

4 participants

@rentalcustard

This fixes the situation illustrated in @ddd's gist https://gist.github.com/13469eb6a970e48ec1dc and reported in #rspec on Freenode, whereby a class defines .inspect with expanded information (e.g. all ActiveRecord model classes) and messes up docstring generation.

BeAnInstanceOf doesn't currently define its own #description method, so the default from BaseMatcher, which calls .inspect, was being called. This PR just patches in a description method to the matcher to ensure that to_s is called instead.

@soulcutter
Collaborator

:+1: Nice and clean.

spec/rspec/matchers/be_instance_of_spec.rb
@@ -25,10 +25,24 @@ module Matchers
matcher.matches?(Numeric)
matcher.description.should == "be an instance of Fixnum"
end
+
+ context "when expected provides an expanded inspect, e.g. AR::Base" do
+ class User
+ def self.inspect
+ "User(id: integer, name: string)"
+ end
+ end
@myronmarston Owner

I don't think we want or need this constant defined outside this one context. Can you use an anonymous class (e.g. Class.new) and/or use stub_const if you want it assigned to a constant? That way we don't "leak" the constant outside of this example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rentalcustard

@myronmarston good point, well made. See latest commit - it seemed easiest to use simple stubbing on an existing class once I explored the options. WDYT?

@myronmarston

That's an improvement. However, I kinda liked your example of a User class as it's more intention revealing of a real situation than Fixnum#inspect is (has anyone ever overriden inspect on Fixnum?)

Thoughts?

@rentalcustard

I tried using Class.new to define a User class with the desired characteristics, but it felt strange to be defining self.to_s (to return the expected value) and self.inspect (to show the incorrect value). I felt like the test was at that point going to be relying on the matcher calling to_s, rather than something else like name.

So then I tried stub_const, but I couldn't use it with an uninitialized constant, so I wasn't sure where to go with that.

Did you have something specific in mind? I'd like to represent the intention accurately, as in the original code, but without leaking a constant.

@myronmarston

So then I tried stub_const, but I couldn't use it with an uninitialized constant, so I wasn't sure where to go with that.

I'm not sure what you mean by this, but I think this should work:

let(:user_klass) do
  Class.new do
    def self.inspect
      "User(id: integer, name: string)"
    end
  end
end

before { stub_const("User", user_klass) }

The name and to_s methods will work automatically due to how ruby classes work when they are first assigned to a constant.

@rentalcustard

@myronmarston you're absolutely right, and having toyed with other approaches (just passing a stub directly to be_instance_of, for example), I think yours is the best. See latest commit. I'm happy to reauthor it since it's your code.

spec/rspec/matchers/be_instance_of_spec.rb
@@ -25,10 +25,28 @@ module Matchers
matcher.matches?(Numeric)
matcher.description.should == "be an instance of Fixnum"
end
+
+ context "when expected provides an expanded inspect, e.g. AR::Base" do
+ let(:user_klass) do
+ Class.new do
+ def self.inspect
+ "User(id: integer, name: string)"
+ end
+ end
+ end
+
+ before { stub_const("User", user_klass) }
+
+ it "provides a description including only the class name" do
+ matcher = be_an_instance_of(User)
+ #it will be namespaced because I defined it inside this spec file
@alindeman Collaborator

Is this still accurate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rentalcustard

God, I'm sloppy on Fridays. Thanks @alindeman.

@myronmarston myronmarston merged commit d5d073e into from
@myronmarston

Thanks @mortice, this looks great. I just merged it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
4 lib/rspec/matchers/built_in/be_instance_of.rb
@@ -5,6 +5,10 @@ class BeAnInstanceOf < BaseMatcher
def match(expected, actual)
actual.instance_of? expected
end
+
+ def description
+ "be an instance of #{expected}"
+ end
end
end
end
View
21 spec/rspec/matchers/be_instance_of_spec.rb
@@ -25,10 +25,27 @@ module Matchers
matcher.matches?(Numeric)
matcher.description.should == "be an instance of Fixnum"
end
+
+ context "when expected provides an expanded inspect, e.g. AR::Base" do
+ let(:user_klass) do
+ Class.new do
+ def self.inspect
+ "User(id: integer, name: string)"
+ end
+ end
+ end
+
+ before { stub_const("User", user_klass) }
+
+ it "provides a description including only the class name" do
+ matcher = be_an_instance_of(User)
+ matcher.description.should == "be an instance of User"
+ end
+ end
end
-
+
describe "actual.should_not #{method}(expected)" do
-
+
it "fails with failure message for should_not if actual is instance of expected class" do
lambda { "foo".should_not send(method, String) }.should fail_with(%Q{expected "foo" not to be an instance of String})
end
Something went wrong with that request. Please try again.