Real-World Example of Testing and Refactoring - Part 2

This is a mini-series about adding unit tests to existing code that has no or very few tests. We use a real-world example from an open source project to illustrate some useful techniques. In part 1 we created simple tests for the context of not being logged-in to the Userbin service. This post deals with the more interesting case: how should the client behave when we’re logged-in?

Properties with a valid session token

I’m defining how a client with a valid session token behaves: it should be authorized, MFA-enabled etc. Create a double with these properties and inject it into the test subject (the Userbin::Client object):

context 'with a valid session token' do
    before(:each) do
      attributes = {
          mfa_enabled?: true,
          device_trusted?: true,
          mfa_in_progress?: false,
          mfa_required?: true,
          has_default_pairing?: true
      }
      subject.session_token = double('session_token', attributes)
    end

Now we have expectations for our client in this scenario. Just copy them from the not-logged-in context above and modify them accordingly:

it { is_expected.to be_authorized }
it { is_expected.to be_mfa_enabled }
it { is_expected.to be_device_trusted }
it { is_expected.not_to be_mfa_in_progress }
it { is_expected.to be_mfa_required }
it { is_expected.to have_default_pairing }

Running the test suite gives six errors that are all complaining about undefined method 'set_cookie' for nil:NilClass.

1) Userbin::Client with a valid session token
    Failure/Error: subject.session_token = double('session_token', attributes)
    NoMethodError:
    undefined method `set_cookie' for nil:NilClass
    # ./spec/client_spec.rb:50:in `block (3 levels) in <top (required)>'

The offending line is in our setup code: subject.session_token = double('session_token', attributes). Looks like we need to do some more work in order to initialize our objects properly.

Switch on the backtrace to get more information:

rspec ./spec/client_spec.rb:53 --backtrace
Run options: include {:locations=>{"./spec/client_spec.rb"=>[53]}}

Userbin::Client
  with a valid session token
    example at ./spec/client_spec.rb:53 (FAILED - 1)

Failures:

  1) Userbin::Client with a valid session token
     Failure/Error: subject.session_token = double('session_token', attributes)
     NoMethodError:
       undefined method `set_cookie' for nil:NilClass
     # ./lib/userbin/support/cookie_store.rb:16:in `[]='
     # ./lib/userbin/token_store.rb:13:in `session_token='
     # ./lib/userbin/client.rb:44:in `session_token='
     # ./spec/client_spec.rb:50:in `block (3 levels) in <top (required)>'

This lets us unravel the call chain that leads to the error. First the session_token= method in client.rb uses a @store:

# client.rb:43
def session_token=(session_token)
  @store.session_token = session_token
end

And @store is a Userbin::TokenStore that hands our token to its @cookies:

# token_store.rb:12
def session_token=(value)
  @cookies['_ubs'] = value

  if value && value != @cookies['_ubs']
    @cookies['_ubs']
  elsif !value
    @cookies['_ubs'] = nil
  end
end

And @cookies is a Userbin::CookieStore::Base that in turn gives our token to its @request.cookies['_ubs'] where finally the error happens.

# cookie_store.rb:13
def []=(key, value)
    @request.cookies[key] = value
    if value
      # error happens here:
      @response.set_cookie(key, value: value,
                                expires: Time.now + (20 * 365 * 24 * 60 * 60),
                                path: '/')
    else
      @response.delete_cookie(key)
    end
end

We have instantiated the Userbin::Client with a nil value as its response. Now we learn that @response should at least be something that understands a set_cookie message.

We had to go three levels deep to reach the source of the error. The CookieStore and TokenStore are both created in Userbin::Client#initialize` and they use the response argument. Therefore let’s try to give a real Response object to our test subject:

  subject do
    dummy_request = double('request', ip: '1.1.2.3', user_agent: 'IE 1.0', cookies: {})
    @response = Rack::Response.new
    Userbin::Client.new(dummy_request, @response)
  end

Now we’re getting a new error message (this is progress):

Double "session_token" received unexpected message :split with (".")

The essential parts of the backtrace:

 # ./lib/userbin/jwt.rb:10:in `initialize'
 # ./lib/userbin/session_token.rb:7:in `new'
 # ./lib/userbin/session_token.rb:7:in `initialize'
 # ./lib/userbin/token_store.rb:9:in `new'
 # ./lib/userbin/token_store.rb:9:in `session_token'
 # ./lib/userbin/client.rb:69:in `authorized?'

The error originates in the client’s authorized? method:

def authorized?
  !!@store.session_token
end

We had set our token earlier, using subject.session_token = ... from where it gets set to the @store. But there is an asymmetry between the TokenStore’s setter method session_token= which accepts a string and the getter method session_token which takes that string and wraps it in a Userbin::SessionToken. Our earlier setup code where we injected a double with subject.session_token= worked fine, but now the getter treats it like a string and fails.

So how do we make the examples run? One approach would be to do more research on how a legal token string should look like. For example, it will probably have some parts that are separated by “.”. Then it may have to be valid (for some definition of valid) etc. When we find out how to make a valid token string we could inject it into the Request. All that would mean a lot of work just to get a valid test setup. Furthermore, we’re going farther away from the class we are testing here.

Would it be valuable to have a test that starts with a token string and checks expectations about being logged in, allowed operations etc.? Certainly, but it would not be a unit test anymore, rather a functional or integration test.

The other approach is to use a test double to isolate our unit test from the rest of the system: What makes our test setup difficult is the long chain of collaboration where Client uses TokenStore, which instantiates a SessionToken which in turn relies on Userbin::JWT (a JSON web token). We short-circuit the chain and employ one more test double to inject our fake session token. The double is one step removed from our test subject (the client) and breaks the chain of dependencies.

In unit tests, try to break dependencies after one level of collaboration.

My solution was to use a double for TokenStore as well. This is a brute-force approach that relies on RSpec’s ability to intercept messages on real objects. The double goes inside the context ‘with a valid session token’. Here it is:

1
2
3
4
5
6
7
8
9
10
11
12
13
context 'with a valid session token' do
    before(:each) do
      attributes = {
          mfa_enabled?: true,
          device_trusted?: true,
          mfa_in_progress?: false,
          mfa_required?: true,
          has_default_pairing?: true
      }
      token_double = double('session_token', attributes)
      store_double = double("token_store", session_token: token_double)
      allow(Userbin::TokenStore).to receive(:new).and_return(store_double)
    end

In line 12 we inject our store_double when a new TokenStore is created. The store_double then returns our token_double when it is asked for a session token.

The whole test suite runs successfully now:

RSpec output

See the complete spec file at this stage: client_spec.rb

Authorize succeeds

With the properties being tested we can move on to the more interesting methods. Authorization should succeed now that we have a valid session token. Start with the simplest example:

describe '#authorize' do
  it 'succeeds' do
    expect {subject.authorize!}.not_to raise_error
  end
end

This produces an error

expected no Exception, got #<RSpec::Mocks::MockExpectationError:
Double "session_token" received unexpected message :expired? with (no args)>

Fix the session_token double with expired?: false to make the test pass (see the commit).

The authorize method checks if the token is expired which is a new context for our tests. If we wanted to come back later to the scenario with an expired token we could create a new context and mark it as pending:

context 'with an expired session token' do
  pending
end

But let’s work on this context right away. After a few more steps it looks like this:

1
2
3
4
5
6
7
8
9
10
11
12
13
context 'with an expired session token' do
  before(:each) do
    token = valid_session_token.merge expired?: true
    setup_session_token token
  end

  describe '#authorize' do
    it 'sends a heartbeat' do
      expect(Userbin::Monitoring).to receive(:heartbeat)
      subject.authorize!
    end
  end
end

We expect the Userbin service to receive a heartbeat in line 9 (again we know this from looking at the code in the Client class).

Setup code that is shared between examples was refactored to separate methods: valid_session_token returns the default responses for a valid session token as a hash, so we only have to override expired?in line 3. The method setup_session_token deals with the doubles for the session token and the token store.

The client_spec.rb code is now 95 lines long already. Code coverage is at 77.95% overall, with 69.64% for client.rb (17 of 56 lines missed). Looking at the coverage report see scenarios that were not covered yet:

authorize! with MFA in progress
authorize! with MFA required but a device that is not trusted
the login and logout methods
trust_device with a valid session

More contexts for authorize

To finish testing the authorize! method, we add two more contexts: ‘with MFA in progress’ and ‘with an untrusted device’. Fill the contexts with examples for authorize!, again looking into the method’s source code to find out about its expected behavior.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
context 'with MFA in progress' do
  before(:each) do
    token = valid_session_token.merge mfa_in_progress?: true
    setup_session_token token
  end
  describe '#authorize' do
    it 'forces a logout' do
      expect(subject).to receive(:logout)
      expect { subject.authorize! }.to raise_error(
                                           Userbin::UserUnauthorizedError,
                                           /unverified/)
    end
  end
end

In line 8 we make sure that the logout happens. There is nothing really new in the test for the context ‘with an untrusted device’. Notice how the setup code in both contexts focuses on the essential differences: MFA being in progress and an untrusted device.

context 'with an untrusted device' do
  before(:each) do
    token = valid_session_token.merge device_trusted?: false
    setup_session_token token
  end

  describe '#authorize' do
    it 'raises ChallengeRequiredError' do
      expect { subject.authorize! }.to raise_error(Userbin::ChallengeRequiredError)
    end
  end
end

The contexts are an invitation to add more tests for methods other than authorize!. There is, for example, the authorized? method which I would expect to return true if everything is fine with that user.

Side note: And I would expect authorized? to return false when the device is not trusted. However, the way it is implemented now, none of the checks used in authorize! happen:

def authorized?
  !!@store.session_token
end

Instead of exploring how the specification could be, let’s continue with testing what’s there now. The login method creates a session and sets ‘the session token for use in all subsequent requests’:

def login(user_id, user_attrs = {})
  # Clear the session token if any
  @store.session_token = nil

  user = Userbin::User.new(user_id.to_s)
  session = user.sessions.create(
    user: user_attrs, trusted_device_token: @store.trusted_device_token)

  # Set the session token for use in all subsequent requests
  @store.session_token = session.token

  session
end

Get started again by a minimal expectation: login should succeed.

describe '#login' do
  it 'works' do
    subject.login 'the_user_id'
  end
end

This gives a kind of failure that we haven’t seen before:

  1) Userbin::Client#login works
     Failure/Error: subject.login 'the_user_id'
     VCR::Errors::UnhandledHTTPRequestError:


       ================================================================================
       An HTTP request has been made that VCR does not know how to handle:
         POST https://:secretkey@api.userbin.com/v1/users/the_user_id/sessions

There is an actual HTTP request to userbin.com involved when doing the login. We use a special user ID here, therefore none of the canned HTTP responses match the URL https://:secretkey@api.userbin.com/v1/users/the_user_id/sessions. Which is good for us, we don’t want to deal with HTTP responses.

Again, add test doubles for collaborators, in this case Userbin::User and the session:

1
2
3
4
5
6
7
8
9
describe '#login' do
  it 'works' do
    user_double = double('user')
    allow(Userbin::User).to receive(:new).and_return(user_double)
    session_double = double('session', token: 'the token')
    allow(user_double).to receive_message_chain(:sessions, :create => session_double )
    subject.login 'the_user_id'
  end
end

There is a more complicated kind of stubbing here: the message chain user.sessions.create(...) is intercepted in line 6. This cuts off the HTTP request nicely. As you can see, ruby and RSpec allow us to intercept (stub) messages almost at any point in the code. While it is very helpful when working with legacy code it lets you cut across interfaces which results in tests that are very much dependent on the implementation.

The documentation of receive_message_chain even says

[…] you should consider any use of receive_message_chain a code smell.

We may want to come back to the login method later and extract user.sessions.create(...) into a separate method.

So far we had no real expectations on login, just that it should execute without errors. Let’s raise the bar and expect something more important: that the session token is handled properly.

1
2
3
4
5
6
7
8
9
10
11
12
13
describe '#login' do
  it 'stores the new session token' do
    user_double = double('user')
    allow(Userbin::User).to receive(:new).and_return(user_double)
    session_double = double('session', token: 'the new token')
    allow(user_double).to receive_message_chain(:sessions, :create => session_double )
    token = setup_session_token valid_session_token
    expect(@store_double).to receive(:session_token=).with(nil).ordered
    expect(@store_double).to receive(:session_token=).with('the new token').ordered
    expect(@store_double).to receive(:trusted_device_token)
    subject.login 'the_user_id'
  end
end

Starting in line 8 we check that the session token is reset and replaced with the new token. The ordered makes sure that the reset happens before the new token is stored.

These are all white-box tests: we figured out the specifications by looking at the existing code. The test code is quite complex and intrusive. It checks the order in which things are done and the expectations are quite low-level. Another reason that we may want to come back later and make login simpler.

Additional tests for logout and trust_device are pretty similar so I won’t describe them in detail. Instead you can look at the source code for client_spec.rb.

Methods without tests

There are quite a few methods that we did not test at all, they are delegated to the current Userbin user in lines 6-18 of client.rb:

def self.install_proxy_methods(*names)
  names.each do |name|
    class_eval <<-RUBY, __FILE__, __LINE__ + 1
      def #{name}(*args)
        Userbin::User.new('$current').#{name}(*args)
      end
    RUBY
  end
end

install_proxy_methods :challenges, :events, :sessions, :pairings,
  :backup_codes, :generate_backup_codes, :trusted_devices,
  :enable_mfa!, :disable_mfa!

Tests for these methods would have to go into the Userbin::User class. Testing them here would basically mean we are checking that install_proxy_methods works.

You can see three groups of methods in the client:

Proxy methods that are delegated to the current Userbin::User. We did not test these.
Core methods like initialize, authorize!, login, logout. We tested them in detail.
Methods that are delegated to the session token, like mfa_enabled?, device_trusted? etc. We tested these in two contexts, with and without a session token.

It makes sense to review the design of the library IMHO. From my limited understanding, an application that wants to use two-factor authentication mostly interacts with the global userbin client. That keeps things simple for the programmer who uses the Userbin API. But maybe the application should better use three objects (user, session and client) with separate responsibilities and life cycles instead of doing everything through the client. Or SessionToken and the core methods of Client could be combined into a session object, leaving two objects that the application programmer has to interact with: session and user.

Summary

In this part, we’ve tested the real core of the Client class: login, logout, authorization. RSpecs testing report produces a rather detailed verbal specification:

Userbin::Client
  without session token
    should not be authorized
    should not be mfa enabled
    should not be device trusted
    should not be mfa in progress
    should not be mfa required
    should not have default pairing
    #trust_device
      should raise Userbin::UserUnauthorizedError with message matching /Need to call login before trusting device/
    #authorize!
      should raise Userbin::UserUnauthorizedError with message matching /Need to call login before authorize/
  with a valid session token
    should be authorized
    should be mfa enabled
    should be device trusted
    should not be mfa in progress
    should be mfa required
    should have default pairing
    #authorize
      succeeds
    #trust_device
      stores the trusted device token
  with an expired session token
    #authorize
      sends a heartbeat
  with MFA in progress
    #authorize
      forces a logout
  with an untrusted device
    #authorize
      raises ChallengeRequiredError
  #login
    stores the new session token
  #logout
    destroys the session
    resets the session token

That could be used as the basis for a review. Did we think about all possible / relevant scenarios? Maybe there are other things that should be checked: Login with an untrusted device? trust_device with an expired session?

Test coverage is at 81.89% overall, and 96.43% for client.rb. Keep in mind that line coverage is just a side effect, not the real goal here. It is much more important to have essential scenarios covered by tests, and maybe even find scenarios that were missed in the implementation.

Making the tests exposed some opportunities for improving the code:

Complicated test setup points to overly coupled objects
Having to stub message chains points to violations of the Law of Demeter

Watch the talk The deep synergy between testability and good design by Michael Feathers to learn more about this.

In part 3, we’ll take advantage of the tests to do some refactoring of the Client class.

Properties with a valid session token

Authorize succeeds

More contexts for authorize

Testing login

Methods without tests

Summary