cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ugo Cei <u....@cbim.it>
Subject Making tests suck less (was Re: ECM++)
Date Fri, 22 Oct 2004 16:55:05 GMT
I wrote:
> Currently, all the unit tests involving sitemap components fail because 
> the class org.apache.cocoon.components.ExtendedComponentSelector, which 
> is referenced by *.xtest files, has been removed.
> 
> Now, those tests depend on the deprecated ExcaliburTestcase class and it 
> would be nice to get rid of that dependency, but as a stopgap measure, 
> is there a class in ECM++ that can take its place just by substituting 
> the classname in its place?

Well, after thinking about the issue some more, I decided to rewrite all 
the sitemap components' testcases following a different approach. I 
intend to drop the SitemapComponentTestCase altogether and its 
dependency on ExcaliburTestCase with it. First, what are the problems 
with it?

1. Dependency on a deprecated class.
2. Writing tests is harder than it should be, with all those *.xtest files.
3. LAST BUT NOT THE LEAST: mostly we are not testing what we should be 
testing. An example will clarify what I mean:

== ResourceExistsActionTestCase.java ==

public void testExistAction() throws Exception {
 

         String src = 
"resource://org/apache/cocoon/acting/ResourceExistsActionTestCase.xtest";
         Parameters parameters = new Parameters();
 

         Map result = act("exist", src, parameters);
         assertNotNull("Test if resource exists", result);
 

         src = 
"resource://org/apache/cocoon/acting/ResourceExistsActionTestCase.abc";

	...
}

=====

This testcase is supposed to test the following method:

== ResourceExistsAction.java ==

     public Map act(Redirector redirector, SourceResolver resolver, Map 
objectModel, String src, Parameters parameters) throws Exception {
         String resourceURI = parameters.getParameter("url", src);
         Source source = null;
         try {
             source = resolver.resolveURI(resourceURI);
             if (source.exists()) {
                 return EMPTY_MAP;
             }
         } catch (SourceNotFoundException e) {
             // Do not log
         } catch (Exception e) {
             getLogger().warn("Exception resolving resource " + 
resourceURI, e);
         } finally {
             if (source != null) {
                 resolver.release(source);
             }
         }
         return null;
     }
====

But does it? What it does is test the SourceResolver.resolveURI method! 
I would expect that the SourceResolver class was already bug-free and 
had its own tests. There's no point testing that.

What should we be testing instead? Everything that can possibly go 
wrong. What can go wrong here? Well, for instance, the programmer might 
have forgotten to relase the source. We should be testing that the 
source is always released. There's not much else here that can go wrong.

How do we test for that? One possibility is to use mock objects and set 
expectations on them. We will give our action a mock SourceResolver 
which returns a mock source. On those objects, we set an expectation 
that the release method is indeed called, and verify that expectation.

Using the jMock library, the test looks like this (slightly edited to 
simplify):

         String src = "don't care";
         ResourceExistsAction action = new ResourceExistsAction();
         Mock resolver = new Mock(SourceResolver.class);
         Mock source = new Mock(Source.class);
         resolver.expects(once()).method("resolveURI").with(same(src)).
                 will(returnValue(source.proxy()));
 
resolver.expects(once()).method("release").with(same(source.proxy()));
 
source.expects(atLeastOnce()).method("exists").will(returnValue(true));
         Map result = action.act(null, (SourceResolver) resolver.proxy(),
                 objectModel, src, parameters);
         assertSame("Test if resource exists", AbstractAction.EMPTY_MAP, 
result);
         resolver.verify();
         source.verify();

If we forget to call the release method, the "resolver.verify()" call 
will fail. You will notice also that the value of the "src" parameter is 
totally irrelevant. We don't need the resolver to actually resolve the 
URI. We just need it to return any implementation of the Source 
interface and call "release" on that. One less test fixture to care of.

What we have gained is a less fragile, more relevant testcase.

In the coming days, I plan to rewrite all tests depending on 
ExcaliburTestCase so that we can forget about it. Stay tuned.

	Ugo

Mime
View raw message