[dev] [commits] Horde branch master updated. 3d5433806251d1e14d73fb2f5abf8347cf07389d

[Gunnar Wrobel]

Quoting Michael M Slusarz <slusarz at horde.org>:


> -----------------------------------------------------------------------
>
> commit 21d7218dc143d6f728c6d2f833e08c2c5e833cce
> Author: Michael M Slusarz <slusarz at curecanti.org>
> Date:   Fri Sep 10 22:38:37 2010 -0600
>
>     Use PHPUnit mock function, rather than static mock files
>
> imp/lib/tests/Imp/Autoload.php                  |    7 -
> imp/lib/tests/Imp/Stub/Browser.php              |   41 -------
> imp/lib/tests/Imp/Stub/Identity.php             |   73 ------------
> imp/lib/tests/Imp/Stub/Injector.php             |   46 -------
> imp/lib/tests/Imp/Stub/Prefs.php                |   41 -------
> imp/lib/tests/Imp/Stub/Registry.php             |   48 --------
> imp/lib/tests/Imp/Unit/Mime/Viewer/ItipTest.php |  145  
> +++++++++++++++++++++--
> 7 files changed, 135 insertions(+), 266 deletions(-)
> delete mode 100644 imp/lib/tests/Imp/Stub/Browser.php
> delete mode 100644 imp/lib/tests/Imp/Stub/Identity.php
> delete mode 100644 imp/lib/tests/Imp/Stub/Injector.php
> delete mode 100644 imp/lib/tests/Imp/Stub/Prefs.php
> delete mode 100644 imp/lib/tests/Imp/Stub/Registry.php
>
> http://git.horde.org/diff.php/imp/lib/tests/Imp/Autoload.php?rt=horde-
> git&r1=edfe90f9ffebf48ea8f9bbd5b98aadc4e0cb6275&
> r2=21d7218dc143d6f728c6d2f833e08c2c5e833cce
> http://git.horde.org/diff.php/imp/lib/tests/Imp/Stub/Browser.php?rt=horde-
> git&r1=f57afbbc344062cc626d2f80fbe2f541f144559b&
> r2=21d7218dc143d6f728c6d2f833e08c2c5e833cce
> http://git.horde.org/diff.php/imp/lib/tests/Imp/Stub/Identity.php?rt=horde-
> git&r1=f57afbbc344062cc626d2f80fbe2f541f144559b&
> r2=21d7218dc143d6f728c6d2f833e08c2c5e833cce
> http://git.horde.org/diff.php/imp/lib/tests/Imp/Stub/Injector.php?rt=horde-
> git&r1=f57afbbc344062cc626d2f80fbe2f541f144559b&
> r2=21d7218dc143d6f728c6d2f833e08c2c5e833cce
> http://git.horde.org/diff.php/imp/lib/tests/Imp/Stub/Prefs.php?rt=horde-
> git&r1=f57afbbc344062cc626d2f80fbe2f541f144559b&
> r2=21d7218dc143d6f728c6d2f833e08c2c5e833cce
> http://git.horde.org/diff.php/imp/lib/tests/Imp/Stub/Registry.php?rt=horde-
> git&r1=d5b189c9f96492312380fb5b9795fa98397adfa1&
> r2=21d7218dc143d6f728c6d2f833e08c2c5e833cce
> http://git.horde.org/diff.php/imp/lib/tests/Imp/Unit/Mime/Viewer
> /ItipTest.php?rt=horde-git&r1=666fd2783ad1e287962493b5fa58c971352e555d&
> r2=21d7218dc143d6f728c6d2f833e08c2c5e833cce

Why do you prefer the mocks here? I do admit that the stubs I wrote  
were really simple and replacing them with mocks probably makes sense  
in the context of this single test.

But in my experience test mocks just don't work well in the long term.  
For one thing they are extremely hard to read. I believe some people  
recently wrote alternative mock implementations for PHPUnit that solve  
this problem at least. But I know from experience that all these mock  
expectations are hard to maintain. They end up all over the test  
suites and if you are changing anything about the mocked interface it  
gets hard to adapt all those calls. In order to understand the mock  
expectations you often need to look into the corresponding code to see  
why some of these are actually needed. So mocking will often tie your  
test to the actual implementation.

As the Itip test was the first application level unit test I added for  
Horde the stubs were still residing within IMP. The plan would have  
been to move these into the "Test" package at some point and extending  
the stubs into a general replacement for "the real thing" when writing  
application level unit tests. So you would have a stub registry  
implementation that would allow access to and manipulation of all the  
features provided by the real registry.

This approach also has some drawbacks:

For one thing these are not really unit tests anymore but tend to be  
integration tests (without any access to external systems though). But  
to me this makes sense for the application level where it seems  
reasonable to define a test that reads "user clicks a, user clicks b,  
event got saved to storage".

Another thing is the complexity of the stubs: Once these get closer to  
the real thing it is harder to actually change them as they may easily  
affect hundreds of tests. But on the other hand this can actually be  
quite useful: Horde_Registry is rather central so it also cannot be  
easily changed. If it needs modification though you would adapt the  
stub implementation, too. The fact that this stub is used in many  
places where the registry is required should help in finding spots  
that still need to be adapted to the change in the real registry.

Last but not least these tests usually turn out to run a lot slower as  
more elements are involved in testing. Not good but hard to change.  
And something I tend to accept for the benefit I see in basing our  
application level testing on stubs.

In summary may not see much of a problem with the one commit I  
commented here. For me the main question is how others view this topic  
and which style of testing they'd prefer. I have seen the mock based  
approach fail horribly in the past half year and that is why this  
issue is important to me.

Cheers,

Gunnar