Wrapped method causing infinite recursion in rcov

[Pedro Côrte-Real]

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 ld_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.

 

[Caleb Clausen]

# Wrap around get to check the markup on :success requests.
alias_method ld_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=3Dinstance_methodget)
define_methodget) 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]

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_methodget)
define_methodget) 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]

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=3Dinstance_methodget)
define_methodget) 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.

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 Fernandez]

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 ld_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_methodrequire)
define_methodrequire) 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_methodget)
define_methodget) 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

 

[Erik Veenstra]

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

old_get=instance_methodget)
define_methodget) 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.erikveen.dds.nl/

[1] http://www.erikveen.dds.nl/monitorfunctions/index.html

 

[Mauricio Fernandez]

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.newconverage) 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.

 

[Pedro Côrte-Real]

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+Markup+Plugin

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_methodrequire)
define_methodrequire) 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 Fernandez]

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+Markup+Plugin

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.