TDD workshop. Anybody welcome; Data storage abstraction.

Discussion of testing theory and practice, including methodologies (such as TDD, BDD, DDD, Agile, XP) and software - anything to do with testing goes here. (Formerly "The Testing Side of Development")

Moderator: General Moderators

Post Reply
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by Chris Corbyn »

Great :) Anybody care to write the test for a TRUE return value on successful save()? :)
georgeoc
Forum Contributor
Posts: 166
Joined: Wed Aug 09, 2006 4:21 pm
Location: London, UK

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by georgeoc »

@arjan.top - You've added a test to check that save() delegates to the storage model, and set up the expectation in the mock. That's great. But are you sure your implementation is only enough code to make the test pass, and no more?

I can make the test pass with this alone:

Code: Select all

 public function save() {
    $this->_storageModel->save();
  }
 
How would you change the test case to test that the store is passed to the storage model as part of the save() ?
arjan.top wrote:DataStoreTest.php

Code: Select all

// ... snip ...
 
  public function testSaveDelegatesToStorageModel() {
    $this->_storageModel->expectOnce('save');
    $this->_dataStore->save();
  }
 
// ... snip ...
 

Code: Select all

 
<?php
 
require_once dirname(__FILE__) . '/StorageModel.php';
 
class DataStore {
 
  private $_store = array();
 
  private $_storageModel;
 
  public function __construct(StorageModel $storageModel) {
    $this->_storageModel = $storageModel;
  }
 
  public function set($key, $value) {
    $this->_store[$key] = $value;
  }
 
  public function get($key) {
    return $this->_store[$key];
  }
 
  public function save() {
    $this->_storageModel->save($this->_store);
  }
 
}
 
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by matthijs »

What about

Code: Select all

 
// class DataStoreTest:
 
  public function testSaveValueReturnsTrueOnSuccess(){
    $this->_storageModel->setReturnValue('save', true);
        //$this->_dataStore->set('foo', 'bar');
    $rs = $this->_dataStore->save();
    $this->assertTrue($rs);
  }
 
// class dataStore:
 
  public function save() {
    $rs = $this->_storageModel->save($this->_store);
    return $rs;
  }
 
it passes but it feels kind of weird. Have to read the mock docs some better.
User avatar
arjan.top
Forum Contributor
Posts: 305
Joined: Sun Oct 14, 2007 4:36 am
Location: Hoče, Slovenia

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by arjan.top »

georgeoc wrote:@arjan.top - You've added a test to check that save() delegates to the storage model, and set up the expectation in the mock. That's great. But are you sure your implementation is only enough code to make the test pass, and no more?

I can make the test pass with this alone:

Code: Select all

 public function save() {
    $this->_storageModel->save();
  }
 
It won't pass under E_ALL
User avatar
arjan.top
Forum Contributor
Posts: 305
Joined: Sun Oct 14, 2007 4:36 am
Location: Hoče, Slovenia

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by arjan.top »

matthijs wrote:What about

Code: Select all

 
// class DataStoreTest:
 
  public function testSaveValueReturnsTrueOnSuccess(){
    $this->_storageModel->setReturnValue('save', true);
        //$this->_dataStore->set('foo', 'bar');
    $rs = $this->_dataStore->save();
    $this->assertTrue($rs);
  }
 
// class dataStore:
 
  public function save() {
    $rs = $this->_storageModel->save($this->_store);
    return $rs;
  }
 
it passes but it feels kind of weird. Have to read the mock docs some better.
Looks good, but I would do replace temp with query

Code: Select all

 
// class DataStoreTest:
 
  public function testSaveValueReturnsTrueOnSuccess(){
    $this->_storageModel->setReturnValue('save', true);
        //$this->_dataStore->set('foo', 'bar');
    $this->assertTrue($this->_dataStore->save(););
  }
 
// class dataStore:
 
  public function save() {
    return $this->_storageModel->save($this->_store);
  }
 
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by Chris Corbyn »

arjan.top wrote:Looks good, but I would do replace temp with query
That's true, and great use of terminology :) What arjan.top is referring to is a very well-known refactor taken from Fowler's "Refactoring" book.

The more temporary (inline) variables you have in code the harder it becomes to refactor. Anywhere you see a temporary variable which only every gets assigned to once, replace it with a method call to get that same value. Even if this seems more expensive of an operation, you can optimize later. But the amount of time it saves you when refactoring to create new classes/methods is worth it. That step alone, "replace temp with query" is often the first step you'll take before using any other refactoring anyway.

I could have let it slip for now since we can do that later, but since arjan.top is on the ball and has raised this point now we might as well go with it earlier rather than later.

Ok, and the test for an unsuccessful save()? :)
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by matthijs »

Code: Select all

 
    public function testSaveValueReturnsFalseOnFail(){
        $this->_storageModel->setReturnValue('save', false);
        $this->assertFalse($this->_dataStore->save());
    }
 
It still feels like I'm cheating ..
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by Chris Corbyn »

matthijs wrote:

Code: Select all

 
    public function testSaveValueReturnsFalseOnFail(){
        $this->_storageModel->setReturnValue('save', false);
        $this->assertFalse($this->_dataStore->save());
    }
 
It still feels like I'm cheating ..
You are, but that's correct ;) Your emaulating the case where saving fails. You don't want to use a real StorageModel and force it to fail. It'll come together like a jigsaw once we write some tests for the actual StorageModels.

Code to pas the test? Already passes yes? :)

Ok, delete() or remove() a value from the DataStore? Not fussed what method name we use.
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by matthijs »

Code: Select all

 
// class DataStoreTest 
    public function testRemoveValueFromStoreReturnsTrueOnSuccess(){
        $this->_dataStore->set('foo', 'bar');
        $this->_storageModel->setReturnValue('remove', true);
        $this->assertTrue($this->_dataStore->remove('foo'));
    }
 
    public function testRemoveValueFromStoreReturnsFalseOnFail(){
        $this->_dataStore->set('foo', 'bar');
        $this->_storageModel->setReturnValue('remove', false);
        $this->assertFalse($this->_dataStore->remove('foo'));
    }
// class DataStore
    public function remove($key){
        return $this->_storageModel->remove($this->_store[$key]);
    }
// StorageModel
interface StorageModel {
    public function save($data);
    public function remove($data);
    public function load();
}
 
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by Chris Corbyn »

Ok, some confusion here. StorageModel has save()/load() only. All set/get/remove operations are to be performed by DataStore ;) This is really just like our get/set test except we're testing that a value is removed now.

Let's side-step to make this test easier to write.

Maybe it'd be clearer if we first write a test for has(), and then make use of that method when we test remove()?

Code: Select all

...
public function testDataStoreKnowsItHasSingleValue() {
  $this->_dataStore->set('foo', 'bar');
  $this->assertTrue($this->_dataStore->has('foo'));
}
 
public function testDataStoreKnowsWhenItDoesntHaveAValue() {
  $this->assertFalse($this->_dataStore->has('zip'));
}
At least if we make that test work we can use it to help us test if a value is removed.
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by matthijs »

But how are we supposed to remove something if the Datastore class has no method to remove? :?

Anyway, I wrote this to make your test pass

Code: Select all

 
// class DataStore
    public function has($key){
        return array_key_exists($key, $this->_store);
    }
 
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by Chris Corbyn »

matthijs wrote:But how are we supposed to remove something if the Datastore class has no method to remove? :?
Thinking too far ahead ;) But save() does save the entire (modified) store so it will be fine.
Anyway, I wrote this to make your test pass

Code: Select all

 
// class DataStore
    public function has($key){
        return array_key_exists($key, $this->_store);
    }
 
Excellent :)

So now it should be easy to test remove() by simply setting a value, calling remove() and then asserting that has() returns false. This is another one of those stateful tests where you're calling a sequence of methods to put the SUT into a wanted state.

Just for anyone joining in, here's the latest set of files.
Attachments
data_store.zip
(136.13 KiB) Downloaded 183 times
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by matthijs »

So now it should be easy to test remove() by simply setting a value, calling remove() and then asserting that has() returns false. This is another one of those stateful tests where you're calling a sequence of methods to put the SUT into a wanted state.
First I need to know: are we removing from the dataStore or from the StorageModel?

If it is the first, this makes the test pass

Code: Select all

 
// test
    public function testRemoveValueRemovesValue(){
        $this->_dataStore->set('foo', 'bar');
        $this->assertTrue($this->_dataStore->has('foo'));
        $this->_dataStore->remove('foo');
        $this->assertFalse($this->_dataStore->has('foo'));
    }
 
// datastore
    public function remove($key){
        unset($this->_store[$key]);
    }
 
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by Chris Corbyn »

matthijs wrote:
So now it should be easy to test remove() by simply setting a value, calling remove() and then asserting that has() returns false. This is another one of those stateful tests where you're calling a sequence of methods to put the SUT into a wanted state.
First I need to know: are we removing from the dataStore or from the StorageModel?

If it is the first, this makes the test pass

Code: Select all

 
// test
    public function testRemoveValueRemovesValue(){
        $this->_dataStore->set('foo', 'bar');
        $this->assertTrue($this->_dataStore->has('foo'));
        $this->_dataStore->remove('foo');
        $this->assertFalse($this->_dataStore->has('foo'));
    }
 
// datastore
    public function remove($key){
        unset($this->_store[$key]);
    }
 
Removing from the DataStore. StorageModel is just the persistence layer; it won't be removed from that persistent storage until save() is called.

Your test looks good. If it were my test personally I'd probably drop the first assertTrue() since we have previous tests which show that works and our test method is only really supposed to be showing the effect of remove(). It's a superfluous assertion but I agree that adding it helps to show what's happening as the state of the SUT changes. I used to always make multiple assertions like that and I'm not particularly opposed to doing it these days... I just generally don't feel the need. We'll keep it as you have it anyway since it's still correct and I'm mainly just talking about personal preference.

This whole "one assertion per test method" idea some people have is a good rule-of-thumb, but not a hard fact.

Ok, the last "simple" test we'll write is to have the DataStore ask the StorageModel to load its existing data (if any). Ideally this will happen in the constructor so that we have a usable object right after instantiation. Looking back at the tests for save() could you think how we'd make the expectation that load() is called?
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Post by matthijs »

Removing from the DataStore. StorageModel is just the persistence layer; it won't be removed from that persistent storage until save() is called.
Ok, now I understand what the purpose is.
This whole "one assertion per test method" idea some people have is a good rule-of-thumb, but not a hard fact.
Good to know. These little things are good to know, because the reason why I stopped testing some time ago after a few days trying was that my tests became one big, horrible, complicated mess of dependencies. At some point I didn't have a clue anymore what I was testing. If anything. That's what happens when you try to run before you can crawl..

Maybe

Code: Select all

 
    public function testInstantiateDatastoreLoadsStorageModel(){
        $this->_storageModel->expectOnce('load');
    }
 
?
But I find this one more difficult, because you want to load it in the constructor. But the datastore is already instantiated in the setup method of the testclass, so how can I test the constructor?
Post Reply