Expect to receive with a private method

[Daniel Vandersluis]

We recently accidentally moved a method from public to private and it was not caught by our test suite because expect(obj).to receive(:method) succeeds regardless of whether or not obj.method is actually accessible or not.

I'm not sure if there's an easy way to get around this (because it should be possible to stub out private methods of course). I can't think off hand of a way to determine if the object has access to a method without calling it, which obviously an expectation shouldn't be doing generally. Maybe we can check if the receiver is self?

If there's a way to do this, it'd be nice to have this as a configuration option!

Thanks,

Daniel Vandersluis

[Jack Royal-Gordon]

How about the following:

expect(object.new.public_methods(false)).to include(:method)

That allows you to explicitly check on method in one object; you can easily alter it to check for multiple methods. But that’s explicit checking; I assume you would prefer something more implicit, but I don’t know of anything that would do that.

--
You received this message because you are subscribed to the Google Groups "rspec" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rspec+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rspec/c0d84ace-2f0d-4730-83d9-e4649a36d02en%40googlegroups.com.

[Jon Rowe]

Hi Daniel

rspec-mocks does not change the visibility of methods when mocking them, this means that your method here should have remained private when you change it, and should have resulted in a `NoMethodError` if called directly for example:

class MyClass

  Yarr = "Yarr"

  def good_ship?

    pirate_method != Yarr

  end

  private

  def pirate_method

    Yarr

  end

end

RSpec.describe "privacy" do

  let(:object) { MyClass.new }

  it "works with private methods" do

    expect(object).to receive(:pirate_method) { "We're a good ship" }

    expect(object.good_ship?).to eq true

  end

  it "works with pirate methods" do

    expect(object).to receive(:pirate_method) { "We're a good ship... honest" }

    object.pirate_method

  end

end

The first spec passes because pirate_method, although private, is called from an internal construct, and is thus allowed and verifying.

The second spec fails with:

NoMethodError:

       private method `pirate_method' called for #<MyClass:0x00007fd1a8a50650>

And if you stub it, it will actually say it failed to be called even if you try to call it.

So if you experienced the change somehow silently breaking your tests, chances are any additional checks we could do, would also not help…

Can you share a more specific example?

Cheers

Jon Rowe

---------------------------

ma...@jonrowe.co.uk

jonrowe.co.uk

[Daniel Vandersluis]

Jon Rowe,

My test looked something like this:

class MyObject

  def call

    do_something

  end

private

  def do_something # this used to be public!

    #...

  end

end

subject { MyObject.new }

expect(obj).to receive(:do_something)

subject.call # this calls do_something

Regardless of whether do_something is public or private, the expectation passes, but when called outside of a test, the method fails with NoMethodError because the method became private. I totally agree that the tests we had were not adequate, but also it wasn't something we were able to catch.

Daniel

[Phil Pirozhkov]

On Tue, Jul 21, 2020 at 11:16 PM 'Daniel Vandersluis' via rspec <rs...@googlegroups.com> wrote:

My test looked something like this:

class MyObject

  def call

    do_something

  end

private

  def do_something # this used to be public!

    #...

  end

end

subject { MyObject.new }

expect(obj).to receive(:do_something)

subject.call # this calls do_something

Regardless of whether do_something is public or private, the expectation passes, but when called outside of a test, the method fails with NoMethodError because the method became private. I totally agree that the tests we had were not adequate, but also it wasn't something we were able to catch.

MyObject.new.call # => nil 

Is it different on your end?

- Phil

[Jon Rowe]

Yep that should continue to work, from the perspective of  `call` it doesn’t matter wether `do_something` is private or not, it is just an implementation detail to it.

Really to catch this you should also write a spec for `do_something` in its own right, if its part of your “public api” it is often a good idea to test this.

I wouldn’t advocate this, but if its too expensive to call un-mocked this would check its available publicly:

RSpec.describe “#do_something” do

  # its too expensive to call in tests, but we want to ensure its public

  it “is a public method” do

    object = MyObject.new

    expect(object).to receive(:do_something)

    object.do_something

  end

end

However, if you’re using standard Ruby semantics, this might make more sense:

RSpec.describe “#do_something” do

  it “is a public method” do

    object = MyObject.new

    expect(object).to respond_to(:do_something)

  end

end

Cheers

Jon Rowe

---------------------------

ma...@jonrowe.co.uk

jonrowe.co.uk

[Daniel Vandersluis]

Sorry my example was slightly off (pains of trying to anonymize!), because it's actually calling a (now) private method of a different class. Oops!

Here's a better sample:

class Post
  private

  def process # used to be public
    # ...
  end
end

class Operation
  def self.call
    post.process
    # do other things here
  end
end

RSpec.describe Operation do
  subject { described_class }

  it do
    expect(post).to receive(:process)
    subject.call
  end
end

Now the test still succeeds but the code doesn't. I understand your suggestion of testing to make sure that methods are the expected accessibility, but this is probably too cumbersome in practice in a large codebase.

[Filipp Pirozhkov]

On Wednesday, July 22, 2020 at 3:55:00 AM UTC+3, Daniel Vandersluis wrote:

Sorry my example was slightly off (pains of trying to anonymize!), because it's actually calling a (now) private method of a different class. Oops!

Thanks for taking the time, now I clearly understand what you mean.

I don't see a problem with how you test `Operation`. It's your choice to set the boundaries like that, and test it as a unit, avoiding testing `Post#process`'s side effects.

There's another problem, though. It's the coverage of `Post`. If you stub the call to `Post#process` in one test, then you have to test what `Post#process` does on its own.

And here's where you'll be able to identify the issue. Just like your program itself, RSpec won't be able to call `Post#process` from specs. Meaning that if you write a test for it, it will tell that the method is private.

And you'll have to either opt-in for `post.public_send(:process)`. It is a smell, as you're testing a private method.

Sometimes, I believe, there's no other way around that. But in that case you should clearly know what you are doing. Specifically, make sure you don't stub this method anywhere.

There seems nothing wrong to me with RSpec itself, it's about design of the specs.

Do you have an idea of something that would have helped you to identify this issue earlier, like `config.warn_on_stubbing_private_methods!`?

- Phil

[Daniel Vandersluis]

Phil, thanks for your reply. I agree that this was a fault of test coverage (I'm sure everyone agrees, not all tests in a test suite are as good as they can be, and there isn't enough time in the day to keep tests audited).

I guess what I was looking to see if it was possible is to have rspec protect us from ourselves, definitely not a failing of RSpec specifically. Ideally I think what would be helpful is to be able to see if a stubbed method is being called in a public or private context, but that doesn't seem like something ruby can do (at least not easily or probably performantly). A warning on all stubbed private methods would probably generate a lot of unwanted noise, because it's standard practice to do write a test where you're *purposely* stubbing out a private method (for instance a method that does heavy processing or hits an external dependency).

So I guess the answer to my question of "can RSpec do this?" is probably "no". :)

Daniel

[Jon Rowe]

Late to the party but your example is incomplete, how is post defined?

Jon Rowe

---------------------------

ma...@jonrowe.co.uk

jonrowe.co.uk