Skip to content

Instantly share code, notes, and snippets.

@lagartoflojo
Last active September 26, 2016 11:28
Show Gist options
  • Save lagartoflojo/06965ee2923d564da3f02a0ff2ac70f9 to your computer and use it in GitHub Desktop.
Save lagartoflojo/06965ee2923d564da3f02a0ff2ac70f9 to your computer and use it in GitHub Desktop.
How to get your butt bitten by Rails' try and rspec's allow(x).to receive(:bla)

Why Rails' try() and rspec's allow(x).to receive(:bla) can bite you in the butt:

When actions are taken by superusers, we're supposed to append admin_ to the SystemLog event names. For example: when one of us logs in, there should be a SystemLog with admin_log_in event. When normal users log in, there's a SystemLog with log_in event.

If you check your SystemLog table, you'll see that's not currently true ;) And it hasn't been for quite a while. This is surprising, since all of our SystemLog specs are passing!

Digging deeper, we find self.prefixed_event_name(event_name) in SystemLog, which is supposed to add the admin_ prefix. That method checks if the current_user is an admin with: current_user.try(:admin?). current_user could be nil, which is why we use try here. But guess what: the method User#admin? does not exist! We recently renamed admins to superusers, so the correct call should be current_user.try(:superuser?).

But then, how can our tests be passing??

In system_log_spec.rb we find the culprit: allow(user).to receive(:admin?).and_return(true). That innocent-looking line creates the admin? method on user, so then the prefixed_event_name method correctly adds the admin_ prefix... but ONLY in our tests!

Lessons learned:

  1. Use try! instead of try. try! will actually call the method even if it doesn't exist. We would have caught this error much earlier, since we would have seen NoMethodError: undefined method admin? for #<User:0x00000003b74a78> all over the place.
  2. Don't use allow(x).to receive(:bla) if you don't need it. In this case, making the user an actual superuser with user.superuser! is much shorter and clearer. Also, using allow(x).to receive(:bla) tests the implementation, not the behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment