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

[postmodern]

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.

• ronin/url.rb
• spec/url_spec.rb

---

  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)>'

[postmodern]

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 { }.

[myronmarston]

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

[myronmarston]

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 19dd6b2d4b25da0042f80bb8ff854672b913ba30 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?

[soulcutter]

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.

[alindeman]

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.

[myronmarston]

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

[myronmarston]

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.