http://www.flickr.com/photos/mumumio/8356659421/
Attribution-NonCommercial License

delicious controversy

Docs & Tests



Thomas Meeks

Why?

Over 27,000 lines

29 committers

Lots of rotation

Lessons Learned

Champion Practicality

Docs!

subscription.cancel

subscription.cancel!

Self Documenting Code is a Pipe Dream

Puzzle piece by Joey Day
http://www.flickr.com/photos/joeyday/147651554/
Attribution License

Umpossible! by Ben
http://www.flickr.com/photos/mrbenn/768375889/
Attribution-NonCommercial-NoDerivs License

It can be done

But it takes too damn long

Also soul-crushing

Tests are Terrible Documentation

In order to understand these 11,000 lines of code, please sift through 16,000 lines of rspec.

Docs exist so others can use classes without deeply understanding them

Sane & Useful Docs

  • Document all public methods
  • Document as you go
  • Keep it short
  • Do not document implementation

Answer three questions

  • What does the method need?
  • What does the method do?
  • What are the far-reaching consequences?

Treat it like a public API

Jigsaw puzzle of ‘GWR The Torbay Express 375’, Chad Valley Co Ltd, 1939 by Birmingham Museum and Art Gallery
http://www.flickr.com/photos/birminghammag/7920558348/
Attribution License

Testing!

We have 16,000 lines of specs

5,000 more lines than application code

16,000 lines of maintenance

Why?

Prevent Bugs

Unit Tests

Test a method without "leaving" the class

Unit Tests are bad tests

  • Too many lines of test code per line covered.
  • Too coupled to implementation.
  • Creates too much maintenance.
  • Fails to ensure you are using classes correctly.

TDD creates bad tests

TDD is about good design, not good tests

But TDD does not guarantee good design

(So stop acting like it does)

Use TDD if it works for you

But please delete your unit tests

(After you replace them with...)

Integration Tests

Test the "full stack", check the result

Integration tests are beautiful

  • Efficienly tests a lot of code
  • Tests our complex interactions
  • Right mindset: make sure the feature works

So Capybara?

Nope

  • Takes too long to implement
  • Too much maintenance
  • Too many gotchas
  • Doesn't encourage good code
  • View changes tend to break tests

So, what then?

Sane Tests

Models, Services, and Presenters

Model

  • Core application logic
  • Do one thing, do it well
  • Not necessarily ActiveRecord

Before:

def charge 
  if billing_object.charge
    self.paid = true
    self.next_bill_on = 30.days.from_now
  else
    self.paid = false
    self.next_bill_on = 1.day.from_now
  end
end

def billing_object
  Billing::Subscription.find(self.id)
end

After:

# @!attribute paid?
#   @return [Boolean] true if payment is current

# Immediately attempts a charge to renew the subscription,
# and updates the paid status. Also updates next_bill_on 
# so we know when to try again.
#
# @return [Date] the next time a charge should be attempted
# @raise  [BillingUnavailableError] if billing is down
def charge 
  if billing_object.charge
    self.paid = true
    self.next_bill_on = 30.days.from_now
  else
    self.paid = false
    self.next_bill_on = 1.day.from_now
  end
end

private :billing_object # who cares?

Testing:

VCR.use_cassette('valid_subscription') do
  subscription = Factory.new(:valid_subscription)
  subscription.charge

  assert subscription.paid
  assert_equal 30.days.from_now, subscription.next_bill_on
end

Service

  • Orchestrates models
  • Handles logic that doesn't fit into a model
  • Helps controllers become paper-thin

Before Service:

def create
  @subscription = Subscription.new(params[:subscription])

  if @subscription.valid?
    discount = Discount.find_for_subscription(@subscription)
    @subscription.add_discount(discount)
    @subscription.charge

    redirect_to user_path(current_user)
  else
    render :new
  end
end

After Service:

def create
  @service = SubscriptionService.new(params[:subscription])

  if @service.create
    redirect_to user_path(current_user)
  else
    render :new
  end
end

Documenting a Service:

  # Creates a new subscription and applies a discount to it,
  # if appropriate. Immediately charges the subscription.
  #
  # @return [Boolean] true if the operation was successful
  # @raise  [BillingDownError] if the billing system is down
  def create
    if @subscription.valid?
      discount = Discount.find_for_subscription(@subscription)
      @subscription.add_discount(discount)
      @subscription.charge
      @subscription.paid?
    else
      false
    end
  end

Testing a Service:

discount = Factory.new(:global_discount)
service = SubscriptionService.create(good_params)

assert service.create
assert service.subscription.paid?

assert_equal discount, service.subscription.discount

Presenter

  • Helps display complex views
  • Template (and framework) agnostic
  • Helps controllers, and views, become paper-thin

Before Presenter:

def show
  @subscription.find(params[:id])
  @user = @subscription.user
end
.subscription-data
  .label= @subscription.name
  .label= @subscription.active? ? @subscription.expiry : "None!"
.course-data
  .label= @user.courses.completed.count
  .label= @user.next_recommended_course || "That's all folks!"

After Presenter:

def show
  @presenter = SubscriptionPresenter.new(params[:id])
end
.subscription-data
  .label= @presenter.name
  .label= @presenter.expiration_date
.course_data
  .label= @presenter.completed_course_count
  .label= @presenter.next_recommended_course

Documenting a Presenter:

# @return [String] the name of the subscription
def name
  @subscription.name
end

# @return [String] the expiration date in string format
def expiration_date
  @subscription.active? ? @subscription.expiry : "None!"
end

# @return [Integer] user's completed course count 
def completed_course_count
  @user.completed_course_count
end

# @return [String] the user's next recommended course
def next_recommended_course
  @user.next_recommended_course || "That's all folks!"
end

Testing a Presenter:

subscription = Factory.new(:subscription)
presenter    = SubscriptionPresenter.new(subscription.id)

assert_equal subscription.name, @presenter.name
assert_equal "1/1/2014", @presenter.expiration_date
assert_equal 5, @presenter.completed_course_count
assert_equal "Backbone", @presenter.next_recommended_course

What about Controllers & Views?

They should be so simple tests are a waste of time

Even if you have to pull parameter processing & redirects into a service

skinny

skeletal controllers

Guidelines

  • Every class is an API defined by its public methods
  • A method should only be public if it must be
  • Document public methods
  • Write feature/integration tests for public methods
  • Every line of code has a maintenance cost, even if they are tests
  • Refactor slowly, do not engage the "big refactor"
  • These are guidelines, bend freely

Exceptions to the rules!

  • Especially critical code
  • Algorithmically complex code
  • Code that breaks frequently
  • HTTP APIs (headers are an important part of the contract)

Mindset is Important

  • No controller testing = very simple controllers
  • No view testing = very simple views
  • Test & document public methods = make sure it really needs to be public
  • No private method tests = good feature tests
  • Less time trying to understand code = happier programmers
  • Less time maintaining tests = happier programmers
  • Working the way that works for you = happier programmers

Thanks!

thomasmeeks.com/controversy