One test affects another

I have the following class:

class Brewer < ActiveRecord::Base
  class EmailNotFound < StandardError; end
  class << self
    def find_by_email(*args)
      b = super(*args)
      raise EmailNotFound if b.nil?
      return b
    end
  end
end

And the following two tests:

require File.dirname(__FILE__) + '/../test_helper'

class BrewerTest < Test::Unit::TestCase
  fixtures :brewers

  def test_should_find_brewer_with_valid_email
    assert_equal brewers(:active_brewer),
Brewer.find_by_email(brewers(:active_brewer).email)
  end

  def test_should_raise_email_not_found_exception_with_invalid_email
    assert_raises Brewer::EmailNotFound do
      Brewer.find_by_email('NotAnEmail')
    end
  end
end

The tests fail like this:

Loaded suite test/unit/brewer_test
Started
.F
Finished in 0.428367 seconds.

  1) Failure:
test_should_raise_email_not_found_exception_with_invalid_email(BrewerTest)
[test/unit/brewer_test.rb:207]:
<Brewer::EmailNotFound> exception expected but none was thrown.

2 tests, 2 assertions, 1 failures, 0 errors

If I comment out test_should_find_brewer_with_valid_email, the other
test passes.

What's going on here?

Regards,
--Dean

Hi --

I have the following class:

class Brewer < ActiveRecord::Base
class EmailNotFound < StandardError; end
class << self
   def find_by_email(*args)
     b = super(*args)
     raise EmailNotFound if b.nil?
     return b
   end
end
end

And the following two tests:

require File.dirname(__FILE__) + '/../test_helper'

class BrewerTest < Test::Unit::TestCase
fixtures :brewers

def test_should_find_brewer_with_valid_email
   assert_equal brewers(:active_brewer),
Brewer.find_by_email(brewers(:active_brewer).email)
end

def test_should_raise_email_not_found_exception_with_invalid_email
   assert_raises Brewer::EmailNotFound do
     Brewer.find_by_email('NotAnEmail')
   end
end
end

The tests fail like this:

Loaded suite test/unit/brewer_test
Started
.F
Finished in 0.428367 seconds.

1) Failure:
test_should_raise_email_not_found_exception_with_invalid_email(BrewerTest)
[test/unit/brewer_test.rb:207]:
<Brewer::EmailNotFound> exception expected but none was thrown.

2 tests, 2 assertions, 1 failures, 0 errors

If I comment out test_should_find_brewer_with_valid_email, the other
test passes.

What's going on here?

What I think is happening is this:

When you call super on #find_by_email, ActiveRecord goes ahead and
creates its own #find_by_email dynamically. That dynamically-created
method then takes over: when the second test runs, it uses that
version, not the one you wrote.

Try changing your find_by_email to, say, get_by_email, and change
super to find_by_email. That may not be a permanent fix but it should
show what's going on.

David

Dean wrote:

I have the following class:

class Brewer < ActiveRecord::Base
  class EmailNotFound < StandardError; end
  class << self
    def find_by_email(*args)
      b = super(*args)

I set up an experiment like yours, and couldn't get the equivalent of 'find_by_email' to fail. The first test case found its model, and the second one took an invalid ID and raised an error.

Architecturally, I'm not sure why you'd raise, over letting 'find_by_email' return 'nil', and detecting this. Maybe you intend to use the Samurai Principle - either return victorious or don't return at all.

But I'm too superstitious to want to call 'super' inside a method that's generated automatically via 'method_missing'. Because your find_by disregards the standard ActiveRecord contract for other find_by methods, why not rename it 'fetch_by_email', then put the 'find_by_email' and the 'if' inside it.

So if you had a dog named Samurai, and you said "fetch", then...

David A. Black wrote:

When you call super on #find_by_email, ActiveRecord goes ahead and
creates its own #find_by_email dynamically. That dynamically-created
method then takes over: when the second test runs, it uses that
version, not the one you wrote.

I thought about that, but the source to method_missing does not "create a method" - but that's possible. It just builds the complete find() setup for you and then calls this.

So where would a super route out of such a concoction?

Try changing your find_by_email to, say, get_by_email, and change
super to find_by_email. That may not be a permanent fix but it should
show what's going on.

In terms of style it's a permanent fix, because things with the same name should have the same contract!

Dean wrote:
> I have the following class:

> class Brewer < ActiveRecord::Base
> class EmailNotFound < StandardError; end
> class << self
> def find_by_email(*args)
> b = super(*args)

I set up an experiment like yours, and couldn't get the equivalent of
'find_by_email' to fail. The first test case found its model, and the second one
took an invalid ID and raised an error.

Architecturally, I'm not sure why you'd raise, over letting 'find_by_email'
return 'nil', and detecting this. Maybe you intend to use the Samurai Principle
- either return victorious or don't return at all.

It's for concise controllers:

8 def activate
9 self.current_brewer = Brewer.activate_with_activation_code
params[:activation_code]
10 flash[:notice] = "Signup complete!"
11 redirect_to brewer_path(current_brewer)
12 rescue Brewer::ActivationCodeNotFound
13 flash[:error] = "Could not find that activation code"
14 redirect_to(activate_path)
15 end

Rather than using an if brewer.nil? branch.

But I'm too superstitious to want to call 'super' inside a method that's
generated automatically via 'method_missing'. Because your find_by disregards
the standard ActiveRecord contract for other find_by methods, why not rename it
'fetch_by_email', then put the 'find_by_email' and the 'if' inside it.

I had forgot that find_by_email was created by method_missing. I've
discarded the idea for an if; then; else thing.

So if you had a dog named Samurai, and you said "fetch", then...

(-:

Thanks for the replies everyone.
--Dean

Hi --

David A. Black wrote:

When you call super on #find_by_email, ActiveRecord goes ahead and
creates its own #find_by_email dynamically. That dynamically-created
method then takes over: when the second test runs, it uses that
version, not the one you wrote.

I thought about that, but the source to method_missing does not "create a
method" - but that's possible. It just builds the complete find() setup for you
and then calls this.

method_missing does create the method. Here's a relevant excerpt:

   def method_missing(method_id, *arguments)
      # ...
             self.class_eval %{
               def self.#{method_id}(*args)

And a little demo:

Loading development environment (Rails 2.0.2)

Auction.method(:find_by_title)

NameError: undefined method `find_by_title' for class `Class'

Auction.find_by_title("blah")

=> nil

Auction.method(:find_by_title)

=> #<Method: Auction(id: integer, title: string, start_date: datetime,
    ...)

And the RDoc comments in base.rb:

         # Each dynamic finder or initializer/creator is also defined
         # in the class after it is first invoked, so that future
         # attempts to use it do not run through method_missing.

So I'm going to stick with my original interpretation of the OP's
result :slight_smile:

David

And a little demo:

Loading development environment (Rails 2.0.2)

Auction.method(:find_by_title)

NameError: undefined method `find_by_title' for class `Class'

Auction.find_by_title("blah")

=> nil

Auction.method(:find_by_title)

=> #<Method: Auction(id: integer, title: string, start_date: datetime,
   ...)

And the RDoc comments in base.rb:

        # Each dynamic finder or initializer/creator is also defined
        # in the class after it is first invoked, so that future
        # attempts to use it do not run through method_missing.

So I'm going to stick with my original interpretation of the OP's
result :slight_smile:

This was new in rails 2.0 - 1.x always hit method_missing

Fred

Hi --

David A. Black wrote:

Try changing your find_by_email to, say, get_by_email, and change
super to find_by_email. That may not be a permanent fix but it should
show what's going on.

In terms of style it's a permanent fix, because things with the same name should
have the same contract!

Maybe, though I don't like the idea of having get_ and find_ as
markers of difference, since they're sort of arbitrary. It's the same
problem as instance_eval and instance_exec; they indicate slightly
different methods, but there's nothing in the method names that tells
you which is which.

A better choice might be find_by_email! (the "dangerous" twin of
find_by_email).

David

Hi --

A better choice might be find_by_email! (the "dangerous" twin of
find_by_email).

D'oh!