Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

#stub does not force defined method to be public #117

Closed
myronmarston opened this Issue · 10 comments

3 participants

@myronmarston

I ran into some weird surprising behavior today. Here's a spec:

describe "a stub" do
  specify do
    test_double = Module.new
    test_double.stub(:use)
    test_double.use
  end
end

Running it normally:

✗ rspec foo_spec.rb 

a stub


Finished in 0.0009 seconds
1 example, 0 failures

Running it with sinatra loaded:

rspec foo_spec.rb -rsinatra

a stub
   (FAILED - 1)

Failures:

  1) a stub 
     Failure/Error: test_double.use
     NoMethodError:
       private method `use' called for #<Module:0x00000102a390a0>
     # ./foo_spec.rb:5:in `block (2 levels) in <top (required)>'

Finished in 0.00075 seconds
1 example, 1 failure

I think this is because Sinatra::Delegator defines use as a private method, and it gets included into the main object.

I think #stub and #should_receive should make the defined method public but do not currently. @dchelimsky / @justinko -- thoughts on this?

@justinko

I don't think messing with method visibility is a good idea -- you're changing implementation code. Obviously, RSpec Mocks does this, but it keeps the behavior of the original method (including visibility).

You should never have to explicitly call a private method in a spec. How are you running into this problem?

@myronmarston

You should never have to explicitly call a private method in a spec. How are you running into this problem?

I've been using rspec-fire on a new project. I'm experimenting with using an anonymous module for the fire class doubles rather than a subclass of RSpec::Mocks::Mock, because modules allow further nested constants (like a real class), but an RSpec::Mocks::Mock does not. I came up with a minimal example that demonstrates what I'm trying to do:

https://gist.github.com/2042094

Notice that the "real" method that corresponds to what I'm trying to mock is public. But once sinatra is loaded in the environment it fails :(.

@justinko

rather than a subclass of RSpec::Mocks::Mock

Okay, so that's why I've never run into this issue before. As you can see, rspec-mocks will set the visibility to "public" if it's a Mock:

https://github.com/rspec/rspec-mocks/blob/master/lib/rspec/mocks/method_double.rb#L29

@justinko

1.) This is just another example of why injecting methods into the global object space is a bad idea, generally. Does Sinatra really need to do that? And even make them private?

2.) Knowing number 1, can you just not use use? Or, switch back to using Mock? Personally, I would hate not being able to use use, but RSpec is not at fault here (it sees it as "private", as it should).

@myronmarston

1.) This is just another example of why injecting methods into the global object space is a bad idea, generally. Does Sinatra really need to do that? And even make them private?

I think it does to give you the pretty DSL at the top level. However, it turns out you can require sinatra/base and just get the base class to use as a superclass for my app, without polluting the top level. I made this change and it solved the problem for me, but I think the deeper problem remains: these class test doubles in rspec-fire are prone to being messed up by other libraries doing something to the main object.

2.) Knowing number 1, can you just not use use? Or, switch back to using Mock? Personally, I would hate not being able to use use, but RSpec is not at fault here (it sees it as "private", as it should).

The switch from using Mock to using a module is in a pull request for now so it's not done deal, but using a module for a class double provides a huge benefit: it allows nested constants. This is especially important when using it in place of the top-level namespace module of a gem your testing. W/o using a module, once the constant replacement happens, you've essentially cut off all access to any constants in the gem, which is a problem. Using a module for the class test double allows you to transfer nested constants to the test double and continue to have access to the constants.

Your idea of switching back to Mock brings up another idea for me, though: if rspec-mocks had a way to tell it that an object is a true test double and not a partial mock even though it does not subclass RSpec::Mocks::Mock, it could treat it with the same rules it treats a Mock and the problem would be solved. Two ideas for how we could do that:

  • Refactor so that RSpec::Mocks::Mock is something like:
class RSpec::Mocks::Mock
  include RSpec::Mocks::TestDouble
end

Essentially, refactor so that there's a mixin. In rspec fire, we can extend the test double module with this mixin as well. The code in RSpec that does Mock === object would have to be changed to do TestDouble === object (or whatever the name of the mixin is).

  • Provide an API to register objects as true test doubles. Something like RSpec::Mocks.register_test_double. The code that currently looks like Mock === object would then have to check if it's a Mock or a subclass or if it's been registered.

Thoughts?

@dchelimsky
Owner

@myronmarston - @patmaddox and I discussed having rspec-mocks swap in constants before, but the context was an alternate (hopefully better) way to deal with stubbing and expecting on class constants. Right now we butcher the class and try our best to restore it to its original state after the example. Much cleaner to just do something like:

def replace(klass)
  originals[klass.name] = klass
  Object.send(:remove_const, klass.name)
  Object.const_set(klass.name, Class.new { include Double })
end

def restore(klass)
  Object.send(:remove_const, klass.name)
  Object.const_set(klass.name, originals[klass.name])
end

replace (or other name) would be exposed in examples, restore would be called automatically after each example.

I'd sooner do this than add an API for registering them. Although ... replace could take an optional 2nd arg:

def replace(klass, replacement=nil)
  replacement ||= Class.new { include Double }
  originals[klass.name] = klass
  Object.send(:remove_const, klass.name)
  Object.const_set(klass.name, replacement)
end

There's your API - serving end users and extension libs alike.

@justinko

@myronmarston ah, okay now I see what you mean by nested constants.

@myronmarston

@dchelimsky -- it's interesting that you're bringing up the idea of a constant stubbing API, because I've been writing the constant stubbing code in rspec-fire with the idea of potentially extracting it into rspec-mocks some day :). For now, I've been thinking it's good to use a less-used library like rspec-fire as a testing ground for something like this. FWIW, the constant-stubbing code in rspec-fire does a fair bit more than your API above. It handles both defined and undefined constants (so you can use it to stub unloaded dependencies) and it handles constants at any level of nesting (such as X::Y::Z).

@patmaddox and I discussed having rspec-mocks swap in constants before, but the context was an alternate (hopefully better) way to deal with stubbing and expecting on class constants.

That certainly does seem like a cleaner way to stub class methods, but it seems like there could be surprises if the constant is not re-evaluated--i.e. if a reference to the original constant is stored previously and used in an example, then the stubbing/mocking on it won't work. This could be as simple as a shared example group using described_class to reference the class-under-test.

Anyhow...I take based on your code examples above that you're open to the idea of making RSpec::Mocks::Mock a mixin (named something like Double, according to your example)?

@dchelimsky
Owner

@myronmarston yes, making it a mixin is fine.

@myronmarston

Cool, I'll take a crack at it.

@myronmarston myronmarston closed this issue from a commit
@myronmarston myronmarston Extract mock logic into TestDouble module.
This allows objects to be treated as a pure test double without needing to subclass RSpec::Mocks::Mock.

Closes #117.
af3f296
@myronmarston myronmarston referenced this issue from a commit
@myronmarston myronmarston Remove `TestDouble.extend_onto`.
This was added in #117 to support using a module
as a pure test double by extending `TestDouble`
onto the module. Since then, I've realized we can
get the same behavior by subclassing module:

class MyModuleTestDouble < Module
  include TestDouble
end

...and that's what we do for class verifying doubles now.

Given that, there's no need to keep this API around anymore.
bc573f4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.