Implicit subject should be memoized with let #768

Merged
merged 2 commits into from Jan 5, 2013

2 participants

@exviva

This is a partial solution to #766, the only thing left is the problem with its.

I'm open to feedback and suggestions on improving this solution.

@dchelimsky dchelimsky and 1 other commented on an outdated diff Jan 4, 2013
lib/rspec/core/example_group.rb
@@ -13,6 +13,8 @@ module Core
# which serves as a wrapper for an instance of the ExampleGroup in which it
# is declared.
class ExampleGroup
+ include Let # Subject::ExampleGroupMethods depends on this module, so include it first
+
@dchelimsky
RSpec member
dchelimsky added a line comment Jan 4, 2013

That's fine if there's an ordering dependency, but this can just go right before Subject::ExampleMethods within the group of include statements rather than making it the very first thing and separating it from the other includes.

@exviva
exviva added a line comment Jan 4, 2013

Unfortunately it depends on Subject::ExampleGroupMethods, not Subject::ExampleMethods, which is in the extended modules, before all the includes.

@dchelimsky
RSpec member
dchelimsky added a line comment Jan 4, 2013

Got it - misread the comment :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dchelimsky dchelimsky and 2 others commented on an outdated diff Jan 4, 2013
lib/rspec/core/subject.rb
# @see ExampleMethods#should
def subject(name=nil, &block)
let(:subject, &block)
alias_method name, :subject if name
end
+ def self.extended(base)
+ # This logic defines an implicit subject.
@dchelimsky
RSpec member
dchelimsky added a line comment Jan 4, 2013

How about a method named implicit_subject instead of a comment?

@exviva
exviva added a line comment Jan 4, 2013

👍 I'll wait for you to finish your review and mass-amend the PR.

@dchelimsky
RSpec member
dchelimsky added a line comment Jan 4, 2013

I'm done, but @myronmarston and @alindeman might have different comments.

@myronmarston
RSpec member
myronmarston added a line comment Jan 4, 2013

I actually prefer this to defining a method named implicit_subject. Each method that gets mixed in to example groups is another potential naming collision waiting to happen, since these methods are added to the same namespace where users define methods. Ideally, RSpec will "reserve" as few names as possible.

@dchelimsky
RSpec member
dchelimsky added a line comment Jan 4, 2013

@myronmarston your reasoning makes sense to me.

@exviva
exviva added a line comment Jan 4, 2013

That was my intention as well (in a commit regarding its I removed all other private methods). I just moved the comment from the original implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dchelimsky dchelimsky and 1 other commented on an outdated diff Jan 4, 2013
spec/rspec/core/subject_spec.rb
@@ -338,4 +338,34 @@ def false_if_first_time
end
end
end
+
+ describe 'using subject in before and let blocks' do
+ shared_examples_for 'a subject' do
+ let(:subject_id_in_let) { subject.object_id }
+ subject_id_in_before = nil
@dchelimsky
RSpec member
dchelimsky added a line comment Jan 4, 2013

Having this assignment "unwrapped" feels a little weird to me. The reason is that it will be evaluated when the shared_examples block is evaluated rather than within the scope of the example.

How about using an instance variable in the before hook? e.g.

before { @subject_id_in_before = subject.object_id }

it 'should be memoized' do
  expect(subject_id_in_let).to eq(@subject_id_in_before)
end
@myronmarston
RSpec member
myronmarston added a line comment Jan 4, 2013

This looks a bit like a technique I blogged about :). I'm comfortable with using a pre-declared local here, but others might find it confusing...and the sorts of reasons I would usually favor this technique over ivars (i.e. limiting the number of variables that "leak" into an including context) don't really apply here. I think the instance variable version recommended by @dchelimsky is more easily readable by more people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston myronmarston and 1 other commented on an outdated diff Jan 4, 2013
lib/rspec/core/subject.rb
+ # # ^ ^ explicit reference to subject not recommended
+ # end
+ # end
+ #
+ # # implicit subject => { Person.new }
+ # describe Person do
+ # it "should be eligible to vote" do
+ # subject.should be_eligible_to_vote
+ # # ^ ^ explicit reference to subject not recommended
+ # end
+ # end
+ #
+ # # one-liner syntax - should is invoked on subject
+ # describe Person do
+ # it { should be_eligible_to_vote }
+ # end
@myronmarston
RSpec member
myronmarston added a line comment Jan 4, 2013

I can see why moving the YARD comments are necessary (since we no longer have def subject), but I'm a bit concerned about how this changes the docs. Currently the docs show up here. With this change I assume they'll show up in the overview section of the module? Is there a way with yard to force-add docs for subject even though we don't have def subject?

@exviva
exviva added a line comment Jan 4, 2013

I don't know YARD too well, but I'd doubt it. Maybe this section should be moved to ExampleGroupMethods#subject?

@myronmarston
RSpec member
myronmarston added a line comment Jan 4, 2013

Actually, I think we can leave a normal def subject with these comments in place so the docs make sense. That version of the method will never be called (as we re-define it with your extended hook below) but it will make the docs more clear, I think.

@dchelimsky -- can you think of a better way to handle the doc needs here?

@exviva
exviva added a line comment Jan 4, 2013

@myronmarston adding an empty ExampleMethods#subject is a bit unfortunate - since this module is included after ExampleGroupMethods gets extended, this method becomes the implicit subject (and first super() in the chain). I'd need to re-organise the includes and extends again. Do you think it's worth it?

@myronmarston
RSpec member
myronmarston added a line comment Jan 4, 2013

I think so. Having easily findable docs for what subject does when called from an example seems important.

That said, given these ordering dependencies, it makes me think we should make the module here mange it (e.g. via an included hook), rather than forcing ExampleGroup to include/extend things in a particular order. Also, given the similarity between subject and let, and the fact that one now depends on the other...I wonder if we should combine these into one module in one file? That'd have a nice side benefit of loading one fewer file at runtime when rspec is loading, leading to ever-so-slightly-faster (but barely noticeable if at all) runtimes.

@exviva
exviva added a line comment Jan 4, 2013

Combining these two modules into one seemed to be the most natural for me too. Any suggestions for the name? How about RSpec::Core::Declarative?

This may be getting a bit ahead of myself, but do you have any policy regarding deprecating constants (modules)?

@myronmarston
RSpec member
myronmarston added a line comment Jan 4, 2013

Declarative is a good suggestion. Another idea: RSpec::Core::MemoizedHelpers -- since both let and subject define memoized helper methods.

This may be getting a bit ahead of myself, but do you have any policy regarding deprecating constants (modules)?

We try to follow SemVer but there are gray areas for cases like these. In a case like an error class that I expect someone to explicitly reference (e.g. to rescue that kind of error) I'd definitely want to properly deprecate it using a technique I blogged about. That said, I don't consider the constant names for the Let and Subject modules to be part of the public API; it's simply the internal code organization we've chosen to use within rspec-core. The public API are the let and subject methods made available in example groups for end users, and the modules they come from shouldn't matter. I can't think of a case where a user would be explicitly referencing one of these modules.

@exviva
exviva added a line comment Jan 4, 2013

Another idea: RSpec::Core::MemoizedHelpers -- since both let and subject define memoized helper methods.

The full API this module adds, is:

#subject
#should
#should_not
.let
.let!
.subject
.subject!
.its

So it's not only about memoization...how about ShorthandSyntax? Nah, doesn't sound good, too :).

Ok, I'll call it MemoizedHelpers for now.

I don't consider the constant names for the Let and Subject modules to be part of the public API

Cool :).

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

This is fantastic, @exviva! Are you planning to tackle the its issue as well?

@exviva

@myronmarston I'm struggling with the its issue, but no luck (yet). You can have a look at my efforts. Since its is broken also in v2.12.2, I'd say these are 2 separate issues, so how about we merge this one in first and I'd work on its in parallel?

@myronmarston
RSpec member

@myronmarston I'm struggling with the its issue, but no luck (yet). You can have a look at my efforts. Since its is broken also in v2.12.2, I'd say these are 2 separate issues, so how about we merge this one in first and I'd work on its in parallel?

Thanks, I'll take a look. I don't have the time right now but hope to get to it this weekend.

And yes, I consider these 2 separate issues that can be merged separately. I'd like to figure out what we're doing about the docs at first, though.

@exviva

@myronmarston I've combined the 2 modules, please have a look.

One side effect of this is that SharedContext now has the subject, subject! and its class methods (since they're now part of the same module as let and let!). This is undocumented and untested behaviour, and I don't think it's harmful, so I guess we could leave it like this. If you disagree, I'll extract these sets of methods into 2 separate modules.

@myronmarston myronmarston commented on the diff Jan 4, 2013
lib/rspec/core/memoized_helpers.rb
+ self.class.class_eval do
+ define_method(:subject) do
+ if defined?(@_subject)
+ @_subject
+ else
+ @_subject = Array === attribute ? super()[*attribute] : _nested_attribute(super(), attribute)
+ end
+ end
+ end
+ instance_eval(&block)
+ end
+ end
+ end
+ end
+
+ module LetDefinitions
@myronmarston
RSpec member
myronmarston added a line comment Jan 4, 2013

I don't think we need to put these methods into this module...they can just exist as direct singleton methods on the MemoizedHelpers module. Singleton methods don't get added to the host class when the module is mixed in so there's no danger of "leaking" these methods into the user's namespace.

Plus, I think it's kinda confusing that the code below looks for a module named LetDefinition and this one is also called LetDefinitions, but the module it is looking for is not this module....it's example_group::LetDefinitions.

@exviva
exviva added a line comment Jan 5, 2013

Will do.

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

One side effect of this is that SharedContext now has the subject, subject! and its class methods (since they're now part of the same module as let and let!). This is undocumented and untested behaviour, and I don't think it's harmful, so I guess we could leave it like this. If you disagree, I'll extract these sets of methods into 2 separate modules.

Actually, I noticed that SharedContext lacked these methods a while ago and thought it was a bit of a curious oversight (or bug, arguably). If the new added methods work fine with SharedContext then we should keep the, but add specs to demonstrate that they work. If they don't work, we should either find a way to make them work or remove them because it would be confusing to have them if they didn't work. If we remove them, I'm a little loath to separate this into 2 modules again, though...so I wonder if it would be simpler to use undef in the SharedContext module to remove any methods that don't work?

Alternately, shared_context provides all of the RSpec DSL methods, and the implementation is simpler than SharedContex, to boot. I've never used SharedContext and never seen it used in the wild. @dchelimsky, what do you think about the possibility of deprecating SharedContext with the plan to remove it in 3.0? Or do you think there are use cases that SharedContext meets better than shared_context? I'm not sure we need both...

@exviva

Ok, I think I'm getting somewhere. Have a look at these examples:

describe 'Implicit subject instantiates the most top-level class used in `describe`' do
#  it { should be_an_instance_of(String) }
  its(:class) { should eq(String) }

  describe OuterClass do
    it { should be_an_instance_of(OuterClass) }
    its(:class) { should eq(OuterClass) }

    describe 'another string' do
      it { should be_an_instance_of(OuterClass) }
      its(:class) { should eq(OuterClass) }

      describe InnerClass do
        it { should be_an_instance_of(OuterClass) }
        its(:class) { should eq(OuterClass) }
      end
    end
  end
end

They all pass on v2.12.2, but only until I...uncomment the first it { should .... As soon as I uncomment this line, all but the first 2 examples fail. I think this is a bug in v2.12.2, but nobody has even used nested groups this way. Most probably people have a top-level class, and then strings, as descriptions.

My proposed solution is to mix in the implicit subject only if the described class is a class or module. Otherwise there should be no implicit subject. This way you can nest groups multiple times, mixing strings and classes/modules as descriptions, and the implicit subject will always instantiate the closest Class (or return the closest Module), which seems most intuitive.

Another idea I see is to mix in the implicit subject only in the top-level example group, but this doesn't feel right if people want to nest describe MyClass inside describe 'a bigger story'.

The problem with its (which turned out to be just a tip of the iceberg) is that the implicit subject gets implemented on each example group, so as soon as I do describe Foo; describe 'bar', 'bar' becomes the implicit subject in the inner group. Now since its uses a nested group and calls super(), they're using the implicit subject for that nested group, not the subject from the parent group.

@myronmarston myronmarston merged commit 0c70d63 into rspec:master Jan 5, 2013

1 check failed

Details default The Travis build failed
@myronmarston
RSpec member

@exviva -- I started looking into the its stuff but since it needs implicit subject memoization to work correctly, it made sense to go ahead and merge what you have here. I took care of removing the unneeded LetDefinitions module, too.

@exviva exviva deleted the exviva:subject_not_memoized branch Jan 5, 2013
@myronmarston
RSpec member

I also cherry-picked over your its refactoring because it makes it much clearer and simpler than before.

@exviva

Cool. I finally understood, why the examples with its fail:

describe 'using its with before and let blocks' do
  subject { :symbol } # or whatever
  let(:subject_id_in_let) { subject.object_id }
  before { @subject_id_in_before = subject.object_id }

  its(:object_id) { should eq(subject_id_in_let) }
  its(:object_id) { should eq(@subject_id_in_before) }
end

It's because when let and before are evaluated, the subject they're referring to is actually :symbol.object_id, since its overrides subject, so hey end up calling :symbol.object_id.object_id. It's as if we wrote:

describe 'using its with before and let blocks' do
  subject { :symbol }
  let(:subject_id_in_let) { subject.object_id } # easy to think it'd always return :symbol.object_id...

  describe(:object_id) do
    subject { :symbol.object_id }
    it { should eq(subject_id_in_let) } # but here it actually returns :symbol.object_id.object_id
  end
end

In other words, doh...:). Actually, I'd call it a bug, because I'd still expect subject to refer to the outer subject, not the internal its subject. But we'd then probably need a special case in the code, I doubt that it's worth it.

I'll try master on my project on Monday, and come back if I find any more issues.

I've had some weird behaviour of described_class returning different values in a nested group, when running all specs or using the line number filter to run only the innermost group, or depending on whether or not some examples are commented out, but I'm too tired now to isolate it :).

@myronmarston
RSpec member

Nice work getting to the bottom of this. That's very, very subtle, and is another argument for using its sparingly, if at all...we plan to move it out of rspec-core into another gem in rspec-3 to signal that it's not really core to rspec, so to speak.

I've had some weird behaviour of described_class returning different values in a nested group, when running all specs or using the line number filter to run only the innermost group, or depending on whether or not some examples are commented out, but I'm too tired now to isolate it :).

Yep, I was playing with the example you pasted above, too, and there is something weird going on. I'm going to play with it more to get to the bottom of it.

And actually, in your example above, here's how I actually think it should behave:

describe 'Implicit subject instantiates the most top-level class used in `describe`' do
#  it { should be_an_instance_of(String) }
  its(:class) { should eq(String) }

  describe OuterClass do
    it { should be_an_instance_of(OuterClass) }
    its(:class) { should eq(OuterClass) }

    describe 'another string' do
      it { should be_an_instance_of(OuterClass) }
      its(:class) { should eq(OuterClass) }

      describe InnerClass do
        it { should be_an_instance_of(InnerClass) }
        its(:class) { should eq(InnerClass) }
      end
    end
  end
end

In other words, I think the implicit subject should be an instance of the inner-most described class, or if, there is not described class, the outermost described thing.

@dchelimsky -- any thoughts on what the correct thing is when nesting a describe SomeClass inside another?

@myronmarston
RSpec member

@exviva -- I just pushed a fix to described_class in 538285b that was the root of the weirdness you mentioned above. Want to give that a try to see if it solves your problems?

@exviva

@myronmarston nice work with described_class!

I've just used 538285b in my project, and unfortunately it still has a regression in its. Here's the failing example (passing on v2.12.2):

describe Coupon do
  describe 'delegating name to source' do
    let(:name) { 'Best program ever!' }
    before { subject.source = LoyaltyProgram.new(name: name) }

    its(:name) { should eq(name) }
  end
end

Failure is RuntimeError: Coupon#name delegated to source.name, but source is nil.

I'll investigate it a bit today.

@exviva

@myronmarston here's a solution I came up with to keep its behaviour backward compatible. I've used my project's specs to check if it's working.

If you like it, I'll cover it with proper specs and docs.

@myronmarston
RSpec member

@exviva -- it's hard to say whether or not I like that solution because it's not clear to me what the bug is. Can you come up with an rspec-core commit that adds a failing example for your use case?

@exviva

Ah, sorry. The bug is that its changes subject, so before and let blocks refer to the wrong subject. This is passing on v2.12.2, but fails on master:

MyClass = Struct.new(:some_attr)

describe MyClass do
  before { subject.some_attr = :foo } # [1]
  its(:some_attr) { should eq(:foo) }
end

On line [1], subject used to refer to MyClass.new (or whatever the subject for that level was), now it refers to MyClass.new.some_attr, because its overrides subject.

I'll write a failing example in a moment.

@myronmarston
RSpec member

Ah, I see what the problem is now...in 2.12.2 (and before), its defined subject from within the example--which means it waited until the example ran to override it--which means that before hooks like in this case would run with the old definition of subject. It seemed very odd to me, and with your refactoring to its, we moved the redefinition of subject into the example group defined by its--which is more natural (and less odd), but breaks this behavior. I think we should just move it back into the example, as that doesn't make us introduce a new method like subject_for_receiverless_should.

@exviva

@myronmarston I don't think this will solve the problem, at least not for let - if a subject-accessing let block is lazily evaluated for the first time from the its block, it'll have access to the wrong subject.

We can either have Example#should and Exampe#should_not call a different method (like my patch showed), or we could include/extend a module which overrides these two methods. WDYT?

@myronmarston
RSpec member

I don't think this will solve the problem, at least not for let - if a subject-accessing let block is lazily evaluated for the first time from the its block, it'll have access to the wrong subject.

Hmm, you're right. That said, I believe that has been the behavior of let in a its block for many, many rspec releases--so I wouldn't consider this a regression.

We can either have Example#should and Exampe#should_not call a different method (like my patch showed), or we could include/extend a module which overrides these two methods. WDYT?

The latter approach appeals to me because I'd prefer not to introduce another method (for reasons we've discussed previously). However, I'm not convinced that it'll solve the problem; changing what should and should_not delegate to won't affect a let declaration that references subject right?

@exviva exviva referenced this pull request Jan 8, 2013
Closed

Bug in subject memoization #766

@exviva

Including a module does the trick: https://github.com/exviva/rspec-core/compare/its_subject.

But there is no way of solving this without adding a new method to the example (with def or let). After all, if let refers to subject, and its(:foo) { should ..., calls subject too, they'll return the same object, which we don't want.

And since I'd expect let { subject ... } to access the outer subject, the only way to fix this is to make #should inside its not access the same subject. Phew, this is hard to explain :).

Have a look at my branch, let me know if it makes sense, if yes, I'll add some more docs and open a PR.

@myronmarston
RSpec member

I like that solution a lot. My one suggestion is to see if we can make the specs for that more clear...they're a bit convoluted. Please open a PR for it!

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