Page 1 of 2
Should I Test Factories?
Posted: Sat Jan 13, 2007 5:27 pm
by Ollie Saunders
Is it worth testing factories? This particular factory does quite a lot, some of its methods involve creating as many as 5 or 6 objects and sending parameters to the correct objects but nonetheless it is doing no actual processing of its own.
I'm aware that you don't test getters and setters and I think that is a great principle. Could you imagine the code bloat involved in testing all those! Part of me thinks a factory testing should just be an extension of that principle. Indeed a testing of a factory would just be lots of assertInstanceOf checking the right objects are created and assertEqual checking the right parameters have been sent to the right objects....In my eyes this is practically like writing the factory twice and pretty pointless. Do you agree?
I think a crucial turning point is where conditions are involved. If my factory is creating different objects depending on something then I would write a test for that, otherwise is it necessary?
Posted: Sat Jan 13, 2007 5:31 pm
by Chris Corbyn
I think about one thing when making such a decision:
1. Is it apprently obvious what's going to happen?
In this case, it sounds as though it's worth testing because it's not just a one-line factory returning new Foo() or whatever. But generally, for basic "convenience" factories I wouldn't bother testing no.
Posted: Sun Jan 14, 2007 7:28 am
by Maugrim_The_Reaper
A Factory, can be simple or complex. The more complex steps it involves, the more likely something can go wrong, and therefore the more inclined you should be to test it so as to reduce the risk of missing future issues.
The simplest test would be to call the Factory with some arbitrary parameters, try to ensure the Factory always returns objects of the same type, and then check the resulting instance is of that type. Since this is a type check, it assumes all resulting objects either extend a common parent class or abstract, or implement a common interface - i.e. you can type check whichever of these is the lowest common type.
If the Factory is returning objects of differing type - say you have a Factory which returns two completely different objects which do *not* share the same interface then its slightly different but still doable. Possibly that scenario is more of the Abstract Factory type pattern...
Posted: Sun Jan 14, 2007 8:24 pm
by Ollie Saunders
I'm possibly just posting this to show off but

this factory is immense! So far it makes containers (fieldsets in HTML speak). It's refactored so adding creation of different objects won't double up the SLOC or anything like that. But still....9 requires and 57 lines to make one thing, I think that's a personal best.
Code: Select all
<?php
require_once 'Osis/Form/Validation/Object.php';
require_once 'Osis/Form/Validation/Callback.php';
require_once 'Osis/Form/AttachmentSet.php';
require_once 'Osis/Form/Attachment/Basic.php';
require_once 'Osis/Form/Attachment/Label.php';
require_once 'Osis/Form/Html/Renderer/Attachment/Label.php';
require_once 'Osis/Form/Container.php';
require_once 'Osis/Form/Html/Renderer/Container.php';
require_once 'Osis/Form/Retriever/Container.php';
class Osis_Form_Html_Factory
{
private $_validationStyle = Osis::VAL_STYLE_OBJECT;
public function setValidationStyle($validationStyle)
{
// MB safe - class names only container single byte characters
$this->_validationStyle = 'Osis_Form_Validation_' . ucfirst($validationStyle);
}
protected function _mkId($id)
{
return new Osis_Form_id($id);
}
protected function _mkAttachmentSet($label, $tooltip)
{
$aSet = new Osis_Form_AttachmentSet();
$aSet->label = $this->_mkAttachmentLabel($label);
$aSet->tooltip = new Osis_Form_Attachment_Basic($tooltip);
return $aSet;
}
protected function _mkAttachmentLabel($label)
{
$label = new Osis_Form_Attachment_Label($label);
$label->setRenderer(new Osis_Form_Html_Renderer_Attachment_Label());
return $label;
}
protected function _mkValidation()
{
return new $validationStyle;
}
public function mkContainer($id = null, $label = null, $tooltip = null, $invisible = false, $class = 'OF')
{
$cont = new Osis_Form_Container($this->_mkId($id));
$cont->setAttachmentSet($this->_mkAttachmentSet($label, $tooltip));
$cont->setRenderer($renderer = new Osis_Form_Html_Renderer_Container());
if ($invisible) {
$renderer->setDirective('parts', Osis::PART_CONTENT);
}
$cont->setValidation($this->_mkValidation());
$cont->setRetriever(new Osis_Form_Retriever_Container());
$renderer->getNodeSet()->outer->getClass()->set($class);
return $cont;
}
}
So ummm, how
exactly should I test this?
Posted: Sun Jan 14, 2007 11:42 pm
by feyd
For given inputs, does it output the expected results. The same as any other testing.

Posted: Mon Jan 15, 2007 4:34 am
by Ollie Saunders
You are saying exactly what I expected you to but not what I hoped.
It's going to be a really boring job but I've got it to it

Posted: Mon Jan 15, 2007 9:06 am
by Maugrim_The_Reaper
Afraid so

.
Look on the bright side, 3 months from now you or someone else will add a really stupid mistake when editing that creature. Your unit test will find it almost immediately. You'll think nothing of it at the time. Without the test, you'd be thinking of nothing else for a long time until you found the bug...

.
Posted: Mon Jan 15, 2007 1:57 pm
by Ambush Commander
That's why you should write the unit tests first.

Posted: Mon Jan 15, 2007 2:21 pm
by Christopher
My question is: why is it a Factory in the first place? Being difficult to test is a code smell. It looks like it should be four classes that can be completely instantiable via their constructors. For example:
Code: Select all
class Osis_Form_Container {
public function __construct($id = null, $label = null, $tooltip = null, $invisible = false, $class = 'OF', $validationStyle=Osis::VAL_STYLE_OBJECT)
{
$cont = new Osis_Form_Container(new Osis_Form_id($id));
$cont->setAttachmentSet(new Osis_Form_AttachmentSet($label, $tooltip));
$cont->setRenderer($renderer = new Osis_Form_Html_Renderer_Container());
if ($invisible) {
$renderer->setDirective('parts', Osis::PART_CONTENT);
}
$cont->setValidation(new Osis_Form_AttachmentSet($validationStyle));
$cont->setRetriever(new Osis_Form_Retriever_Container());
$renderer->getNodeSet()->outer->getClass()->set($class);
}
}
Posted: Mon Jan 15, 2007 8:39 pm
by Ollie Saunders
Maugrim wrote:Look on the bright side, 3 months from now you or someone else will add a really stupid mistake when editing that creature. Your unit test will find it almost immediately. You'll think nothing of it at the time. Without the test, you'd be thinking of nothing else for a long time until you found the bug.
Yes you are right and this is exactly what I've got to keep telling myself. With any luck I will be in a situation where people are contributing to this code and making changes so yeah. Thanks Maugrim.
AC wrote:That's why you should write the unit tests first.
In this case I think it would have been harder that way round. I need to write the actual code so as to prompt my brain for everything that needed to be done, just writing a series of test wouldn't have done that. Stupid brain.
Yes it is. But the factory isn't difficult to test there's just a lot to test and yes it was boring to write

. The test is actually very simple:
Code: Select all
public function testMkContainer()
{
$exp = array();
$cont = $this->inst->mkContainer(
$exp['id'] = uniqid(),
$exp['label'] = uniqid(),
$exp['tooltip'] = uniqid(),
false,
$exp['class'] = uniqid()
);
$this->assertIsA($cont, 'Osis_Form_Container');
$this->assertIsA($ren = $cont->getRenderer(), 'Osis_Form_Html_Renderer_Container');
$this->assertIsA($atch = $cont->getAttachmentSet(), 'Osis_Form_AttachmentSet');
$this->assertIsA($atch->label, 'Osis_Form_Attachment_Label');
$this->assertIsA($atch->label->getRenderer(), 'Osis_Form_Html_Renderer_Attachment_Label');
$this->assertIsA($atch->tooltip, 'Osis_Form_Attachment_Basic');
$this->assertIsA($atch->errors, 'Osis_Form_Attachment_MsgSet');
$this->assertIsA($atch->errors->getRenderer(), 'Osis_Form_Html_Renderer_Attachment_MsgSet');
$this->assertIsA($cont->getValidation(), 'Osis_Form_Validation_Object');
$this->assertEqual($cont->getId(), $exp['id']);
$this->assertEqual($atch->label->getLabel(), $exp['label']);
$this->assertEqual($atch->tooltip->get(), $exp['tooltip']);
$this->assertEqual($ren->getNodeSet()->outer->getClass()->__tostring(), $exp['class']);
$this->assertEqual($ren->getDirective('parts'), Osis::PART_ALL);
$cont = $this->inst->mkContainer(null, null, null, true);
$this->assertEqual($cont->getRenderer()->getDirective('parts'), Osis::PART_CONTENT);
}
it didn't take as long as I expected actually.
arborint wrote:t looks like it should be four classes that can be completely instantiable via their constructors.
I don't mean to offend arborint, but you couldn't be more wrong. In OO you should encapsulate what changes. The way in which a container is build is a
very changeable thing in this case. If all that stuff was lumped in there I would be seriously restricted. One example: this is an HTML factory but there could be, HTML5 or XForms factories in future and without being able use abstract classes and inheritance I would have a real maintenance problem with your suggestion.
Posted: Mon Jan 15, 2007 8:42 pm
by Ambush Commander
They are two different ways of looking at the problem: neither is totally sufficient. When I write unit tests, I discover some implementation quirks (usually regarding the interface but also regarding scope of the function) that I have to iron out, when I write the actual code, I realize there are more edge cases that I have to put into my unit tests. If you, the coder, can't wrap your head around the expected API and dependencies, how should anyone else be reasonably expected to do so either?
Posted: Mon Jan 15, 2007 9:07 pm
by Ollie Saunders
Ambush Commander wrote:They are two different ways of looking at the problem: neither is totally sufficient. When I write unit tests, I discover some implementation quirks (usually regarding the interface but also regarding scope of the function) that I have to iron out, when I write the actual code, I realize there are more edge cases that I have to put into my unit tests.
Yes I experience exactly the same; if I am understanding you correctly.
If you, the coder, can't wrap your head around the expected API and dependencies, how should anyone else be reasonably expected to do so either?
Can't say I agree with this. Bugs arise in code often through oversights of our own APIs. Also who says I can't wrap my head around it? I understand it perfectly, it is completely logical. The only problem is that it is also extensive, there is quite a lot to remember. In previous versions of this library I made simplicity of API the driving force of the design. That was a terrible terrible move. I centralized things and put properties and behaviour in objects where it didn't belong so as to make it accessible. And doing so led to failure.
I think in my time writing this library over and over with increasing quality each time I've learnt as much about
the importance of object orientated principles as what they actually are. APIs should be motivated by logical, maintainable design and nothing else. If you end up with a complex API at the end, that's probably because its solving a complex, multi-faceted problem. And that there is the reason why nobody has ever written a decent forms library before. It's really surprisingly difficult. So difficult in fact, it took me 4, munge fuddling, sock cucking, dis jipping, times to get it right.
Sidenote: The first 2 writes, I also didn't have a clue what I was doing
which didn't help much either.
Posted: Mon Jan 15, 2007 11:41 pm
by Christopher
ole wrote:I don't mean to offend arborint, but you couldn't be more wrong. In OO you should encapsulate what changes. The way in which a container is build is a very changeable thing in this case. If all that stuff was lumped in there I would be seriously restricted. One example: this is an HTML factory but there could be, HTML5 or XForms factories in future and without being able use abstract classes and inheritance I would have a real maintenance problem with your suggestion.
No offense taken, but it still looks to me like you have crammed a bunch of Builder pattern classes into a Factory-like thing ... and that is maybe why it is difficult to test.
Also, I have never heard that "
In OO you should encapsulate what changes." I'm not sure I really understand that ... to minimize side-effects?
Posted: Tue Jan 16, 2007 5:06 am
by Ollie Saunders
No offense taken, but it still looks to me like you have crammed a bunch of Builder pattern classes into a Factory-like thing ... and that is maybe why it is difficult to test.
Hmm this may be a builder more than it is a factory in fact. In fact, I've just realised it definitely is. Crap, I'm going to have to change the name of this thing.
Also, I have never heard that "In OO you should encapsulate what changes." I'm not sure I really understand that
OK, put it another way, this seems to be more common: -
OO Design Principles wrote:Single Responsibility Principle
A class should have only one reason to change
Posted: Tue Jan 16, 2007 7:30 am
by Maugrim_The_Reaper
I guess there's some terminology you've missed, arborint, in your OOP reading...

You do know this of course... Also called the "concept that varies" rule which is likely the more common use term...
When you finish kicking youself...