Testing discovered a problem in my code


#1

When trying to test using sqlite in-memory in ran into a problem:

  • rake test raises an error on a test
  • running the failing test alone works perfectly.

So what’s the problem? here is the method giving the trouble:

def self.expiry_date_for(user)
@expiry_date_cache ||= find_if_expiry_date_for(user)
end

That cached method is also called by another file, and therefore sets
the @expiry_date_cache to some value therefore not acting correctly.

So is my code flawed or is it the testing framework that doesn’t clear
correctly the cached variable? In production, would my code work
correctly?


#2

Your single test may be relying on database data that is set up (and
left
there) by a different test. (In your non-sqlite database, the data may
be
sitting there as the result of previous testing activity, so the single
test
may pass there and not on sqlite)


#3

Matthew K. wrote:

Your single test may be relying on database data that is set up (and
The tests don’t hit the database. Only one previous test hits the same
method and forces the class to set this instance variable.

But I thing that I really have a flaw in my code, as this class instance
variable will be set once by the process and will keep the same value as
long as the process is running. I should probably rewrite it to an
instance variable.


#4

But I thing that I really have a flaw in my code
I confirm, my code had a big flaw.


#5

@expiry_date_cache ||= {}
@expiry_date_cache[user.id] ||= find_if_expiry_date_for(user)

From the beginning my code was silly. The cache has to be tied to an
object that only exists for the current request being processed. So I
have refactored it.

tdd/bdd/rspec/test::unit/whatever helped me to discover this.


#6

I missed that; sorry. When code uses class-cached data for performance,
I’ve developed a testing pattern that explicitly clears class-cached
data as
part of the test data setup. It does require careful attention. I’d be
interested in what others do!


#7

You’re setting an instance variable on a class, which is a global
variable and is not garbage collected. The state you set is maintained
across runs, screwing things up.

If you set config.cache_classes to false in environments/test.rb I
think it ought to reload the classes each time.

There’s no way for rspec to know that you would want to clear that
variable, you have to do that yourself. This is one example of how
global variables suck.

Also your code doesn’t make sense to me. I’d you called it twice, each
with different users, you would get the same result which is prob not
what you want.

Pat


#8

Also, just noticed your class-caching isn’t keyed off user. (I’m also
honestly not sure what @ instead of @@ means inside a self. class
method;
I’d have to look that up and write specs to test it!)

Something like this may work better.

@expiry_date_cache ||= {}
@expiry_date_cache[user.id] ||= find_if_expiry_date_for(user)

(Also Rails 2.something has a memoize feature which does key off
arguments–see the Railscast. Not sure if it works for class methods).

Matt


#9

This is one example of how global variables suck.

Also your code doesn’t make sense to me. I’d you called it twice, each
with different users, you would get the same result which is prob not
what you want.

Yup, that’s why I corrected it. Now the method is an instance method of
User, so I call it as following:

current_user.membership_expiry_date


#10

On Thu, Apr 16, 2009 at 5:46 AM, Matthew K. removed_email_address@domain.invalid
wrote:

Also, just noticed your class-caching isn’t keyed off user. (I’m also
honestly not sure what @ instead of @@ means inside a self. class method;
I’d have to look that up and write specs to test it!)

@var inside a class method is just an instance variable like any
other. There’s one for each object instantiated from its class and no
other object can access it. The difference is that in this case, the
object is a class - i.e., it is instantiated from the Class class.
It’s call a class instance variable.

@@var is a class variable and can be accessed from both the class
itself and objects instantiated from it.

///ark


#11

On Thu, Apr 16, 2009 at 7:33 AM, Pat M. removed_email_address@domain.invalid
wrote:

You’re setting an instance variable on a class, which is a global
variable

I wouldn’t call a class instance variable global. It’s accessible to
only one object, after all.

and is not garbage collected. The state you set is maintained
across runs, screwing things up.

But that’s a good point.

///ark


#12

On Thursday, April 16, 2009, Mark W. removed_email_address@domain.invalid wrote:

On Thu, Apr 16, 2009 at 7:33 AM, Pat M. removed_email_address@domain.invalid wrote:

You’re setting an instance variable on a class, which is a global
variable

I wouldn’t call a class instance variable global. It’s accessible to
only one object, after all.

Neither would I :slight_smile: A class, being referenced by a constant, is a
global variable.

Pat


#13

On Thu, Apr 16, 2009 at 10:23 AM, Pat M. removed_email_address@domain.invalid
wrote:

global variable.
Sorry I misunderstood you - the perils of relative pronouns. :slight_smile:

///ark