Fixing the Bug
Sometimes the signals you’ll get from the code that something is wrong are pretty faint. We always try to keep a to-do list handy and write down everything that concerns us as we go. That way, we can stay focused on the task at hand but know that we’ll get a chance to check over and clean up any inconsistencies before we move onto the next task.
As we were finishing off the last scenario, we noticed that Teller#withdraw_from takes two arguments, but only one of them was used in the method to make the scenario pass. This inconsistency means that something is wrong, but we need to investigate further to know what we need to do about it. Let’s take a look at the code that uses the method:
| support_code/01/features/step_definitions/teller_steps.rb | |
| | When /^I withdraw (#{CAPTURE_CASH_AMOUNT})$/ do |amount| |
| | teller.withdraw_from(my_account, amount) |
| | end |
It seems pretty obvious what we intended to happen here. The Teller should take the specified amount of money out of the account and hand it to the CashSlot. So, how did we manage to make the scenario pass without having to do anything to the account when we implemented that method? Let’s take another look at the scenario:
| support_code/01/features/cash_withdrawal.feature | |
| | Feature: Cash Withdrawal |
| | Scenario: Successful withdrawal from an account in credit |
| | Given I have deposited $100 in my account |
| | When I withdraw $20 |
| | Then $20 should be dispensed |
A-ha! There’s nothing here that actually checks that the balance of the account has been reduced to $80. It’s a good thing we caught this now—our code would have been handing out cash to customers for free!
One of the wonderful things I discovered when I first used Cucumber to build a complete application from the outside-in was when we started manual exploratory testing and discovered some bugs. Almost without exception, every one of those bugs could be traced back to a gap in the Cucumber scenarios we’d written for that part of the system. Because we’d written the scenarios collaboratively, with businesspeople and developers working together to get them right, it wasn’t anybody’s fault that there was a bug. It was just an edge case we hadn’t originally anticipated.
In my experience, bugs are a big source of friction and unhappiness in software teams. Businesspeople blame them on careless developers, and developers blame them on inadequate requirements from the businesspeople. Actually they’re just a natural consequence of software development being a really complex endeavor. Using Cucumber really helped the team see that closing these gaps is everyone’s job. The blaming just didn’t happen, and we were a much happier team as a result.
Let’s tack on this extra outcome as another Then step:
| support_code/02/features/cash_withdrawal.feature | |
| | Feature: Cash Withdrawal |
| | Scenario: Successful withdrawal from an account in credit |
| | Given I have deposited $100 in my account |
| | When I withdraw $20 |
| | Then $20 should be dispensed |
* | And the balance of my account should be $80 |
Run cucumber and paste the snippet for the new step definition into the file features/step_definitions/account_steps.rb:
| support_code/02/features/step_definitions/account_steps.rb | |
| | Given /^I have deposited (#{CAPTURE_CASH_AMOUNT}) in my account$/ do |amount| |
| | my_account.deposit(amount) |
| | my_account.balance.should eq(amount), |
| | "Expected the balance to be #{amount} but it was #{my_account.balance}" |
| | end |
| | |
| | Then /^the balance of my account should be $(d+)$/ do |arg1| |
| | pending # express the regexp above with the code you wish you had |
| | end |
We can tidy up the regular expression for the step definition using the transform we created in the previous chapter and duplicate the assertion from the previous step definition:
| support_code/03/features/step_definitions/account_steps.rb | |
| | Given /^I have deposited (#{CAPTURE_CASH_AMOUNT}) in my account$/ do |amount| |
| | my_account.deposit(amount) |
| | my_account.balance.should eq(amount), |
| | "Expected the balance to be #{amount} but it was #{my_account.balance}" |
| | end |
| | |
| | Then /^the balance of my account should be (#{CAPTURE_CASH_AMOUNT})$/ do |amount| |
* | my_account.balance.should eq(amount), |
* | "Expected the balance to be #{amount} but it was #{my_account.balance}" |
| | end |
We’ll make a note on our to-do list to tidy up that duplication and carry on trying to fix this bug. Run cucumber now to see whether we have it trapped:
| | Feature: Cash Withdrawal |
| | |
| | Scenario: Successful withdrawal from an account in credit |
| | Given I have deposited $100 in my account |
| | When I withdraw $20 |
| | Then $20 should be dispensed |
| | And the balance of my account should be $80 |
| | Expected the balance to be 80 but it was 100 |
| | (RSpec::Expectations::ExpectationNotMetError) |
| | ./features/step_definitions/account_steps.rb:9 |
| | features/cash_withdrawal.feature:6 |
| | |
| | Failing Scenarios: |
| | cucumber features/cash_withdrawal.feature:2 |
| | |
| | 1 scenario (1 failed) |
| | 4 steps (1 failed, 3 passed) |
| | 0m0.003s |
Great—the bug has been caught by our scenario. Just as we suspected, the account’s balance remains untouched by the withdrawal. But not for long! Crack open lib/nice_bank.rb, and take a look at our Teller class:
| support_code/03/lib/nice_bank.rb | |
| | class Teller |
| | def initialize(cash_slot) |
| | @cash_slot = cash_slot |
| | end |
| | def withdraw_from(account, amount) |
| | @cash_slot.dispense(amount) |
| | end |
| | end |
Change the withdraw_from method to tell the account what to do:
| support_code/04/lib/nice_bank.rb | |
| | class Teller |
| | def initialize(cash_slot) |
| | @cash_slot = cash_slot |
| | end |
| | def withdraw_from(account, amount) |
* | account.debit(amount) |
| | @cash_slot.dispense(amount) |
| | end |
| | end |
Now when we run cucumber, we’re told we need to create this new method on the Account. On a real project, we’d drop down a gear at this point and start writing unit tests for the Account class to drive out that method. We don’t mind sketching out interfaces to classes with a few basic method stubs with only Cucumber to back us up, but as soon as we start adding interesting behavior to a class, we like to make sure that behavior has been specified in a unit test. The RSpec Book [CADH09] and Growing Object-Oriented Software, Guided by Tests [FP09] both do a great job of explaining this balance with plenty of examples, but since this book is focused on Cucumber, we’re going to gloss over this step and carry on.
Find the Account class in lib/nice_bank.rb and change it to look like this:
| support_code/05/lib/nice_bank.rb | |
| | class Account |
| | def deposit(amount) |
| | @balance = amount |
| | end |
| | def balance |
| | @balance |
| | end |
| | def debit(amount) |
| | @balance -= amount |
| | end |
| | end |
We’ve implemented a new method called debit that decrements the @balance by the given amount. Let’s see whether we’ve managed to fix the bug:
| | Feature: Cash Withdrawal |
| | |
| | Scenario: Successful withdrawal from an account in credit |
| | Given I have deposited $100 in my account |
| | When I withdraw $20 |
| | Then $20 should be dispensed |
| | And the balance of my account should be $80 |
| | |
| | 1 scenario (1 passed) |
| | 4 steps (4 passed) |
| | 0m0.003s |
Hooray, we did it! Now it’s time for some refactoring before we move on.
Reviewing and Refactoring
In Extreme Programming Explained [Bec00], Kent Beck gives four criteria for a simple design. In order, with the most important first, they are as follows:
-
Passes all the tests
-
Reveals all the intention
-
Contains no duplication
-
Uses the fewest number of classes or methods
Let’s review our current code against those four criteria. It’s been driven from a single test made from a single Cucumber scenario that is passing, so that’s rule 1 taken care of. How about rule 2? Revealing intention is essentially about how things are named, which matters to us a great deal. Let’s take a look at the methods on our Account class:
| support_code/05/lib/nice_bank.rb | |
| | class Account |
| | def deposit(amount) |
| | @balance = amount |
| | end |
| | def balance |
| | @balance |
| | end |
| | def debit(amount) |
| | @balance -= amount |
| | end |
| | end |
Reflecting on it, it’s confusing that we’ve used the two method names deposit and debit. Really, we’d either like to see deposit and withdraw or credit and debit, but not a combination of the two. Which pair is more appropriate for our Account class?
We spend a bit of time chatting with one of our domain experts, and it becomes clear that credit and debit are the right names for the methods on Account. In fact, deposit is something you’d more likely ask a Teller to do for you. These conversations happen all the time as you start to establish a ubiquitous language, and you’ll find they become easier and easier as your knowledge of the domain, and the ubiquitous language you all use to discuss it, grows.
We’re going to rename the Account#deposit method to Account#credit. Since our test is nice and fast, we can just rename the method and see what breaks:
| | Feature: Cash Withdrawal |
| | |
| | Scenario: Successful withdrawal from an account in credit |
| | Given I have deposited $100 in my account |
| | undefined method ‘deposit’ for #<Account:0x63756b65> (NoMethodError) |
| | ./features/step_definitions/account_steps.rb:2 |
| | features/cash_withdrawal.feature:3 |
| | When I withdraw $20 |
| | Then $20 should be dispensed |
| | And the balance of my account should be $80 |
| | |
| | Failing Scenarios: |
| | cucumber features/cash_withdrawal.feature:2 |
| | |
| | 1 scenario (1 failed) |
| | 4 steps (1 failed, 3 skipped) |
| | 0m0.002s |
In Refactoring: Improving the Design of Existing Code [FBBO99], this technique (make the change, see what breaks) is called leaning on the compiler. Since we don’t have a compiler in Ruby programs, we have to settle for leaning on the tests. From the backtrace, it looks like we need to change line 2 of features/step_definitions/account_steps.rb. Go ahead and change that, and run cucumber again.
Our scenario is passing, but we’re not quite done yet! The language in the step is now inconsistent with the code inside the step definition we’ve just changed:
| support_code/07/features/step_definitions/account_steps.rb | |
| | Given /^I have deposited (#{CAPTURE_CASH_AMOUNT}) in my account$/ do |amount| |
| | my_account.credit(amount) |
| | my_account.balance.should eq(amount), |
| | "Expected the balance to be #{amount} but it was #{my_account.balance}" |
| | end |
In the step, we’re still using the term deposited, but now we’re crediting the account in the Ruby code underneath. How much does this matter? Perhaps not much, but we’d prefer our test code to be as transparent as possible. After another discussion with our domain expert, we decide to reword the step to read Given my account has been credited with $100. We’ll change the feature and the underlying step definition:
| support_code/08/features/cash_withdrawal.feature | |
| | Feature: Cash Withdrawal |
| | Scenario: Successful withdrawal from an account in credit |
| | Given my account has been credited with $100 |
| | When I withdraw $20 |
| | Then $20 should be dispensed |
| | And the balance of my account should be $80 |
| support_code/08/features/step_definitions/account_steps.rb | |
| | Given /^my account has been credited with (#{CAPTURE_CASH_AMOUNT})$/ do |amount| |
| | my_account.credit(amount) |
| | my_account.balance.should eq(amount), |
| | "Expected the balance to be #{amount} but it was #{my_account.balance}" |
| | end |
Looking at this step definition now, it doesn’t make sense anymore to check that the balance is the same as the amount credited. If this step definition were used in another scenario that involved other credit or debit transactions before this one, the balance might not be the same as the amount credited, and that would be quite all right. So, we don’t want this assertion here any more.
That gives us an easy way to cross off the last item on our to-do list: removing the duplication of the balance assertion in the two-step definitions for the account. We can simply delete the first one, because it’s no longer relevant:
| support_code/09/features/step_definitions/account_steps.rb | |
| | Given /^my account has been credited with (#{CAPTURE_CASH_AMOUNT})$/ do |amount| |
| | my_account.credit(amount) |
| | end |
| | Then /^the balance of my account should be (#{CAPTURE_CASH_AMOUNT})$/ do |amount| |
| | my_account.balance.should eq(amount), |
| | "Expected the balance to be #{amount} but it was #{my_account.balance}" |
| | end |
That completes our refactoring. The code is now as clear and communicative and free of duplication as we can imagine at this stage. We’re ready to add some new functionality.
Figure 7. What skills do you need to write Cucumber tests?