Trying to understand the point of Gary Bernhardt's "Boundaries" talk

original.rb

describe Sweeper do
  context 'a subscription is expired' do
    let(:bob) { double(active?: true, paid_at: 2.months.ago) }
    let(:users) { [bob] }
    before { allow(User).to receive(:all).and_return(users) }
    
    it 'emails the user' do
      expect(UserMailer).to receive(:billing_problem).with(bob)
      described_class.sweep
    end
  end
end

class Sweeper
  def self.sweep
    User.all.select do |user|
      user.active? and user.paid_at < 1.month.ago
    end.each do |user|
      UserMailer.billing_problem(bob)
    end
  end
end

refactored.rb

describe Sweeper do
  context 'a subscription is expired' do
    let(:bob) { double(active?: true, paid_at: 2.months.ago) }
    
    it 'emails the user' do
      expect(ExpiredUsers.for_users(users)).to eq [bob]
    end
  end
end

class ExpiredUsers
  def self.for_users(user)
    users.select do |user|
      user.active? and user.paid_at < 1.month.ago
    end
  end
end

class Sweeper
  def self.sweep
    ExpiredUsers.for_users(User.all).each do |user|
      UserMailer.billing_problem(user)
    end
  end
end

This is the "before" and refactored "after" code from Gary Bernhardt's "Boundaries" talk (slightly modified to use modern RSpec syntax).

What did this refactoring accomplish?

By extracting to ExpiredUsers.for_users, we get more test coverage, so if the test for Sweeper.sweep breaks due to a change in the the "users.select" code, it'll be easier to precisely pinpoint. A small win.

But notice that there's no code testing Sweeper.sweep anymore. That test has to:

1. Ensure that users are returned by ExpiredUsers.for_users, which can be done by:
   1. Stubbing ExpiredUsers.for_users, or
   2. Saving users to the database.
2. Ensure that UserMailer.billing_problem was called with the expected user(s), which can be done by:
   1. Mocking UserMailer.billing_problem, or
   2. Checking some external state (e.g. the database) for whatever effects UserMailer.billing_problem is supposed to have.

But the refactoring has no effect on any of this.

Now you might make the argument that the refactoring allows Sweeper.sweep to be tested with merely one user being returned by ExpiredUsers.for_users. That's a performance advantage if we chose option 1.2 above ("Saving users to the database") because it means we have less interaction with the database. But it has nothing to do with reducing code paths for an integration test. You could do the integration test of Sweeper.sweep with e.g. three users (all saved within a single example, so just one integration test example):

1. A user who is active and paid.
2. A user who is active but not paid.
3. A user who is paid but not active.

Then you've simultaneously integration tested ExpiredUsers.for_users and Sweeper.sweep, regardless of whether you choose to do the refactoring. Given this, the refactoring just seems like a way to make the code clearer (SRP ftw). It also has the database interaction performance advantage. These are worthwhile advantages, but they have absolutely nothing to do with "reducing integration test paths".

Sweeper.sweep is so simple that it doesn't justify a test of its own - there is no branching, and any test is basically just going to copy the implementation.

As long as that code gets executed somewhere (say by an acceptance test), that should be sufficient.

You could do the integration test of Sweeper.sweep with e.g. three users

This argument doesn't scale up with the combinatoric explosion of different branches as code complexity increases.

@xaviershay Iiiinteresting point about not testing it at all because the test would mirror the implementation. This reminds me of the argument against testing e.g. "has_many :users" or "should have_db_column(:foo)" - this is not code that you would ever refactor, hence tests accomplish very little. Thanks!

I think the trouble here is that the example Gary created is somewhat contrived. Where I think it becomes very obviously useful is when you have complex conditional logic mixed with imperative logic that modifies state.

At that point, when you factor our the functional stuff, it's all really easy to test because you just assert on values. The imperative stuff is easy to test because you only need one good test that shows that the functional innards got called. Whether you decide to do that with a stub and some kind of rspec-fire/bogus thing that ensures the contract, or an actual integration test (my preference in most cases) - you assume that the functional innards can do their job well, so you only have 1 happy path test on the integration side.

I mentioned over twitter that we recently refactored an accounting system in this way. When we started refactoring we didn't think about Boundaries or functional vs imperative, we just started separating things by SRP principle. The result incidentally became a split between the functional "Line Item Builder" which was concerned with lots of math and conditionals (things like calculating fees, etc), and the imperative "Statement Builder" which wrote line items to the database as part of a Statement object.

The original code had conflated it all into one object called a Statement Builder. The only way to meaningfully test it, since it wrote stuff to the database, was to have N paths for every kind of line item it was creating and to assert on database state after the fact.

Post our refactoring, we have N tests in the functional line item piece, covering all the conditional and math logic. It does not talk to the DB. On the imperative StatmentBuilder side we basically have 1 test that says given a line item, you attach it to a statement. There's a pretty big win in terms of test speed here, as well as test readability. But I think most of it came from just applying SRP principles. The functional/imperative split was a nice cherry on top. After I realized we had actually done the functional/imperative split - I said - hey this is Gary's boundaries at work. And now I think this technique will help me look more for code that doesn't belong together.

My accounting stuff is too Reverb specific to post, but at some point in the future if I have a good example of this type of refactoring, I'll make a post on the Reverb Dev Blog