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

Initial implementation of warning in before(:all) blocks #449

Merged
merged 1 commit into from Jan 2, 2014

Conversation

@penelopezone
Copy link
Member

penelopezone commented Nov 2, 2013

So this works, but I'm not sure I like the implementation.

The nice thing is that it doesn't talk to RSpec core at all and achieves the desired behaviour (I believe). On the minus side it requires tracking more state and because we can't do message expectations in before blocks I've had to stub Kernel.warn manually, also I'm not sure if the RSpec::Mocks class itself is the right place to track this state.

I'd love some design feedback here, it's a little rough and ready, but it definitely solves the problem.

An example of what it looks like when running outside of RSpec's own test suite:

(warn-stubbing-before-all)$ ber --color hi_spec.rb 
/Users/sam/dev/rspec/rspec-dev/repos/rspec-support/lib/rspec/support/warnings.rb:28: warning: method redefined; discarding old warning
/Users/sam/dev/rspec/rspec-dev/repos/rspec-core/lib/rspec/core/warnings.rb:25: warning: previous definition of warning was here
/Users/sam/dev/rspec/rspec-dev/repos/rspec-support/lib/rspec/support/warnings.rb:35: warning: method redefined; discarding old warn_with
/Users/sam/dev/rspec/rspec-dev/repos/rspec-core/lib/rspec/core/warnings.rb:32: warning: previous definition of warn_with was here
/Users/sam/dev/rspec/rspec-dev/repos/rspec-mocks/lib/rspec/mocks.rb:19: warning: instance variable @usable not initialized
WARNING: calling stub on objects in `before(:all)` blocks is unsupported. Consider using `let` or `before(:each)` instead. Called from /Users/sam/dev/rspec/rspec-dev/repos/rspec-mocks/hi_spec.rb:5:in `block (2 levels) in <top (required)>'.
F

Failures:

  1) qwoifej return nil
     Failure/Error: Foo.find.should eq(nil)
     NoMethodError:
       undefined method `find' for Foo:Class
     # ./hi_spec.rb:9:in `block (2 levels) in <top (required)>'

Finished in 0.00059 seconds
1 example, 1 failure

Failed examples:

rspec ./hi_spec.rb:8 # qwoifej return nil
@penelopezone
Copy link
Member Author

penelopezone commented Nov 2, 2013

Fixes #240

@@ -22,6 +22,13 @@ def self.warn_unless_should_configured(method_name)
end

# @api private
def self.warn_about_before_all_blocks(method_name)
(!RSpec::Mocks.usable?).tap do |warn|
RSpec.warning("calling #{method_name} on objects in `before(:all)` blocks is unsupported. Consider using `let` or `before(:each)` instead") if warn

This comment has been minimized.

Copy link
@xaviershay

xaviershay Nov 2, 2013

Member

If possible should include a "why" in this message, it's not immediately obvious.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 2, 2013

Member

Also, this message really only makes sense in the context of rspec-core. Consider that rspec-mocks can be used with other frameworks (e.g. test-unit, minitest, shindo, cucumber, etc). Getting a warning about before(:all) would be very confusing if you're not using rspec-core.

Instead, here's what I think we should do:

  • Have a default warning message that says something like: "the use of doubles or partial doubles from rspec-mocks outside of the per-test lifecycle is not supported". This message could make sense in any test framework.
  • Provide a config option that rspec-core will use to inject an rspec-core specific message that mentions before(:all). This could completely override the default warning message or simply add some text to append to it (or to interpolate in the middle of it -- not sure what's best yet).

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 2, 2013

Member

Also, I think this should be an error, not a warning. (Consider that calling let from before(:all) raises an error in rspec-core 3.0). We should add a warning to 2.99, though.

And one last thing: I'm not sure that the suggestion to use let makes much sense (let is about defining a named dependent object; it's not about mocking, and also, let from before(:all) is not supported). There are ways to get stub-like behavior in before(:all) that we could mention instead, such as using standard ruby constructs like defining singleton methods or extending a module onto a partial double. I've done this before as a way of stubbing a method on an object that was only used in before(:all). That also might be confusing, though...given that this is kinda a complex issue that's hard to summarize in an error message it might be good to have it refer to a gist or wiki entry or something that explains the issue and available work arounds.


before(:all) do
Kernel.singleton_class.send(:remove_method, :warn)
Kernel.send(:define_singleton_method, :warn) { |message| warn_message = message }

This comment has been minimized.

Copy link
@xaviershay

xaviershay Nov 2, 2013

Member

Since you're using RSpec.warning in the implementation, shouldn't that be the method you sub out here?

Also I'm guessing you have to redefine this explicitly is exactly because you're in a before all block? Seems like their should be a way for us to run an RSpec example group inside an example, but I'm not sure how off the top of my head.

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 2, 2013

Author Member

@xaviershay it's much easier to get the call site from the data that gets passed to Kernel#warn. You can run an example group inside an example, but at that point the RSpec::Mocks.space is usable and it gets kinda funky. Most people aren't running example groups inside examples, and I think it's easier to model this by manually munging kernel than it is to do some magic with RSpec's own libraries.


it "includes the call site in the warning" do
#Expect the call site to be the line 9 above where Object.stub(:foo) is called
expect(warn_message).to match(/#{__FILE__}:#{__LINE__-9}/)

This comment has been minimized.

Copy link
@xaviershay

xaviershay Nov 2, 2013

Member

Try to localize this LINE to what it is referencing by using a variable. -9 is a big jump and too likely to change.

@@ -29,18 +36,21 @@ def self.enable_should(syntax_host = default_should_syntax_host)

syntax_host.class_exec do
def should_receive(message, opts={}, &block)
return if ::RSpec::Mocks::Syntax.warn_about_before_all_blocks(__method__)

This comment has been minimized.

Copy link
@xaviershay

xaviershay Nov 2, 2013

Member

I wonder if we could do this polymorphically? I.e. "disable" this syntax for the before(:all) and "enable" a different one that only emits warnings. Probably would be too slow?

@myronmarston
Copy link
Member

myronmarston commented Nov 2, 2013

Thanks for tackling this, @samphippen. I was going to comment on #240 that RSpec::Mocks.space is not nil in all before(:all) hooks like you thought it was -- just in the first one. But it looks like you figured that out :).

Also, I have some changes planned coming to the interface between rspec-core and rspec-mocks,so that the setup method is split into one_time_setup and per_example_setup methods. The RSpec::Mocks.space initialization and the module includes would go in the one_time_setup and then the toggling of the usable flag you've created here would go in the per_example_setup method. See this comment for more of my musings on this topic. Regardless, it looks like your changes here don't conflict with my plans there so we can get this ready to merge before that's done (even though I had originally planned to do that one first).

Anyhow, I'll go do a detailed review of the code changes now :).

::RSpec::Mocks::Syntax.warn_unless_should_configured(__method__)
::RSpec::Mocks.space.proxy_for(self).remove_stub(message)
end

def stub_chain(*chain, &blk)
return if ::RSpec::Mocks::Syntax.warn_about_before_all_blocks(__method__)

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 2, 2013

Member

It looks like you've only disabled things for the legacy should syntax, and not for the expect syntax at all. Also, doing this in each method is error prone. Instead, I think there's a far simpler way to make this work:

  • Rather than setting a usable flag, replace RSpec::Mocks.space with an object that implements proxy_for in such a way that it raises a useful error. All rspec-mocks mocking/stubbing methods ultimately go through that to get a proxy so it can be centralized. You wouldn't have to litter all these methods with checks like this. Polymorphism FTW!
  • That actually opens up an interesting possibility: rather than setting a generic message for when rspec-core is not loaded (as I recommended above), we could have rspec-core take care of replacing RSpec::Mocks.space with the object that raises an error for before(:all). Then the message can be as specific to rspec-core as we want :).
  • There's also the issue of stubbing constants and and any_instance: we'll have to figure out how to make those interact well with all this.
@penelopezone
Copy link
Member Author

penelopezone commented Nov 2, 2013

@myronmarston you are so smart, this different mock space makes the code so much nicer (patches incoming).

@penelopezone
Copy link
Member Author

penelopezone commented Nov 2, 2013

@myronmarston @xaviershay updated, but the expect syntax methods aren't available in the before(:all) block. I assume this has always been the case, but do you think we should make them also raise the same set of messages. (I've included a failing test)

@penelopezone
Copy link
Member Author

penelopezone commented Nov 2, 2013

@myronmarston given that this is now an exception do I need to include a backtrace includes correct call site test? I'd have thought not as it can be assumed?

@penelopezone
Copy link
Member Author

penelopezone commented Nov 2, 2013

Also here's some output from RSpec with this exception:

(warn-stubbing-before-all)$ ber hi_spec.rb 
/Users/sam/dev/rspec/rspec-dev/repos/rspec-support/lib/rspec/support/warnings.rb:28: warning: method redefined; discarding old warning
/Users/sam/dev/rspec/rspec-dev/repos/rspec-core/lib/rspec/core/warnings.rb:25: warning: previous definition of warning was here
/Users/sam/dev/rspec/rspec-dev/repos/rspec-support/lib/rspec/support/warnings.rb:35: warning: method redefined; discarding old warn_with
/Users/sam/dev/rspec/rspec-dev/repos/rspec-core/lib/rspec/core/warnings.rb:32: warning: previous definition of warn_with was here
F

Failures:

  1) qwoifej return nil
     Failure/Error: Foo.stub(:find)
     RuntimeError:
       The use of doubles or partial doubles from rspec-mocks outside of the per-test lifecycle is not supported.
     # ./lib/rspec/mocks/error_space.rb:18:in `raise_lifecycle_message'
     # ./lib/rspec/mocks/error_space.rb:5:in `proxy_for'
     # ./lib/rspec/mocks.rb:29:in `proxy_for'
     # ./lib/rspec/mocks.rb:56:in `allow_message'
     # ./lib/rspec/mocks/syntax.rb:49:in `stub'
     # ./hi_spec.rb:5:in `block (2 levels) in <top (required)>'

Finished in 0.00041 seconds
1 example, 1 failure

Failed examples:

rspec ./hi_spec.rb:8 # qwoifej return nil

Deprecation Warnings:

Using `stub` from the old `:should` syntax without explicitly enabling the syntax is deprecated. Use the new `:expect` syntax or explicitly enable `:should` instead. Called from /Users/sam/dev/rspec/rspec-dev/repos/rspec-mocks/hi_spec.rb:5:in `block (2 levels) in <top (required)>'.

1 deprecation warning total
(warn-stubbing-before-all)$ 
@penelopezone
Copy link
Member Author

penelopezone commented Nov 2, 2013

the expect syntax methods aren't available in the before(:all) block.

This is false, I broke something. Going to give fixing it a try now.

@penelopezone
Copy link
Member Author

penelopezone commented Nov 2, 2013

Ok, so expect is, allow isn't, receive etc isn't. Do we want to include all the expect syntax in a before(:all) hook for this warning, or is the no method error good enough?

@myronmarston
Copy link
Member

myronmarston commented Nov 2, 2013

Ok, so expect is, allow isn't, receive etc isn't. Do we want to include all the expect syntax in a before(:all) hook for this warning, or is the no method error good enough?

This is happening due to the slightly odd way we include RSpec::Mocks::ExampleMethods in RSpec::Mocks.setup -- we include it in the singleton class of the example group instance.

I plan to re-jigger this so that it is included directly in RSpec::Core::ExampleGroup once, which will solve that problem. That's part of the changes I've been planning that I mentioned above (e.g. splitting setup into one_time_setup vs per_project_setup).

I think we should hold off on this until that's done. At the moment I'm focused on other things for the upcoming prerelease so this'll have to sit tight for now. Is that OK?

@myronmarston
Copy link
Member

myronmarston commented Nov 26, 2013

@samphippen -- my changes in #471 have been merged, so it should make this easier now. Want to take a stab at rebasing this and wrapping it up? Let me know when you want another review.

@penelopezone
Copy link
Member Author

penelopezone commented Dec 12, 2013

@myronmarston I've been super busy recently. If anyone wants to build on top of this let me know. Otherwise I'm happy to look at it over my break for the holidays!

@penelopezone
Copy link
Member Author

penelopezone commented Jan 1, 2014

@myronmarston I'm having an issue where autogenerated descriptions for examples are being set after RSpec mocks gets torn down and the error space is in place.

I'd like to add a hook that gets called when RSpec.current_example = self happens (run_before_example) and one that gets called when RSpec.current_example = nil happens (run_after_example). What do you think?

@penelopezone
Copy link
Member Author

penelopezone commented Jan 1, 2014

@myronmarston I think this is good to go (it passed locally for me). I had to add a setup hook for the test-unit feature. How do you feel about that?

expectation
@expectation ||= mock_proxy.build_expectation(@method_name)
apply_constraints_to @expectation
@expectation

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 1, 2014

Member

Seeing how you did this...I think it would actually be best to memoize the entire method. I'm concerned about the fact that constraints can be applied to the same expectation instance which seems potentially problematic.

@@ -33,6 +34,7 @@ def self.verify
# each example, even if an error was raised during the example.
def self.teardown
space.reset_all
self.space = RSpec::Mocks::ErrorSpace.new
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 1, 2014

Member

setup and teardown are each assigning space to a new object instance -- so this has the effect of adding two more objects to every example, which can add to the GC time a test suite has. ErrorSpace is stateless so it's fine to re-use the same instance. Space has logic that takes care of clearing things between examples (e.g. reset_all clears out its various arrays/hashes), so I think we can re-use the space instance as well (given that was happening before)...

That said, I'm actually leaning towards using a new fresh space for each example, and just removing unneeded logic from Space#reset_all (e.g. the clear calls), since using a new instance seems like a cleaner way to have clean slate.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 1, 2014

Member

Hmm, one potential issue with using a new space for each example: if an object was created in one example, but is still referenced or used in another example, it'll be a different space it uses for its mock proxy than what it originated with. That may have some bad repercussions. I need to think about it more, though.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 1, 2014

Member

OK, so thinking more about this, I think that until we implement #352, it could be problematic to use a different space instance for different examples. So -- let's keep re-using the same space instance for now, and consider changing it when we implement #352.


# Performs per-test/example setup. This should be called before
# an test or example begins.
def self.setup(host)
# Nothing to do for now
def self.setup(host=nil)

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 1, 2014

Member

Are you planning to remove the host arg entirely once we update rspec-core?

This comment has been minimized.

Copy link
@penelopezone

penelopezone Jan 1, 2014

Author Member

Not sure. Do other people use this setup/teardown/verify as an integration point?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 1, 2014

Member

Well, even if they do, it's a confusing API to have an unused argument. We should remove the arg and find a way to deprecate it in 2.99.

@@ -0,0 +1,28 @@
module RSpec
module Mocks
OutsideOfExampleError = Class.new(StandardError)

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 1, 2014

Member

Needs a YARD doc comment.

module Mocks
OutsideOfExampleError = Class.new(StandardError)

class ErrorSpace

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 1, 2014

Member

This does too. It can (and should) be marked as private, though.

@myronmarston
Copy link
Member

myronmarston commented Jan 1, 2014

I had to add a setup hook for the test-unit feature. How do you feel about that?

It's fine. We used to have one, but I removed it in 7af0bec since setup was a no-op at that point, but with the plan to add it back once you implemented this.

@myronmarston
Copy link
Member

myronmarston commented Jan 2, 2014

Hey @samphippen, I saw your gist. It definitely feels like a hack, though. In #240 we've discussed adding a new with_temporary_scope API that can be used to scope some code to a particular rspec-mocks space. If we implement that correctly (e.g. so that it uses a stack and supports nested scopes), I think we can use it as a replacement for rspec-core's sandboxing. It'll be nice in the long run for rspec-mocks to directly support the kind of sandboxing rspec-core's specs need, anyway.

I have a fair bit of experience with the sandboxing logic so let me know if you'd like me to take a stab at this, otherwise I'm happy to let you do it :).

end

describe "#stub" do
it_behaves_like "A stub/mock in a before(:all) block", lambda { Object.stub(:foo) }

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 2, 2014

Member

The lambda and instance_exec above for it feel kinda hacky. Instead, I would recommend this:

it_behaves_like "A stub/mock in a before(:all) block" do
  def use_rspec_mocks
    Object.stub(:foo)
  end
end

In the before(:all) block above you can just call use_rspec_mocks (rather than the instance_exec business).

@myronmarston
Copy link
Member

myronmarston commented Jan 2, 2014

This needs a changelog entry, and could maybe be squashed a bit, then I think it's ready to merge.

penelopezone pushed a commit that referenced this pull request Jan 2, 2014
Sam Phippen

describe "#stub" do
it_behaves_like "A stub/mock in a before(:all) block" do
def use_rspec_mocks

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 2, 2014

Member

What's up with the 3-space indentation rather than 2?

This comment has been minimized.

Copy link
@penelopezone

penelopezone Jan 2, 2014

Author Member

I've no idea, an entertaining text editor failure. I'll take a look.

@@ -7,6 +7,8 @@ Breaking Changes for 3.0.0:
* Change how to integrate rspec-mocks in other test frameworks. You now
need to include `RSpec::Mocks::ExampleMethods` in your test context.
(Myron Marston)
* Prevent RSpec mocks' doubles and partial doubles from being used
outside of the per-test lifecycle. (Sam Phippen)

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 2, 2014

Member

Might be good to add a clarifying note like:

  outside of the per-test lifecycle (e.g. from a `before(:all) hook`). (Sam Phippen)
For example: in a before(:all) block.
penelopezone pushed a commit that referenced this pull request Jan 2, 2014
Initial implementation of warning in before(:all) blocks
@penelopezone penelopezone merged commit b6ca52f into master Jan 2, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
@penelopezone penelopezone deleted the warn-stubbing-before-all branch Jan 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.