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

Setting expectation on private method inherited from Kernel not working #260

Closed
chrisroos opened this Issue Aug 26, 2016 · 9 comments

Comments

Projects
None yet
2 participants
@chrisroos
Member

chrisroos commented Aug 26, 2016

This has been created based on @grosser's comment in PR #244.

You can replicate the problem by creating the following acceptance test in test/acceptance/setting_expectation_on_private_method_test.rb:

require File.expand_path('../acceptance_test_helper', __FILE__)
require 'mocha/setup'

class SettingExpectationOnPrivateMethodTest < Mocha::TestCase
  include AcceptanceTest

  def setup
    setup_acceptance_test
  end

  def teardown
    teardown_acceptance_test
  end

  def test_setting_expectation_on_private_method_inherited_from_kernel
    instance = Class.new.new

    instance.expects(:warn)
    instance.warn
  end
end

NOTE. This is slightly different from the code in @grosser's comment (the class under test doesn't include a method_missing) but I think it illustrates the same problem.

Running the test against master (2c64209) results in the following failure:

$ bundle exec ruby test/acceptance/setting_expectation_on_private_method_test.rb 
Run options: --seed 30417

# Running:

E

Finished in 0.001600s, 625.0000 runs/s, 0.0000 assertions/s.

  1) Error:
SettingExpectationOnPrivateMethodTest#test_setting_expectation_on_private_method_inherited_from_kernel:
NoMethodError: private method `warn' called for #<#<Class:0x007fc6e6c43178>:0x007fc6e6c43128 @mocha=nil>
    test/acceptance/setting_expectation_on_private_method_test.rb:19:in `test_setting_expectation_on_private_method_inherited_from_kernel'

1 runs, 0 assertions, 0 failures, 1 errors, 0 skips

Running the test against the v1.1.0 tag results in a pass:

$ bundle exec ruby test/acceptance/setting_expectation_on_private_method_test.rb 
Run options: --seed 59809

# Running tests:

.

Finished tests in 0.002051s, 487.5670 tests/s, 487.5670 assertions/s.

1 tests, 1 assertions, 0 failures, 0 errors, 0 skips
@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Aug 26, 2016

Member

I think the change in behaviour was introduced in e87c03b.

Running the test against e87c03b068efc48267fbcd5a295514077c52b901:

$ git co e87c03b068efc48267fbcd5a295514077c52b901

$ bundle exec ruby test/acceptance/setting_expectation_on_private_method_test.rb 
Run options: --seed 8006

# Running tests:

E

Finished tests in 0.002969s, 336.8137 tests/s, 0.0000 assertions/s.

  1) Error:
SettingExpectationOnPrivateMethodTest#test_setting_expectation_on_private_method_inherited_from_kernel:
NoMethodError: private method `warn' called for #<#<Class:0x007fac48952850>:0x007fac48952800>
    test/acceptance/setting_expectation_on_private_method_test.rb:19:in `test_setting_expectation_on_private_method_inherited_from_kernel'
    <snipped>

1 tests, 0 assertions, 0 failures, 1 errors, 0 skips

Running the test against e87c03b068efc48267fbcd5a295514077c52b901~:

$ git co e87c03b068efc48267fbcd5a295514077c52b901~

$ bundle exec ruby test/acceptance/setting_expectation_on_private_method_test.rb 
Run options: --seed 35086

# Running tests:

.

Finished tests in 0.001977s, 505.8169 tests/s, 505.8169 assertions/s.

1 tests, 1 assertions, 0 failures, 0 errors, 0 skips
Member

chrisroos commented Aug 26, 2016

I think the change in behaviour was introduced in e87c03b.

Running the test against e87c03b068efc48267fbcd5a295514077c52b901:

$ git co e87c03b068efc48267fbcd5a295514077c52b901

$ bundle exec ruby test/acceptance/setting_expectation_on_private_method_test.rb 
Run options: --seed 8006

# Running tests:

E

Finished tests in 0.002969s, 336.8137 tests/s, 0.0000 assertions/s.

  1) Error:
SettingExpectationOnPrivateMethodTest#test_setting_expectation_on_private_method_inherited_from_kernel:
NoMethodError: private method `warn' called for #<#<Class:0x007fac48952850>:0x007fac48952800>
    test/acceptance/setting_expectation_on_private_method_test.rb:19:in `test_setting_expectation_on_private_method_inherited_from_kernel'
    <snipped>

1 tests, 0 assertions, 0 failures, 1 errors, 0 skips

Running the test against e87c03b068efc48267fbcd5a295514077c52b901~:

$ git co e87c03b068efc48267fbcd5a295514077c52b901~

$ bundle exec ruby test/acceptance/setting_expectation_on_private_method_test.rb 
Run options: --seed 35086

# Running tests:

.

Finished tests in 0.001977s, 505.8169 tests/s, 505.8169 assertions/s.

1 tests, 1 assertions, 0 failures, 0 errors, 0 skips
@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Aug 26, 2016

Contributor

I think instance.warn should fail since it is calling a private method ... but not that big of a deal ...

I'd recommend to also add a test for the method_missing + private method madness just in case ;)

Contributor

grosser commented Aug 26, 2016

I think instance.warn should fail since it is calling a private method ... but not that big of a deal ...

I'd recommend to also add a test for the method_missing + private method madness just in case ;)

chrisroos added a commit that referenced this issue Aug 30, 2016

WIP: Add failing tests for setting expectations on private methods
This test demonstrates the problem described in Mocha issue #260.
@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Aug 30, 2016

Member

Thanks @grosser. I've created a branch with the failing tests in PR #261. I'll take a look at it soon(ish).

Member

chrisroos commented Aug 30, 2016

Thanks @grosser. I've created a branch with the failing tests in PR #261. I'll take a look at it soon(ish).

@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Sep 8, 2016

Member

Hey @grosser.

@floehopper and I spent some time investigating this change in behaviour yesterday. We think the new behaviour is correct and we're not currently planning to revert it.

The new behaviour mirrors Ruby's behaviour in that method_missing is used if you try to call a private method:

class Foo
  def method_missing(*args)
    "#method_missing(#{args})"
  end

  private
  def my_private_method
    '#my_private_method'
  end
end

puts Foo.new.my_private_method
#=> #method_missing([:my_private_method])

puts Foo.new.send(:my_private_method)
#=> #my_private_method

Your example of stubbing a private method and then calling that directly in the test previously worked because the visibility of the stubbed method was incorrectly set to public. Commit e87c03b changed this so that the stubbed method respects the visibility of the original method.

Are you able to change your tests so that they use .send(:warn) instead of .warn?

Member

chrisroos commented Sep 8, 2016

Hey @grosser.

@floehopper and I spent some time investigating this change in behaviour yesterday. We think the new behaviour is correct and we're not currently planning to revert it.

The new behaviour mirrors Ruby's behaviour in that method_missing is used if you try to call a private method:

class Foo
  def method_missing(*args)
    "#method_missing(#{args})"
  end

  private
  def my_private_method
    '#my_private_method'
  end
end

puts Foo.new.my_private_method
#=> #method_missing([:my_private_method])

puts Foo.new.send(:my_private_method)
#=> #my_private_method

Your example of stubbing a private method and then calling that directly in the test previously worked because the visibility of the stubbed method was incorrectly set to public. Commit e87c03b changed this so that the stubbed method respects the visibility of the original method.

Are you able to change your tests so that they use .send(:warn) instead of .warn?

@chrisroos chrisroos added the wont-fix label Sep 8, 2016

@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Sep 8, 2016

Member

Commit 5f768de adds tests to help us avoid breaking this new behaviour in future.

Member

chrisroos commented Sep 8, 2016

Commit 5f768de adds tests to help us avoid breaking this new behaviour in future.

@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Sep 8, 2016

Member

I've added the 'wont-fix' label and am going to close this issue.

@grosser: Feel free to re-open this if you disagree with our decision.

Member

chrisroos commented Sep 8, 2016

I've added the 'wont-fix' label and am going to close this issue.

@grosser: Feel free to re-open this if you disagree with our decision.

@chrisroos chrisroos closed this Sep 8, 2016

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Sep 8, 2016

Contributor

that makes sense ... stubbing method_missing should do the trick ...
ideally the stub would work too since eventually warn got called, but no big deal :)

Contributor

grosser commented Sep 8, 2016

that makes sense ... stubbing method_missing should do the trick ...
ideally the stub would work too since eventually warn got called, but no big deal :)

@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Sep 9, 2016

Member

What do you mean by, "ideally the stub would work too since eventually warn got called", @grosser? I wonder if I've misunderstood something about your original code.

Member

chrisroos commented Sep 9, 2016

What do you mean by, "ideally the stub would work too since eventually warn got called", @grosser? I wonder if I've misunderstood something about your original code.

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Sep 9, 2016

Contributor

ah no ... I got that wrong ... warn never got called ... so the stub should have failed :)

Contributor

grosser commented Sep 9, 2016

ah no ... I got that wrong ... warn never got called ... so the stub should have failed :)

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