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

Fix alias method from included module on 2.1 #244

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@grosser
Contributor

grosser commented Jun 21, 2016

replacement PR for #202

since I just ran into this again and the original has Unknown repository ...

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Jun 24, 2016

Member

@grosser: Thanks for (re-)submitting this PR. I'm investigating the CI build failures.

Member

floehopper commented Jun 24, 2016

@grosser: Thanks for (re-)submitting this PR. I'm investigating the CI build failures.

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Jun 24, 2016

Member

Having now fixed the Travis CI build on master, I've created my own version of the PR branch and pushed it to GitHub. This has kicked off a new build for the branch.

Member

floehopper commented Jun 24, 2016

Having now fixed the Travis CI build on master, I've created my own version of the PR branch and pushed it to GitHub. This has kicked off a new build for the branch.

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Jun 24, 2016

Member

I'm seeing a single test failure when running under Ruby v1.8.7 and using the built-in version of test-unit. I need to understand more about the change itself before accepting this PR.

Member

floehopper commented Jun 24, 2016

I'm seeing a single test failure when running under Ruby v1.8.7 and using the built-in version of test-unit. I need to understand more about the change itself before accepting this PR.

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Jun 24, 2016

Member

At the risk of repeating what I said in a comment on the original PR:

  • Can you explain how the scenario in your acceptance test is different from this other existing acceptance test?
  • Does method aliasing somehow work differently in Ruby >= v2.0?
  • Does method aliasing work differently when done via extending a module?
  • Can you explain why Mocha currently does the wrong thing in the scenario you're trying to fix?
  • Is the acceptance test the simplest scenario which reproduces the problem? - it seems quite convoluted.
  • Can the acceptance test be modified to cover the any_instance & instance method equivalents of this scenario?

I have to admit I'm pretty confused.

Member

floehopper commented Jun 24, 2016

At the risk of repeating what I said in a comment on the original PR:

  • Can you explain how the scenario in your acceptance test is different from this other existing acceptance test?
  • Does method aliasing somehow work differently in Ruby >= v2.0?
  • Does method aliasing work differently when done via extending a module?
  • Can you explain why Mocha currently does the wrong thing in the scenario you're trying to fix?
  • Is the acceptance test the simplest scenario which reproduces the problem? - it seems quite convoluted.
  • Can the acceptance test be modified to cover the any_instance & instance method equivalents of this scenario?

I have to admit I'm pretty confused.

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Jun 24, 2016

Contributor

I'm not really sure either ... I'll have to dig back into this patch :D

On Fri, Jun 24, 2016 at 10:13 AM, James Mead notifications@github.com
wrote:

At the risk of repeating what I said in a comment on the original PR
#202 (comment):

I have to admit I'm pretty confused.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#244 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAAsZzGIcBy-mKLzr76rw2USuFCpGb5oks5qPBA_gaJpZM4I7Qxe
.

Contributor

grosser commented Jun 24, 2016

I'm not really sure either ... I'll have to dig back into this patch :D

On Fri, Jun 24, 2016 at 10:13 AM, James Mead notifications@github.com
wrote:

At the risk of repeating what I said in a comment on the original PR
#202 (comment):

I have to admit I'm pretty confused.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#244 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAAsZzGIcBy-mKLzr76rw2USuFCpGb5oks5qPBA_gaJpZM4I7Qxe
.

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Jun 26, 2016

Contributor

managed to simplify the test case 1 step ... but it only happens when a inherited base method is aliased and then the aliased method is stubbed

was not able to reproduce for instance methods ... so this possibly was added by accident ... removed ...

the difference to the existing test is that it is overwriting a method on the same class and not an inherited method

... I did not dive deeper into why the logic in mocha is wrong / how extending a class is different / how it is different in ruby 2.1 ...

Contributor

grosser commented Jun 26, 2016

managed to simplify the test case 1 step ... but it only happens when a inherited base method is aliased and then the aliased method is stubbed

was not able to reproduce for instance methods ... so this possibly was added by accident ... removed ...

the difference to the existing test is that it is overwriting a method on the same class and not an inherited method

... I did not dive deeper into why the logic in mocha is wrong / how extending a class is different / how it is different in ruby 2.1 ...

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Jun 26, 2016

Contributor

updated to fix ruby 1.8 test case ... good enough ?

Contributor

grosser commented Jun 26, 2016

updated to fix ruby 1.8 test case ... good enough ?

floehopper added a commit that referenced this pull request Aug 17, 2016

Add test for stubbing class method defined on module & aliased
This test was derived from the tests in #244 and was failing before we moved to
always (in Ruby v2+) using the prepended module stubbing mechanism. We'll
probably want to expand it to cover some more scenarios, but it's useful as it
stands.
@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Aug 19, 2016

Member

We haven't forgotten about this, @grosser.

@floehopper and I spent quite a while investigating the problem earlier this week and think/hope we can come up with a solution that works for Ruby 1.8.7 and up.

Member

chrisroos commented Aug 19, 2016

We haven't forgotten about this, @grosser.

@floehopper and I spent quite a while investigating the problem earlier this week and think/hope we can come up with a solution that works for Ruby 1.8.7 and up.

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Aug 19, 2016

Contributor

❤️

Contributor

grosser commented Aug 19, 2016

❤️

chrisroos added a commit that referenced this pull request Aug 25, 2016

Add test for stubbing class method defined on module & aliased
This test was derived from the tests in #244 and was failing before we moved to
always (in Ruby v2+) using the prepended module stubbing mechanism. We'll
probably want to expand it to cover some more scenarios, but it's useful as it
stands.

chrisroos added a commit that referenced this pull request Aug 25, 2016

Add test for stubbing class method defined on module & aliased
This test was derived from the tests in #244 and was failing before we
moved to always (in Ruby v2+) using the prepended module stubbing
mechanism. We'll probably want to expand it to cover some more
scenarios, but it's useful as it stands.

This test fails in Ruby 1.8.x and we can't find an obvious fix.

The test passes in Ruby 1.9.x due to a change in behaviour of `#owner`
when a class is `extend`ed by a module.

The test passes in Rubies > 2.0 because we now always prepend a module
to avoid overwriting the original method at all.
@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Aug 25, 2016

Member

Hi @grosser. @floehopper and I have just pushed some changes to master. Would you be able to see whether the master branch version of Mocha fixes your problem? If it does we'll aim to release a new version shortly.

Member

chrisroos commented Aug 25, 2016

Hi @grosser. @floehopper and I have just pushed some changes to master. Would you be able to see whether the master branch version of Mocha fixes your problem? If it does we'll aim to release a new version shortly.

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Aug 25, 2016

Contributor

fixes the issue ... but it broke something else:

class A
  def method_missing(*args)
    1
  end
end

a = A.new
a.expects(:warn) # a private method inherited from Kernel ...
a.warn

- expected exactly once, not yet invoked: #<A:0x7fcaa9eed3a8>.warn(any_parameters)

... we got the worst code, just so you can find all edge-cases ;)

FYI using send :warn makes it work properly ...

Contributor

grosser commented Aug 25, 2016

fixes the issue ... but it broke something else:

class A
  def method_missing(*args)
    1
  end
end

a = A.new
a.expects(:warn) # a private method inherited from Kernel ...
a.warn

- expected exactly once, not yet invoked: #<A:0x7fcaa9eed3a8>.warn(any_parameters)

... we got the worst code, just so you can find all edge-cases ;)

FYI using send :warn makes it work properly ...

@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Aug 26, 2016

Member

Thanks for the update, @grosser. We'll investigate the new problem.

Member

chrisroos commented Aug 26, 2016

Thanks for the update, @grosser. We'll investigate the new problem.

@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Aug 26, 2016

Member

@grosser:

I've created issue #260 to capture the most recent problem you've described. Can you take a look and see whether you think I've understood correctly, please?

Ignoring the new issue for a moment, are you happy for us to close this PR given the fix we've applied to master?

Member

chrisroos commented Aug 26, 2016

@grosser:

I've created issue #260 to capture the most recent problem you've described. Can you take a look and see whether you think I've understood correctly, please?

Ignoring the new issue for a moment, are you happy for us to close this PR given the fix we've applied to master?

@grosser grosser closed this Aug 26, 2016

@grosser grosser deleted the zendesk:eac/alias_method_fix branch Aug 26, 2016

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Aug 26, 2016

Contributor

Yes, this issue should be gone ... and there is a test in case is comes
back :)

On Fri, Aug 26, 2016 at 2:52 AM, Chris Roos notifications@github.com
wrote:

@grosser https://github.com/grosser:

I've created issue #260 #260
to capture the most recent problem you've described. Can you take a look
and see whether you think I've understood correctly, please?

Ignoring the new issue for a moment, are you happy for us to close this PR
given the fix we've applied to master?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#244 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAsZxDC4vzyvb8qJQR3Evxs8uUrhs3yks5qjrdTgaJpZM4I7Qxe
.

Contributor

grosser commented Aug 26, 2016

Yes, this issue should be gone ... and there is a test in case is comes
back :)

On Fri, Aug 26, 2016 at 2:52 AM, Chris Roos notifications@github.com
wrote:

@grosser https://github.com/grosser:

I've created issue #260 #260
to capture the most recent problem you've described. Can you take a look
and see whether you think I've understood correctly, please?

Ignoring the new issue for a moment, are you happy for us to close this PR
given the fix we've applied to master?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#244 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAsZxDC4vzyvb8qJQR3Evxs8uUrhs3yks5qjrdTgaJpZM4I7Qxe
.

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Sep 12, 2016

Contributor

FYI unrelated but good change: some stub or expect results used to be dupped strings (not sure exactly what edge-case caused that), but now they are no longer dupped :)

Contributor

grosser commented Sep 12, 2016

FYI unrelated but good change: some stub or expect results used to be dupped strings (not sure exactly what edge-case caused that), but now they are no longer dupped :)

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Sep 12, 2016

Contributor

all our project builds are green now ... want to release ?

Contributor

grosser commented Sep 12, 2016

all our project builds are green now ... want to release ?

@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Sep 14, 2016

Member
Member

chrisroos commented Sep 14, 2016

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Oct 10, 2016

Member

@grosser: Mocha v1.2.0 has just been released incorporating these changes. Thanks for your help & patience!

Member

floehopper commented Oct 10, 2016

@grosser: Mocha v1.2.0 has just been released incorporating these changes. Thanks for your help & patience!

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