subject { } is broken in rspec 2.13.x when before(:all) references a `let` declaration #802

Closed
postmodern opened this Issue Feb 26, 2013 · 7 comments

Comments

Projects
None yet
4 participants
Contributor

postmodern commented Feb 26, 2013

Ran some specs that make heavy use of subject { } against rspec 2.13.x. It appears to be ignoring the subject { } blocks, and using described_class instead.


  16) Ronin::URL from should parse URL schemes
     Failure/Error: subject.scheme.should_not be_nil
     NoMethodError:
       undefined property or relationship 'nil?' on Ronin::URLScheme
     # ./spec/url_spec.rb:95:in `block (3 levels) in <top (required)>'

  17) Ronin::URL from should parse host names
     Failure/Error: subject.host_name.address.should == host_name
       expected: "www.example.com"
            got: #<DataMapper::Query::Path:0x000000028e5f60 @relationships=[#<DataMapper::Associations::ManyToOne::Relationship:0x00000001efea38 @required=true, @key=false, @unique=false, @child_model=Ronin::URL, @child_model_name="Ronin::URL", @parent_model=Ronin::HostName, @parent_model_name="HostName", @name=:host_name, @instance_variable_name="@host_name", @options={:min=>1, :max=>1, :child_repository_name=>:default, :parent_repository_name=>nil}, @child_repository_name=:default, @parent_repository_name=nil, @min=1, @max=1, @reader_visibility=:public, @writer_visibility=:public, @default=nil, @query={}, @parent_key=[#<DataMapper::Property::Serial @model=Ronin::Address @name=:id>], @child_key=[#<DataMapper::Property::Integer @model=Ronin::URL @name=:host_name_id>], @inverse=#<DataMapper::Associations::OneToMany::Relationship:0x00000001ef1950 @child_model=Ronin::URL, @child_model_name="URL", @parent_model=Ronin::HostName, @parent_model_name="Ronin::HostName", @name=:urls, @instance_variable_name="@urls", @options={:min=>0, :max=>Infinity, :child_repository_name=>nil, :parent_repository_name=>:default}, @child_repository_name=nil, @parent_repository_name=:default, @min=0, @max=Infinity, @reader_visibility=:public, @writer_visibility=:public, @default=nil, @query={}, @inverse=#<DataMapper::Associations::ManyToOne::Relationship:0x00000001efea38 ...>, @parent_key=[#<DataMapper::Property::Serial @model=Ronin::Address @name=:id>]>>], @repository_name=:default, @model=Ronin::HostName, @property=#<DataMapper::Property::String @model=Ronin::HostName @name=:address>> (using ==)
       Diff:
       @@ -1,2 +1,2 @@
       -"www.example.com"
       +#<DataMapper::Property::String @model=Ronin::HostName @name=:address>

     # ./spec/url_spec.rb:100:in `block (3 levels) in <top (required)>'

  18) Ronin::URL from should parse port numbers
     Failure/Error: subject.port.number.should == port
       expected: 8080
            got: #<DataMapper::Query::Path:0x000000028f10e0 @relationships=[#<DataMapper::Associations::ManyToOne::Relationship:0x00000001f08588 @required=false, @key=false, @unique=false, @child_model=Ronin::URL, @child_model_name="Ronin::URL", @parent_model=Ronin::TCPPort, @parent_model_name="TCPPort", @name=:port, @instance_variable_name="@port", @options={:min=>0, :max=>1, :required=>false, :child_repository_name=>:default, :parent_repository_name=>nil}, @child_repository_name=:default, @parent_repository_name=nil, @min=0, @max=1, @reader_visibility=:public, @writer_visibility=:public, @default=nil, @query={}, @parent_key=[#<DataMapper::Property::Serial @model=Ronin::Port @name=:id>], @child_key=[#<DataMapper::Property::Integer @model=Ronin::URL @name=:port_id>], @inverse=#<DataMapper::Associations::OneToMany::Relationship:0x0000000205a378 @child_model=Ronin::URL, @child_model_name="URL", @parent_model=Ronin::TCPPort, @parent_model_name="Ronin::TCPPort", @name=:urls, @instance_variable_name="@urls", @options={:min=>0, :max=>Infinity, :child_key=>[:port_id], :child_repository_name=>nil, :parent_repository_name=>:default}, @child_repository_name=nil, @parent_repository_name=:default, @min=0, @max=Infinity, @reader_visibility=:public, @writer_visibility=:public, @default=nil, @query={}, @child_properties=[:port_id], @inverse=#<DataMapper::Associations::ManyToOne::Relationship:0x00000001f08588 ...>, @parent_key=[#<DataMapper::Property::Serial @model=Ronin::Port @name=:id>]>>], @repository_name=:default, @model=Ronin::TCPPort, @property=#<DataMapper::Property::Integer @model=Ronin::Port @name=:number>> (using ==)
     # ./spec/url_spec.rb:104:in `block (3 levels) in <top (required)>'

  19) Ronin::URL from should parse paths
     Failure/Error: subject.path.should == path
       expected: "/path"
            got: #<DataMapper::Property::String @model=Ronin::URL @name=:path> (using ==)
       Diff:
       @@ -1,2 +1,2 @@
       -"/path"
       +#<DataMapper::Property::String @model=Ronin::URL @name=:path>

     # ./spec/url_spec.rb:108:in `block (3 levels) in <top (required)>'

  20) Ronin::URL from should parse query strings
     Failure/Error: subject.query_string.should == query_string
     NoMethodError:
       undefined method `query_string' for Ronin::URL:Class
     # ./spec/url_spec.rb:112:in `block (3 levels) in <top (required)>'

  21) Ronin::URL from should parse URL fragments
     Failure/Error: subject.fragment.should == fragment
       expected: "frag"
            got: #<DataMapper::Property::String @model=Ronin::URL @name=:fragment> (using ==)
       Diff:
       @@ -1,2 +1,2 @@
       -"frag"
       +#<DataMapper::Property::String @model=Ronin::URL @name=:fragment>

     # ./spec/url_spec.rb:116:in `block (3 levels) in <top (required)>'

  22) Ronin::URL#to_uri should convert the scheme
     Failure/Error: subject.scheme.should == scheme
     NoMethodError:
       undefined property or relationship 'method' on Ronin::URLScheme
     # ./spec/url_spec.rb:131:in `block (3 levels) in <top (required)>'

  23) Ronin::URL#to_uri should convert the host name
     Failure/Error: subject.host.should == host_name
     NoMethodError:
       undefined method `host' for Ronin::URL:Class
     # ./spec/url_spec.rb:135:in `block (3 levels) in <top (required)>'

  24) Ronin::URL#to_uri should convert the port number
     Failure/Error: subject.port.should == port
     NoMethodError:
       undefined property or relationship 'method' on Ronin::TCPPort
     # ./spec/url_spec.rb:139:in `block (3 levels) in <top (required)>'

  25) Ronin::URL#to_uri should convert the path
     Failure/Error: subject.path.should == path
       expected: "/path"
            got: #<DataMapper::Property::String @model=Ronin::URL @name=:path> (using ==)
       Diff:
       @@ -1,2 +1,2 @@
       -"/path"
       +#<DataMapper::Property::String @model=Ronin::URL @name=:path>

     # ./spec/url_spec.rb:143:in `block (3 levels) in <top (required)>'

  26) Ronin::URL#to_uri should convert the query string
     Failure/Error: subject.query.should == query_string
       expected: "q=1"
            got: #<DataMapper::Query @repository=:default @model=Ronin::URL @fields=[#<DataMapper::Property::Serial @model=Ronin::URL @name=:id>, #<DataMapper::Property::String @model=Ronin::URL @name=:path>, #<DataMapper::Property::String @model=Ronin::URL @name=:fragment>, #<DataMapper::Property::Time @model=Ronin::URL @name=:last_scanned_at>, #<DataMapper::Property::DateTime @model=Ronin::URL @name=:created_at>, #<DataMapper::Property::Integer @model=Ronin::URL @name=:scheme_id>, #<DataMapper::Property::Integer @model=Ronin::URL @name=:host_name_id>, #<DataMapper::Property::Integer @model=Ronin::URL @name=:port_id>] @links=[] @conditions=nil @order=[#<DataMapper::Query::Direction @target=#<DataMapper::Property::Serial @model=Ronin::URL @name=:id> @operator=:asc>] @limit=nil @offset=0 @reload=false @unique=false> (using ==)
       Diff:
       @@ -1,2 +1,2 @@
       -"q=1"
       +#<DataMapper::Query @repository=:default @model=Ronin::URL @fields=[#<DataMapper::Property::Serial @model=Ronin::URL @name=:id>, #<DataMapper::Property::String @model=Ronin::URL @name=:path>, #<DataMapper::Property::String @model=Ronin::URL @name=:fragment>, #<DataMapper::Property::Time @model=Ronin::URL @name=:last_scanned_at>, #<DataMapper::Property::DateTime @model=Ronin::URL @name=:created_at>, #<DataMapper::Property::Integer @model=Ronin::URL @name=:scheme_id>, #<DataMapper::Property::Integer @model=Ronin::URL @name=:host_name_id>, #<DataMapper::Property::Integer @model=Ronin::URL @name=:port_id>] @links=[] @conditions=nil @order=[#<DataMapper::Query::Direction @target=#<DataMapper::Property::Serial @model=Ronin::URL @name=:id> @operator=:asc>] @limit=nil @offset=0 @reload=false @unique=false>

     # ./spec/url_spec.rb:147:in `block (3 levels) in <top (required)>'

  27) Ronin::URL#to_uri should convert the fragment
     Failure/Error: subject.fragment.should == fragment
       expected: "frag"
            got: #<DataMapper::Property::String @model=Ronin::URL @name=:fragment> (using ==)
       Diff:
       @@ -1,2 +1,2 @@
       -"frag"
       +#<DataMapper::Property::String @model=Ronin::URL @name=:fragment>

     # ./spec/url_spec.rb:158:in `block (3 levels) in <top (required)>'

  28) Ronin::URL#to_s should convert the URL back into a String URI
     Failure/Error: subject.should == uri.to_s
       expected: "https://www.example.com:8080/path?q=1#frag"
            got: Ronin::URL (using ==)
       Diff:
       @@ -1,2 +1,2 @@
       -"https://www.example.com:8080/path?q=1#frag"
       +Ronin::URL

     # ./spec/url_spec.rb:166:in `block (3 levels) in <top (required)>'

  29) Ronin::URL#inspect should include the full URL
     Failure/Error: subject.should include(uri.to_s)
       expected Ronin::URL to include "https://www.example.com:8080/path?q=1#frag"
       Diff:
       @@ -1,2 +1,2 @@
       -["https://www.example.com:8080/path?q=1#frag"]
       +Ronin::URL

     # ./spec/url_spec.rb:174:in `block (3 levels) in <top (required)>'
Contributor

postmodern commented Feb 26, 2013

Also happens in spec/worldlist_spec.rb.

  1) Ronin::Wordlist#each_word with wordlist file should enumerate over the words
     Failure/Error: subject.each_word.to_a.should == words
     NoMethodError:
       undefined method `each_word' for Ronin::Wordlist:Class
     # ./spec/wordlist_spec.rb:97:in `block (4 levels) in '
  2) Ronin::Wordlist#each_word with words should enumerate over the words
     Failure/Error: subject.each_word.to_a.should == words
     NoMethodError:
       undefined method `each_word' for Ronin::Wordlist:Class
     # ./spec/wordlist_spec.rb:105:in `block (4 levels) in '
  3) Ronin::Wordlist#each should rewind file lists
     Failure/Error: subject.each { |word| }
     NoMethodError:
       undefined method `each' for Ronin::Wordlist:Class
     # ./spec/wordlist_spec.rb:112:in `block (3 levels) in '
  4) Ronin::Wordlist#each_n_words should enumerate over every combination of N words
     Failure/Error: subject.each_n_words(2).to_a.should == %w[
     NoMethodError:
       undefined method `each_n_words' for Ronin::Wordlist:Class
     # ./spec/wordlist_spec.rb:127:in `block (3 levels) in '
  5) Ronin::Wordlist#save should save the words with mutations to a file
     Failure/Error: subject.save(saved_path)
     NoMethodError:
       undefined method `save' for Ronin::Wordlist:Class
     # ./spec/wordlist_spec.rb:139:in `block (3 levels) in '

The describe blocks appear to not be inheriting the top-level subject { }.

Owner

myronmarston commented Feb 26, 2013

Thanks for reporting this. I'll take a look soon.

Owner

myronmarston commented Feb 26, 2013

After digging into this, I've discovered that it's due to interplay between let/subject memoization and before(:all). Here's a simpler example that fails:

describe Array do
  before(:all) { subject }

  describe "Class Methods" do
    subject { described_class }
    example { expect(subject).to be(Array) }
  end

  describe "Instance Methods" do
    subject { described_class.new([1, 2]) }
    example { expect(subject).to be_an(Array) }
  end
end

Output:

Array
  Class Methods
    should equal Array (FAILED - 1)
  Instance Methods
    should be a kind of Array

Failures:

  1) Array Class Methods 
     Failure/Error: example { expect(subject).to be(Array) }

       expected #<Class:70325266257580> => Array
            got #<Array:70325274767720> => []

       Compared using equal?, which compares object identity,
       but expected and actual are not the same object. Use
       `expect(actual).to eq(expected)` if you don't care about
       object identity in this example.


       Diff:
       @@ -1,2 +1,2 @@
       -Array
       +[]

If you comment out the before(:all), this passes. Here's another example that uses let instead of subject and similarly fails, but passes if before(:all) is removed:

describe "Let memoization" do
  before(:all) { foo }

  let(:foo) { 5 } 

  describe "Class Methods" do
    let(:bar) { 3 } 
    example { expect(bar).to eq(3) }
  end 

  describe "Instance Methods" do
    let(:bar) { 4 } 
    example { expect(bar).to eq(4) }
  end 
end

Output:

Let memoization
  group 1
    should eq 3
  group 2
    should eq 4 (FAILED - 1)

Failures:

  1) Let memoization group 2 
     Failure/Error: example { expect(bar).to eq(4) }

       expected: 4
            got: 3

       (compared using ==)

The source of the problem is that instance variables that are set by code running in before(:all) get stored and re-assigned for each example, and that includes the @__memoized variable that is used by let declarations. As of 19dd6b2 subject is implemented in terms of let, so that if @__memoized gets stored in before(:all), these weird bugs occur.

I tried the second example on rspec 2.12 and 2.11, and it fails on those versions as well, so this has been a long-standing bug in rspec. Actually, I don't think the lifecycle of let-memoized objects when referenced in before(:all) has ever been specified. We should decide what the behavior should be.

A couple ideas:

  1. Disallow let methods from being called in before(:all).
  2. Clear let memoization before every example -- which would imply that the value memoized in before(:all) will not be preserved for any of the examples.
  3. Allow let memoizations to survive for the duration of whatever scope they are first referenced in--so if a let declaration is first referenced in before(:all) it would survive for all the examples in that group.

I'm leaning towards the second option, and also adding a warning (something like "WARNING: let declaration foo referenced in before(:all). The value will not be memoized since this is outside the scope of one example.")

@dchelimsky / @alindeman -- what are your thoughts here?

Member

soulcutter commented Feb 26, 2013

I don't think a let should be permissible inside a before(:all) . There's some dissonance in allowing it in some before calls and not others - this leads me to think that they might belong as separate methods rather than one method that takes a symbol parameter indicating the type of hook it is. Changing method names would take a whole deprecation/removal cycle, not to mention changing something that has been baked in for so long could grate on people, so I understand if that's not really on the table here. Bottom line, I think the most clear behavior is to not allow it in before(:all)

If you go with 2, I would only say there certainly should be a warning in that case since it can result in surprising behavior. The copy you came up with for that is good.

Contributor

alindeman commented Feb 28, 2013

Disallow let methods from being called in before(:all).

Given that let is normally memoized on a test-by-test basis, referencing it in before(:all) doesn't make sense at all to me.

If we were starting from scratch, I'd go with "Disallow let methods from being called in before(:all)."

Because it has unintentionally(?) worked in previous versions, though, I think we should go with "Clear let memoization before every example -- which would imply that the value memoized in before(:all) will not be preserved for any of the examples." + a warning.

Owner

myronmarston commented Feb 28, 2013

@alindeman -- that's 100% my opinion, too. I'll take a stab at implementing option 2 + a warning.

myronmarston was assigned Mar 1, 2013

Owner

myronmarston commented Mar 1, 2013

BTW, I had forgotten that this had come up so many times before, but we've previously discussed this in #500, #656, #718 and now in this issue. I think the plan we have now (which I've started on, and have a good idea how to implement) is the right approach, and it's in line with our previous discussions.

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