Illustrate bug with current ancestor approach #258

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@marcandre
Contributor

marcandre commented Mar 30, 2013

Here is a test that fails.
I didn't check extensively how to fix it. I'm not convinced the current approach is the best one. Has it been attempted to use super from the mocked method, for example if @rspec_call_original is set?

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Mar 30, 2013

Contributor

Note: Not a problem in Ruby 2.1 because of the consistent ancestors list (assuming #257 is merged).

Contributor

marcandre commented Mar 30, 2013

Note: Not a problem in Ruby 2.1 because of the consistent ancestors list (assuming #257 is merged).

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Mar 30, 2013

Member

Thanks, I'll have to dig in to it.

Member

myronmarston commented Mar 30, 2013

Thanks, I'll have to dig in to it.

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Mar 30, 2013

Contributor

BTW, the reason why this fails is that the current handling goes through the ancestors list first (but Ruby 2.0- hides the singleton classes), then checks the superclass if there is one (because it is hidden from the ancestors list).

The test is an example where the superclass actually comes before the module found in ancestors. You can't check the superclass first either, because in other cases the first definition would be in an included module...

Contributor

marcandre commented Mar 30, 2013

BTW, the reason why this fails is that the current handling goes through the ancestors list first (but Ruby 2.0- hides the singleton classes), then checks the superclass if there is one (because it is hidden from the ancestors list).

The test is an example where the superclass actually comes before the module found in ancestors. You can't check the superclass first either, because in other cases the first definition would be in an included module...

myronmarston added a commit that referenced this pull request Apr 2, 2013

@myronmarston myronmarston referenced this pull request Apr 2, 2013

Merged

Candidate fix for #258 #264

myronmarston added a commit that referenced this pull request Jun 25, 2013

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Aug 9, 2013

Member

Did we fix this?

Member

JonRowe commented Aug 9, 2013

Did we fix this?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Aug 9, 2013

Member

I still need to wrap up the candidate fix. Thanks for reminding me. Too much to do, too little time....

Member

myronmarston commented Aug 9, 2013

I still need to wrap up the candidate fix. Thanks for reminding me. Too much to do, too little time....

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Aug 9, 2013

Member

Just noticed the issue and noted the 2-14 milestone ;)

On Friday, August 9, 2013, Myron Marston wrote:

I still need to wrap up the candidate fix. Thanks for reminding me. Too
much to do, too little time....


Reply to this email directly or view it on GitHubhttps://github.com/rspec/rspec-mocks/pull/258#issuecomment-22400473
.

Member

JonRowe commented Aug 9, 2013

Just noticed the issue and noted the 2-14 milestone ;)

On Friday, August 9, 2013, Myron Marston wrote:

I still need to wrap up the candidate fix. Thanks for reminding me. Too
much to do, too little time....


Reply to this email directly or view it on GitHubhttps://github.com/rspec/rspec-mocks/pull/258#issuecomment-22400473
.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Aug 9, 2013

Member

Milestone has been cleared :).

We may consider backporting this to a 2.14.x release, though, but I'll wait to decide that.

Member

myronmarston commented Aug 9, 2013

Milestone has been cleared :).

We may consider backporting this to a 2.14.x release, though, but I'll wait to decide that.

myronmarston added a commit that referenced this pull request Aug 15, 2013

myronmarston added a commit that referenced this pull request Aug 15, 2013

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Aug 15, 2013

Member

I just merged a fix for this in #264 that also greatly simplifies the logic. The fix will be in RSpec 3.

Thanks for reporting this and the clear failure example, @marcandre!

Member

myronmarston commented Aug 15, 2013

I just merged a fix for this in #264 that also greatly simplifies the logic. The fix will be in RSpec 3.

Thanks for reporting this and the clear failure example, @marcandre!

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