Skip to content

Commit

Permalink
BUGFIX: UserStat spec was over ambitious with its mocking
Browse files Browse the repository at this point in the history
  • Loading branch information
SamSaffron committed Jan 6, 2014
1 parent b703d8c commit 6befdce
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 7 deletions.
20 changes: 16 additions & 4 deletions app/models/user_stat.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,27 @@ def update_topic_reply_count
MAX_TIME_READ_DIFF = 100
# attempt to add total read time to user based on previous time this was called
def update_time_read!
last_seen_key = "user-last-seen:#{id}"
last_seen = $redis.get(last_seen_key)
if last_seen.present?
if last_seen = last_seen_cached
diff = (Time.now.to_f - last_seen.to_f).round
if diff > 0 && diff < MAX_TIME_READ_DIFF
UserStat.where(user_id: id, time_read: time_read).update_all ["time_read = time_read + ?", diff]
end
end
$redis.set(last_seen_key, Time.now.to_f)
cache_last_seen(Time.now.to_f)
end

private

def last_seen_key
@last_seen_key ||= "user-last-seen:#{id}"

This comment has been minimized.

Copy link
@joakimk

joakimk Jan 6, 2014

A bit of a premature optimization here? Not very expensive to make a string.

end

def last_seen_cached
$redis.get(last_seen_key)
end

def cache_last_seen(val)
$redis.set(last_seen_key, val)
end

end
Expand Down
6 changes: 3 additions & 3 deletions spec/models/user_stat_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,22 +77,22 @@
let(:stat) { user.user_stat }

it 'makes no changes if nothing is cached' do
$redis.expects(:get).with("user-last-seen:#{user.id}").returns(nil)
stat.expects(:last_seen_cached).returns(nil)
stat.update_time_read!
stat.reload
stat.time_read.should == 0
end

it 'makes a change if time read is below threshold' do
$redis.expects(:get).with("user-last-seen:#{user.id}").returns(Time.now - 10.0)
stat.expects(:last_seen_cached).returns(Time.now - 10)
stat.update_time_read!
stat.reload
stat.time_read.should == 10
end

it 'makes no change if time read is above threshold' do
t = Time.now - 1 - UserStat::MAX_TIME_READ_DIFF
$redis.expects(:get).with("user-last-seen:#{user.id}").returns(t)
stat.expects(:last_seen_cached).returns(t)
stat.update_time_read!
stat.reload
stat.time_read.should == 0
Expand Down

1 comment on commit 6befdce

@dball
Copy link

@dball dball commented on 6befdce Jan 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change moves your mock from the service layer to a private method in the unit under test. That's not usually recommended. The former specifies the contract with redis when various events occur, which is what you ultimately care about.

I suppose it's possible you're really looking for a cache abstraction layer that's implemented by redis in production, but by an in-memory or mock object in your tests.

Please sign in to comment.