Asp Forum - Wrapped method causing infinite recursion in rcov

Pedro Côrte-Real

7/27/2006 2:50:00 PM

I added this to my rails test helper to check for valid HTML automatically:

# Wrap around get to check the markup on :success requests.
alias_method :old_get, :get
def get(*args)
old_get(*args)
if @response.success?
assert_valid_markup
end
end

This works fine with my tests. When running rcov though it creates
infinite recursion in the call to old_get. Has anyone tried anything
of the sort? Is it a bug in rcov?

Pedro.

8 Answers

Caleb Clausen

7/27/2006 4:58:00 PM

On 7/27/06, Pedro Côrte-Real <pedro@pedrocr.net> wrote:
> # Wrap around get to check the markup on :success requests.
> alias_method :old_get, :get
> def get(*args)
> old_get(*args)
> if @response.success?
> assert_valid_markup
> end
> end
>
> This works fine with my tests. When running rcov though it creates
> infinite recursion in the call to old_get. Has anyone tried anything
> of the sort? Is it a bug in rcov?

At a guess, (something is) rcov is also wrapping #get and using the
same name for the old version of the method. At least, that's always
been the explanation when I've encountered these 'wrapping a method
causes infinite recursion' problems. Try this way instead, it should
eliminate any possibility that your code is participating in the
recursion.

old_get=instance_method(:get)
define_method(:get) do |*args|
old_get[*args]
if @response.success?
assert_valid_markup
end
end

This trick was originated by Mauricio Fernandez, as far as I know.

Pedro Côrte-Real

7/27/2006 5:20:00 PM

On 7/27/06, Caleb Clausen <vikkous@gmail.com> wrote:
> At a guess, (something is) rcov is also wrapping #get and using the
> same name for the old version of the method. At least, that's always
> been the explanation when I've encountered these 'wrapping a method
> causes infinite recursion' problems. Try this way instead, it should
> eliminate any possibility that your code is participating in the
> recursion.

I tried a different name and it didn't work so I don't know what's
actually happening. But I'd like to know. Anyone?

> old_get=instance_method(:get)
> define_method(:get) do |*args|
> old_get[*args]

I had to replace this line with:

old_get.bind(self).call(*args)

apparently old_get is an UnboundMethod and needs to be bound to an
object before it can work.

> if @response.success?
> assert_valid_markup
> end
> end

Thanks for this solution. It's working again now. This probably means
that there should be an alias_method equivalent to do this. I didn't
really want to change the method name, I wanted to replace it with
something that could still call the old behavior. 0-step inheritance
so to speak.

> This trick was originated by Mauricio Fernandez, as far as I know.

Thanks to him too then,

Pedro.

Caleb Clausen

7/27/2006 6:07:00 PM

On 7/27/06, Pedro Côrte-Real <pedro@pedrocr.net> wrote:
> On 7/27/06, Caleb Clausen <vikkous@gmail.com> wrote:
> > At a guess, (something is) rcov is also wrapping #get and using the
> > same name for the old version of the method. At least, that's always
> > been the explanation when I've encountered these 'wrapping a method
> > causes infinite recursion' problems. Try this way instead, it should
> > eliminate any possibility that your code is participating in the
> > recursion.
>
> I tried a different name and it didn't work so I don't know what's
> actually happening. But I'd like to know. Anyone?

That's odd, considering that the define_method trick seems to be
working for you. As I said, my diagnosis was just a guess.

> > old_get=instance_method(:get)
> > define_method(:get) do |*args|
> > old_get[*args]
>
> I had to replace this line with:
>
> old_get.bind(self).call(*args)

Ooops! My apologies for posting broken code. Looks like you got the
idea, anyway.

> Thanks for this solution. It's working again now. This probably means
> that there should be an alias_method equivalent to do this. I didn't
> really want to change the method name, I wanted to replace it with
> something that could still call the old behavior. 0-step inheritance
> so to speak.

Wrapping methods seems to be pretty common in ruby. FYI the
define_method trick won't be able to wrap methods that take a block
until ruby 1.9.

> > This trick was originated by Mauricio Fernandez, as far as I know.
>
> Thanks to him too then,

Just a BTW, he's the same guy that wrote rcov. So now you have 2
things to thank him for.

Mauricio Fernández

7/27/2006 7:55:00 PM

On Fri, Jul 28, 2006 at 01:57:56AM +0900, Caleb Clausen wrote:
> On 7/27/06, Pedro Côrte-Real <pedro@pedrocr.net> wrote:
> > # Wrap around get to check the markup on :success requests.
> > alias_method :old_get, :get
> > def get(*args)
> > old_get(*args)
> > if @response.success?
> > assert_valid_markup
> > end
> > end
> >
> >This works fine with my tests. When running rcov though it creates
> >infinite recursion in the call to old_get. Has anyone tried anything
> >of the sort? Is it a bug in rcov?
>
> At a guess, (something is) rcov is also wrapping #get and using the
> same name for the old version of the method. At least, that's always
> been the explanation when I've encountered these 'wrapping a method
> causes infinite recursion' problems. Try this way instead, it should
> eliminate any possibility that your code is participating in the
> recursion.

rcov doesn't redefine anything, it just uses a trace_func (or the C equivalent
when rcovrt is available). In theory, running a program through rcov shouldn't
change its behavior, but this is the second time I'm told there's some sort of
bothersome interference with Rails (I haven't yet been able to fix the first
issue, reported by Assaph Mehr).

I'd love to solve this, but I first need to reproduce the bug; a naïve attempt
fails, as expected:

$ rcov --text-counts foo.rb
1
================================================================================
foo.rb
================================================================================
| 0
class Foo; def foo; end end | 5
class Bar < Foo | 2
alias_method :old_foo, :foo | 1
def foo; p 1; old_foo end | 5
end | 0
| 0
Bar.new.foo | 1

If you cannot show me your code, please keep a copy of the codebase exhibiting
the bug. I might send you a modified rcov for you to test, if I can figure
this out.

Actually, I think I know what is going on: when running under rcov, the file
holding your Rails test helper is being require()d twice. If I'm right, it
should be possible to infer what is going on by tracing #require, with e.g.

module Kernel
req = instance_method(:require)
define_method(:require) do |*a|
puts "-" * 80
puts "Kernel#require " + a.inspect
puts caller.inspect
req.bind(self).call(*a)
end
end

and then looking for the calls where your test file was loaded. Things should
be easy until that stage. After that, we'd have to see why rcov introduces a
difference in the second call.

[Thinking as I write] rcov loads the files supplied in the command line with
Kernel#load, so if you later #require' any of them, it would be loaded twice.
Moreover, Rake's default test loader also works this way; here's its source
code:

ARGV.each { |f| load f unless f =~ /^-/ }

If this hunch is correct, the following patch to bin/rcov could save the day:

diff -rN -u old-tmp/bin/rcov new-tmp/bin/rcov
--- old-tmp/bin/rcov 2006-07-27 21:50:33.000000000 +0200
+++ new-tmp/bin/rcov 2006-07-27 21:50:33.000000000 +0200
@@ -450,7 +450,7 @@
if options.replace_prog_name
$0 = File.basename(File.expand_path(prog))
end
- load prog
+ require prog
end

Rake would also have to be changed, but I'd like to make sure the modification
is appropriate before applying it to rcov and sending jweirich a patch.

> old_get=instance_method(:get)
> define_method(:get) do |*args|
> old_get[*args]
> if @response.success?
> assert_valid_markup
> end
> end
>
> This trick was originated by Mauricio Fernandez, as far as I know.

It was most probably discovered by some anonymous Japanese Rubyist before I
popularized it to some extent, though :)

--
Mauricio Fernandez - http://eige... - singular Ruby

Erik Veenstra

7/27/2006 8:53:00 PM

I don't like advertising my own work, but...

> old_get=instance_method(:get)
> define_method(:get) do |*args|
> old_get[*args]

This works, unless you want the method to receive a &block.
(Though that's fixed in Ruby 1.9.)

I would do this:

class Module
def assert_valid_markup(method_name) do
post_condition(method_name) do |obj, method_name, args, block|
# Check it...
end
end
end

class Foo
def get(*args)
# The real stuff...
end

assert_valid_markup :get if $DEBUG
end

I call this assert_valid_markup a "monitor-function" [1]. This
post_condition is defined on the site as well.

gegroet,
Erik V. - http://www.erikve...

[1] http://www.erikve...monitorfunctions/index.html

Mauricio Fernández

7/27/2006 9:38:00 PM

On Fri, Jul 28, 2006 at 04:55:15AM +0900, Mauricio Fernandez wrote:
> On Fri, Jul 28, 2006 at 01:57:56AM +0900, Caleb Clausen wrote:
> > On 7/27/06, Pedro Côrte-Real <pedro@pedrocr.net> wrote:
> rcov doesn't redefine anything, it just uses a trace_func (or the C
> equivalent when rcovrt is available). In theory, running a program through
> rcov shouldn't change its behavior, but this is the second time I'm told
> there's some sort of bothersome interference with Rails (I haven't yet been
> able to fix the first issue, reported by Assaph Mehr).
========================================
Meanwhile, I've figured this one out. Rails was interfering with itself: rake
test runs unit, functional and integration tests in three separate processes.
If you load them all simultaneously, you can run into puzzling errors
(reported for the wrong controllers, as
SomeController.new.class != SomeController in some circumstances (!)).

So we're down to one bug/strange interaction. Let's address it:

if you are defining the coverage task like this:

Rcov::RcovTask.new(:converage) do |t|
t.libs << "test"
t.test_files = FileList['test/**/*_test.rb']
t.output_dir = 'test/coverage'
t.verbose = true
t.rcov_opts << '--rails'
end

the helper could get loaded twice if it is being required in both the unit
_and_ the functional tests (Rails does ugly things with $: and require). We
can get some evidence to support that theory by monitoring #require, as shown
in my previous message.

At any rate, I will soon implement the ability to merge coverage data from
several runs, allowing you to run the tests separately and obtain a single
coverage figure.

--
Mauricio Fernandez - http://eige... - singular Ruby

Pedro Côrte-Real

7/28/2006 11:06:00 AM

On 7/27/06, Mauricio Fernandez <mfp@acm.org> wrote:
> If you cannot show me your code, please keep a copy of the codebase exhibiting
> the bug. I might send you a modified rcov for you to test, if I can figure
> this out.

I can send you the code if you want but you've pretty much seen it
all. assert_valid_markup is a plugin:

http://wiki.rubyonrails.com/rails/pages/Assert+Valid+Mar...

I just wrapped the call to that assertion so that on every successful
request the HTML is tested. Other than that it's just normal tests.

Just say so if you still can't reproduce it.

> Actually, I think I know what is going on: when running under rcov, the file
> holding your Rails test helper is being require()d twice. If I'm right, it
> should be possible to infer what is going on by tracing #require, with e.g.
>
> module Kernel
> req = instance_method(:require)
> define_method(:require) do |*a|
> puts "-" * 80
> puts "Kernel#require " + a.inspect
> puts caller.inspect
> req.bind(self).call(*a)
> end
> end
>
> and then looking for the calls where your test file was loaded. Things should
> be easy until that stage. After that, we'd have to see why rcov introduces a
> difference in the second call.
>
> [Thinking as I write] rcov loads the files supplied in the command line with
> Kernel#load, so if you later #require' any of them, it would be loaded twice.
> Moreover, Rake's default test loader also works this way; here's its source
> code:
>
> ARGV.each { |f| load f unless f =~ /^-/ }
>
> If this hunch is correct, the following patch to bin/rcov could save the day:
>
>
> diff -rN -u old-tmp/bin/rcov new-tmp/bin/rcov
> --- old-tmp/bin/rcov 2006-07-27 21:50:33.000000000 +0200
> +++ new-tmp/bin/rcov 2006-07-27 21:50:33.000000000 +0200
> @@ -450,7 +450,7 @@
> if options.replace_prog_name
> $0 = File.basename(File.expand_path(prog))
> end
> - load prog
> + require prog
> end

No, doesn't work :(

> It was most probably discovered by some anonymous Japanese Rubyist before I
> popularized it to some extent, though :)

Thanks anyway.

Pedro.

Mauricio Fernández

8/7/2006 5:39:00 PM

On Fri, Jul 28, 2006 at 08:05:37PM +0900, Pedro Côrte-Real wrote:
> On 7/27/06, Mauricio Fernandez <mfp@acm.org> wrote:
> >If you cannot show me your code, please keep a copy of the codebase
> >exhibiting
> >the bug. I might send you a modified rcov for you to test, if I can figure
> >this out.
>
> I can send you the code if you want but you've pretty much seen it
> all. assert_valid_markup is a plugin:
>
> http://wiki.rubyonrails.com/rails/pages/Assert+Valid+Mar...
>
> I just wrapped the call to that assertion so that on every successful
> request the HTML is tested. Other than that it's just normal tests.
>
> Just say so if you still can't reproduce it.

Could you try to reproduce it with rcov 0.7.0, running the unit, functional &
integration tests separately as explained in [ruby-talk:206760]? I think the
file was being require()d with 2 different names, in tests that break unless
executed in different processes.

--
Mauricio Fernandez - http://eige... - singular Ruby

comp.lang.ruby

Wrapped method causing infinite recursion in rcov

Pedro Côrte-Real

Caleb Clausen

Pedro Côrte-Real

Caleb Clausen

Mauricio Fernández

Erik Veenstra

Mauricio Fernández

Pedro Côrte-Real

Mauricio Fernández

x Login to ForumsZone