Should I Test Factories?
Moderator: General Moderators
- Ollie Saunders
- DevNet Master
- Posts: 3179
- Joined: Tue May 24, 2005 6:01 pm
- Location: UK
Should I Test Factories?
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?
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?
- Chris Corbyn
- Breakbeat Nuttzer
- Posts: 13098
- Joined: Wed Mar 24, 2004 7:57 am
- Location: Melbourne, Australia
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.
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.
- Maugrim_The_Reaper
- DevNet Master
- Posts: 2704
- Joined: Tue Nov 02, 2004 5:43 am
- Location: Ireland
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...
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...
- Ollie Saunders
- DevNet Master
- Posts: 3179
- Joined: Tue May 24, 2005 6:01 pm
- Location: UK
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.
So ummm, how exactly should I test this?
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;
}
}- Ollie Saunders
- DevNet Master
- Posts: 3179
- Joined: Tue May 24, 2005 6:01 pm
- Location: UK
- Maugrim_The_Reaper
- DevNet Master
- Posts: 2704
- Joined: Tue Nov 02, 2004 5:43 am
- Location: Ireland
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...
.
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...
- Ambush Commander
- DevNet Master
- Posts: 3698
- Joined: Mon Oct 25, 2004 9:29 pm
- Location: New Jersey, US
- Christopher
- Site Administrator
- Posts: 13596
- Joined: Wed Aug 25, 2004 7:54 pm
- Location: New York, NY, US
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);
}
}(#10850)
- Ollie Saunders
- DevNet Master
- Posts: 3179
- Joined: Tue May 24, 2005 6:01 pm
- Location: UK
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.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.
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.AC wrote:That's why you should write the unit tests first.
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
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);
}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.arborint wrote:t looks like it should be four classes that can be completely instantiable via their constructors.
- Ambush Commander
- DevNet Master
- Posts: 3698
- Joined: Mon Oct 25, 2004 9:29 pm
- Location: New Jersey, US
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?
- Ollie Saunders
- DevNet Master
- Posts: 3179
- Joined: Tue May 24, 2005 6:01 pm
- Location: UK
Yes I experience exactly the same; if I am understanding you correctly.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.
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.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?
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
- Christopher
- Site Administrator
- Posts: 13596
- Joined: Wed Aug 25, 2004 7:54 pm
- Location: New York, NY, US
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.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.
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?
(#10850)
- Ollie Saunders
- DevNet Master
- Posts: 3179
- Joined: Tue May 24, 2005 6:01 pm
- Location: UK
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.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.
OK, put it another way, this seems to be more common: -Also, I have never heard that "In OO you should encapsulate what changes." I'm not sure I really understand that
OO Design Principles wrote:Single Responsibility Principle
A class should have only one reason to change
- Maugrim_The_Reaper
- DevNet Master
- Posts: 2704
- Joined: Tue Nov 02, 2004 5:43 am
- Location: Ireland