TDD workshop. Anybody welcome; Data storage abstraction.
Moderator: General Moderators
- 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.
Great
Anybody care to write the test for a TRUE return value on successful save()? 
Re: TDD workshop. Anybody welcome; Data storage abstraction.
@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:
How would you change the test case to test that the store is passed to the storage model as part of the save() ?
I can make the test pass with this alone:
Code: Select all
public function save() {
$this->_storageModel->save();
}
arjan.top wrote:DataStoreTest.phpCode: 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.
What about
it passes but it feels kind of weird. Have to read the mock docs some better.
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;
}
Re: TDD workshop. Anybody welcome; Data storage abstraction.
It won't pass under E_ALLgeorgeoc 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(); }
Re: TDD workshop. Anybody welcome; Data storage abstraction.
Looks good, but I would do replace temp with querymatthijs wrote:What aboutit passes but it feels kind of weird. Have to read the mock docs some better.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; }
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);
}
- 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.
That's true, and great use of terminologyarjan.top wrote:Looks good, but I would do replace temp with query
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.
Code: Select all
public function testSaveValueReturnsFalseOnFail(){
$this->_storageModel->setReturnValue('save', false);
$this->assertFalse($this->_dataStore->save());
}
- 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.
You are, but that's correctmatthijs wrote:It still feels like I'm cheating ..Code: Select all
public function testSaveValueReturnsFalseOnFail(){ $this->_storageModel->setReturnValue('save', false); $this->assertFalse($this->_dataStore->save()); }
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.
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();
}
- 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.
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()?
At least if we make that test work we can use it to help us test if a value is removed.
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'));
}Re: TDD workshop. Anybody welcome; Data storage abstraction.
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
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);
}
- 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.
Thinking too far aheadmatthijs wrote:But how are we supposed to remove something if the Datastore class has no method to remove?![]()
ExcellentAnyway, I wrote this to make your test passCode: Select all
// class DataStore public function has($key){ return array_key_exists($key, $this->_store); }
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 182 times
Re: TDD workshop. Anybody welcome; Data storage abstraction.
First I need to know: are we removing from the dataStore or from the StorageModel?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.
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]);
}
- 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.
Removing from the DataStore. StorageModel is just the persistence layer; it won't be removed from that persistent storage until save() is called.matthijs wrote:First I need to know: are we removing from the dataStore or from the StorageModel?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.
If it is the first, this makes the test passCode: 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]); }
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.
Ok, now I understand what the purpose is.Removing from the DataStore. StorageModel is just the persistence layer; it won't be removed from that persistent storage until save() is called.
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..This whole "one assertion per test method" idea some people have is a good rule-of-thumb, but not a hard fact.
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?