Page 5 of 15

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

Posted: Thu Apr 24, 2008 6:44 pm
by Chris Corbyn
Great :) Anybody care to write the test for a TRUE return value on successful save()? :)

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

Posted: Thu Apr 24, 2008 9:37 pm
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);
  }
 
}
 

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

Posted: Fri Apr 25, 2008 12:29 am
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.

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

Posted: Fri Apr 25, 2008 1:26 am
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

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

Posted: Fri Apr 25, 2008 1:28 am
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);
  }
 

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

Posted: Fri Apr 25, 2008 2:28 am
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()? :)

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

Posted: Fri Apr 25, 2008 3:01 am
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 ..

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

Posted: Fri Apr 25, 2008 3:11 am
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.

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

Posted: Fri Apr 25, 2008 3:30 am
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();
}
 

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

Posted: Fri Apr 25, 2008 3:51 am
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.

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

Posted: Fri Apr 25, 2008 4:28 am
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);
    }
 

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

Posted: Fri Apr 25, 2008 4:50 am
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.

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

Posted: Fri Apr 25, 2008 5:47 am
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]);
    }
 

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

Posted: Fri Apr 25, 2008 6:25 am
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?

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

Posted: Fri Apr 25, 2008 7:09 am
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?