More than year ago I was assigned to different project. To my delight team was writing not only unit tests, but also end-to-end tests. Every test class undeployed all plugins and components from server, reconfigured environment and deployed tested artefacts (not necessarily the same combination). Fantastic, tests were self-contained and independent.

But there were problems. Tests were slow and redeployment step was unnecessary in many cases, because all needed artefacts for subsequent test were already deployed and no changes were actually needed. Additionally all tests performed test setup in the old fashioned procedural way, describing in all gory details how to achieve desired state.

It could look like following code (comments are place holders for real functionality I am not going to show here).

@BeforeClass public static void beforeClass() {
    TestingUtil util = // get it ...

    util.undeployAllComponents();
    util.undeployAllPlugins();

    // initialize Database
    util.deployPlugin("daos.jar");
    util.deployPlugin("router.jar", routerParams);
    // some necessary tasks
    util.deployComponent("authenticator.jar");
    util.deployComponent("decisions.jar");
  
    User userA = // prepare user profile
    // ...
    // save user profiles
}

Instead of comments there were many lines of other code. Number of deployed components was varying between tests and also their order was not always the same (when it did not matter). Additionally there were 2 environments thus plugin router was usually unclosed with if-else together with varying DB setup. It was rather hard to track down what exactly is going on, but after some time we got used to it. If only it was not duplicated 17 times and every time it looked differently (nearly every test tested different component, but nearly all components depended on other components). Occasionally a new dependency was introduced and we needed to modify all those tests and add new one in correct place, unless it was not needed for particular test. It was messy.

Solution

I don't remember any more if I was begging our team leader to give me time to fix it or he asked me to do so, but I got the chance to find correct abstraction for this problem and to remove duplication.

I've realized our existing approach mixed together:

• dependency management
• description of scenario being tested
• deployment tasks
• varying environment
• preparation of data in database

So I have created following:

• Artefact knows what it is (plugin, component) and what other artefacts it depends on. There are subclasses for every component and plugin in order to encapsulate and reuse that knowledge.
• Scenario collects Artefacts that need to be deployed and can iterate through them in different ways (in deployment order, in undeployment order). It got more responsibilities later so I have split it to 2 separate classes.
• ScenarioDeployer is responsible for coordination of whole redeployment of scenario
• Deployer knows what environment it is deploying to and how to perform it.
• A hook for inserting custom code (e.g. for DB) between components are undeployed and deployed.

I have decided to push as much responsibilities as I could into individual objects. In this case the key was to give Artefact responsibilities of knowing what it depends on and how to deploy itself (by delegating to Deployer). Alternative with testing superclass having different methods doing deployment led to original mess. Alternative with static methods would fail too. What we've got was following:

@BeforeClass public static void beforeClass() {

    // initialize Database
    Scenario scenario = new Scenario();
    scenario.add(new AuthenticatorComponent());
    scenario.add(new DecisionsComponnent());
    createScenarioDeployer().redeploy(scenario);
  
    User userA = // prepare user profile
    // ...
    // save user profiles
}

Notice it does not mention anything about how to undeploy artefacts or which ones to undeploy. It will happen inside redeploy() and it allows for any changes in implementation as needed. It also mentions only top level artefacts (those that are tested by this test). All others, needed as dependency do not need to be mentioned, they are implicit and will be resolved.

Real solution is a bit more complex than this example, but original pseudo code I have shown does not show everything too. Main point is it goes directly to essence of scenario setup - it shows nicely what we want to use in test instead of obscuring it with details how it is achieved. There are, however, a few even more important things that are not shown:

• It was easy to achieve optimization - to undeploy only unwanted artefacts, deploy missing ones and redeploy those whose dependency changed, everything in correct order. All without touching beforeClass at all. This was possible only because it was not done explicitly, but hidden inside of redeploy().
• whole code together with optimization was heavily unit tested, so I could do changes confidently, while original duplicated test setup was tested only indirectly through tests it was meant to configure. (To be fair, there was one bug found a year later - optimization did not work effectively under all circumstances).
• Less than 1 month later after introducing it, our team leader realized we can use whole framework and existing artefact implementations in installer, we just need to provide slightly modified Deployer.
• When I returned back from my Round The World travelling I found the framework was moved from our project into shared space and it is used by other projects. Currently we are thinking we will use it for yet another unforeseen application, but it is too soon to be sure. But I think it should be possible just by providing another implementation of Deployer.

Conclusion

I have hinted in Solution that duplication of code usually means you are missing some abstraction. If you find it and make explicit you might save a lot of pain you had before, because information hiding allows you to introduce new approaches or enhancements cheaply.

With correct abstraction and well distributed responsibilities you might be surprised how your piece of code will get reused. In early 90s everybody hoped for OO languages being a solution for reusable code, but somehow it seemed OO could not deliver. IMHO it was always problem with badly assigned responsibilities (and misused inheritance). This claim could be slight simplification, but there is a lot of truth in it.

I wish I was there at the time it was decided it will be published to wider audience. I have some suspicion I have made not-so-good-decision-after-all with how Artefact handles equality. It is OK for my original task, but with wider usage it seems to be restrictive and confusing. And it is too late to change now after it is published contract ;-)

I am posting this because I was not able to find anything on Net. Even I consider it now to belong into category obvious.

I am working on obfuscation of our project and was bitten by following exception when trying to use the obfuscated JAR:

java.lang.ClassFormatError: StackMapTable format error: bad class index

StackMapTable is feature of Java to provide faster class verification and you cannot configure Klassmaster what to do about it as you can with any proper class.

Eventually I found it works when built locally, but fails with version built by a build server. That helped me to find the culprit.

Solution

We were using old version of Zelix Klassmaster on server due to incorrect default home directory of Klassmaster. After changing path to use version 5.3 it works. It works also with 5.2.4b.

I am slowly getting through Domain Specific Languages by Martin Fowler. I really enjoy it as I expected I will. Apart of the main theme - building DSLs, their parsing and processing - it is full of little gems, making it much more useful than I would imagine before. But I would not expect anything less from Martin Fowler.

I have done a few internal DSLs before, always thinking external DSLs could be more useful for developers "stuck" in Java world, where internal DSLs are rather limited in way they can look like and when they can be processed (compilation). The big plus of this book is in that it shows ANTLR can be useful and is not very complicated if you don't want to build Turing complete language. A point of this exercise is to check how simple is to use it.

I was not able to make up a nice example, but this book provided a help also here. I really liked the example showing internal DSL for access control list. It reminded me something I was working years ago, but Martin solved it in much better way. I don't mean a usage of DSL for configuring ACL, but his semantic model and a way it processes security checks - by usage of Specification pattern. He provides several implementations (Java, Ruby) and I've decided to base the format of my external DSL on his internal DSL in Ruby to serve as example in Java.

I don't want to repeat his work here, so I will show only text I want to parse, the grammar and how to load it. I will skip creation of semantic model, because it would be ripped of the book directly and I don't want to do that, not to mention it would not be useful for my experiment. I don't want to hone my copy&pasting skills here :-) Instead I will only log information parsed from the file.

What I want to parse

As I said he uses following configuration in example, I've just removed ':' forming symbols.

The point is to list rules allowing or disallowing access to some resource based on when a person does it and to which department they belong.

allow {
   department mf
   ends 2008, 10, 18
}
refuse department finance
refuse department audit
allow {
  gradeAtLeast director
  during 1100, 1500
  ends 2008, 5, 1
}
refuse {
  department k9
  gradeAtLeast director
}
allow department k9

Initial grammar that parses the configuration

grammar Acl;

acl	:	(allows | refuses)+;
allows	:	'allow' what;
refuses	:	'refuse' what;

what	:	condition | '{' condition+ '}';

condition :	department | ends | grade_at_least | during;
	

department : 'department' ID;
	
ends	:	'ends' INT ',' INT ',' INT;

grade_at_least : 'gradeAtLeast' ID;
	
during	:	'during' INT ',' INT;

	
ID  :	('a'..'z'|'A'..'Z'|'_') ('a'..'z'|'A'..'Z'|'0'..'9'|'_')*;

INT :	'0'..'9'+;

WS  :   (' ' | '\t' | '\r' | '\n' ) + {skip();};

ILLEGAL	: .;

Token rules ID and INT were generated by ANTLWorks. Token rules WS and ILLEGAL come from the book. ILLEGAL is about parser stopping when no other rule matches. Also the usage of EOF in the top most rule was described in the book. Rest of it is my own creativity. I think it is not very complicated or hard to read for now. It will get more complicated shortly when I will add some clutter.

Initial loader

I would like to write a piece of code that will invoke generated parser and reads the file. Again, this is taken directly from the book, there is not much point to do it differently.

public class AclLoader {
    private Reader input;
    private AclParser parser;

    public AclLoader(Reader input) {
        this.input = input;
    }

    public void load() {
        try {
            AclLexer lexer = new AclLexer(new ANTLRReaderStream(input));
            parser = new AclParser(new CommonTokenStream(lexer));
            parser.acl();
        } catch (Exception e) {
            throw new RuntimeException(e);
        }
    }
}

AclLexer and AclParser were generated by ANTLR from my grammar. It should parse the file, but it does nothing with it yet. Let's add some tests.

public class AclLoaderTest {
    @Test
    public void readsValidFile() throws IOException {
        Reader input = new FileReader("resources/acl.txt");
        AclLoader loader = new AclLoader(input);
        loader.load();
    }

    @Test(expected = RuntimeException.class)
    public void failsForInvalidFile() throws IOException {
        Reader input = new FileReader("resources/acl-bad.txt");
        AclLoader loader = new AclLoader(input);
        loader.load();
    }
}

I am bit confused with the 2nd one - it fails, because invalid file is not refused. Hmm, I've read something about errors, in the book. Let's review it...

Error handling

I forgot to add reporting of errors to my parser. After seeing advantages in the book I am going to add superclass for parser (so that I can add custom code to parser without being afraid ANTLR will delete it next time I will generate the parser. It will be useful later when processing output from parser too. This is taken directly from the book, but simplified for my needs.

public abstract class BaseAclParser extends Parser {
    private List errors = new ArrayList();

    public BaseAclParser(TokenStream input, RecognizerSharedState state) {
        super(input, state);
    }

    public void reportError(RecognitionException e) {
        errors.add(e);
    }

    public boolean hasErrors() {
        return !errors.isEmpty();
    }

    public String getErrorReport() {
        StringBuffer result = new StringBuffer();
        for (Object e : errors) result.append(e).append("\n");
        return result.toString();
    }
}

I have to tell ANTLR to use it. I am going to modify beginning of my grammar file:

grammar Acl;
options {superClass = BaseAclParser;}
...

Finally to check for errors at the end of parsing in AclLoader (again taken from the book).

if (parser.hasErrors()) throw new RuntimeException("Loading failed: "+parser.getErrorReport());

Now both tests are passing, but I do not like error message I am getting. I don't want to spend time with cryptic error messages now, so I will ignore it.

Experiments with processing content

My parser seems to be reading correct configuration file, but it ignores everything it learns there. It is time to process that information. Now it will get a bit messy, but I hope I will make it (really, I have not done it myself yet - let's go back to the book to reread particular chapter so I can pretend I am smart again).

ANTLR allows to invoke custom code and I am going to try simple thing to see if it works as expected. I modify the grammar like follows:

allows	:	'allow' {allowBlock();} what;

That should invoke method allowBlock in BaseAclParser when 'allow' is parsed, but before parsing of conditions starts. Let's add simple implementation of allowBlock:

    protected void allowBlock() {
        System.out.println("allowBlock");
    }

As I said in the beginning I am not going to build semantic model, just use logging instead. If I wanted to do something more serious I would set Context Variable here, or alternatively I could propagate complete information about parsed text from inner rules here, but that would get pretty messy. I do similar thing for 'refuse' block:

refuses	:	'refuse' {refuseBlock();} what;

BaseAclParser:

    protected void refuseBlock() {
        System.out.println("refuseBlock");
    }

Processing conditions

Analogicaly to previous code I modified the grammar to inform parser about parsed content. Any time a rule about condition is successfuly parsed it invokes custom method in BaseAclParser that processes it.

grammar Acl;
options {superClass = BaseAclParser;}

acl	:	(allows | refuses)+ EOF;
allows	:	'allow' {allowBlock();} what;
refuses	:	'refuse' {refuseBlock();} what;

what	:	condition | '{' condition+ '}';


condition : department | ends | grade_at_least | during;
	

department : 'department' depName=ID
                {departmentCondition($depName.text);};
	
ends : 'ends' year=INT ',' month=INT ',' day=INT
          {endCondition($year.text, $month.text, $day.text);};

grade_at_least : 'gradeAtLeast' grade=ID 
                    {gradeAtLeastCondition($grade.text);};
	
during	: 'during' after=INT ',' before=INT 
             {duringCondition($after.text, $before.text);};

	
ID  :	('a'..'z'|'A'..'Z'|'_') ('a'..'z'|'A'..'Z'|'0'..'9'|'_')*;

INT :	'0'..'9'+;

WS  :   (' ' | '\t' | '\r' | '\n' ) + {skip();};

ILLEGAL	: .;

Of course, processing in my case means just logging it:

    protected void departmentCondition(String name) {
        System.out.println("\tdepartmentCondition: " + name);
    }

    protected void endCondition(String year, String month, String day) {
        System.out.println("\tendCondition: "+year + "-" + month + "-" + day);
    }

    protected void gradeAtLeastCondition(String grade) {
        System.out.println("\tgradeAtLeastCondition: " + grade);
    }

    protected void duringCondition(String after, String before) {
        System.out.println("\tduringCondition: <" + after + ", " + before + ">");
    }

What I have got

And as the result I am getting following output:

allowBlock
	departmentCondition: mf
	endCondition: 2008-10-18
refuseBlock
	departmentCondition: finance
refuseBlock
	departmentCondition: audit
allowBlock
	gradeAtLeastCondition: director
	duringCondition: <1100, 1500>
	endCondition: 2008-5-1
refuseBlock
	departmentCondition: k9
	gradeAtLeastCondition: director
allowBlock
	departmentCondition: k9
allowBlock
refuseBlock
	departmentCondition: k9

It looks suspiciously similar to original file :-) but I am happy with outcome. I am going to play with it more, to add building of semantic model and to add more unit tests. It should be easy, because the book contains all code for semantic model, I need just adapt my hook methods. Unfortunately I don't think I can publish it here completely. It is coming from still unpublished book...

While I was wading through some tests recently I have found following piece of code. I thought you might enjoy it too.

    @BeforeClass
    public static void setUpBeforeClass() throws Exception
    {
        System.out.println("BeforeClass");
        main(null);
    }

    public void testDummy()
    {
        // do nothing
    }

    public static void main(String [] args) throws Exception
    {
        System.out.println("main");
        // a lot of code is here
    }

The point is that given test class is meant for setting up an environment and it does not contain any real test methods. It is just an inconvenient way to use convenience of JUnit for execution of some code.

Please, do not do these things!

About 2 weeks ago I had discussion with a few colleagues of mine. I have asked them what they think about following Javadoc and how to enhance it.

class Person
{
    // definition of attributes, I am not stating them to 
    // avoid attracting your attention now
 
    /**
     * Gets value of the first name property.
     * @return first name of person
     */
    public String getFirstName()
    {
        return m_FirstName;
    }
}

I have provided additional constraints:

• the class is as-is, we do not care about e.g. persistence (to avoid temptation to define maximum size).
• we do not care about constructor (or setter) and what constraints it brings.
• only this getter for Java Bean interests us here as my idea is to represent any simple method in general.
• do not speculate what else could be done in this method, just provide useful Javadoc.
• I have selected Person and first name as widely understood concept to make it harder to provide useful Javadoc (in order to shed more light into this apparently confusing concept ;-) )

As I expected I did not receive any useful (and in my opinion valid) suggestion. Following was suggested for example:

• "Maybe format could be described" (there is none in my example).
• "In case of future enhancements (e.g. encryption) it is better to have Javadoc present since beginning, because some developers will not add one when they are modifying it" (I have simplified this suggestion to make it shorter. Well, I am not sure if a group of developers who would not modify existing Javadoc is not bigger...)

I have got also useful suggestion: "I think that javadocs should not add anything unnecessary (over restrictive or superfluous or presumptuous)." Exactly that's my idea! What could Javadoc add in this case that is not already described what we have learned about in kindergarten? Yet, I have to provide one otherwise our Checkstyle will protest.

As is written in book Clean Code:

Comments are always failures. We must have them because we cannot always figure out how to express ourselves without them, but their use is not a cause for celebration.

I am not as strict about this (I am probably not able to express all my ideas cleanly often), but in general I agree that strict asking for Javadoc is healing symptoms instead of real root cause in typical code. Are you not persuaded yet? Well, what do you think about following example describing common usage of Logger?

/** Logger */
private static final Logger LOG = Logger.getLogger(Person.class);

While it is probable that there is only one Person.getFirstName() in codebase and it deserves some kind of documentation, what should I think about comment describing well known way how logger is used in about 2000 classes I have seen in our project till now?

Conclusion

I wanted to make title of this post attractive... I generally consider Javadoc to be useful. But I wanted to point out that sometimes there are valid reasons to skip it. Unfortunately Checkstyle is not able to identify these situations while people are.

I had interesting mail exchange with my colleague Gilles today. It was about multithreading in Java and how hard is for people to buy into all this.

I would say that the biggest mistake in Java is that it made multithreading to look easy while it is not. Everybody can spawn new threads easily with copy&paste (in the worst case scenario ;-) ). But consequences of incorrect thread-safeness and ignored memory visibility are usually terrible. And it is even worse since introduction of multi core CPUs.

It was about 4 years ago when I was looking for bug in JBoss that caused that an instance of Entity Bean was not saved every 4-6 hours. It took me a week to track it down to incorrect synchronization - two places, one synchronized at instance other at class level. On other side it was rather relaxing - wait till it fails, go through logs, formulate new hypothesis, modify logging in sources, start and wait till another error occurs. Fine for me, but I am not sure about my employer :-) Unfortunately before I could send a patch it was already fixed, so no glory for me :-( ;-) . This example should show how hard it is to fix such a problem and that it happens also to experts occasionally.

Now back to knowledge about Java multithreading. Recently I published my list of influential books. It did not contain anything about particular technology, but I should add following: Brian Goetz: Java Concurrency in Practice. Everybody programming in Java professionally should know its content by heart. There is no better book about this topic.

I believe it is only fair to downgrade to Junior all Senior Java developers not being able to explain how they use multithreading correctly.

Recently I have created 2 tests for two classes that looked very similar. Well, they were same apart for testing different subclasses of the same parent. Indeed, tested classes were also similar. I have to fix this (on both sides if possible).

I have found this problem when I wanted to introduce new method to interface and was adding test for it. So my first idea was to reuse test implementation for both classes by creating abstract test and two subclasses handling different initialization of tested classes (different class to instantiate and also different colaborators). Problem is that these two classes together with their tests are not located in the same module. Natural place for abstract test is to put it into module that contains interface, because it is basically testing contract of that interface. Then add concrete tests into modules that implement interface. However our build system does not allow me to do that because tests are not part of built artefact, thus dependent modules do not see them.

A workaround for this problem is to introduce a new module that will contain abstract test masked as production class, hence it will be visible from other modules. Of course, this new helper module will not be included in installer of product. All problems solved... Not so fast, theoretically this works perfectly, but it might not be practical if that new module would contain only 1 or 2 abstract tests. It is heavy weight solution for such amount of code. Our codebase contains tens of small modules separating different parts of product to abstract and concrete modules (which is good) and introduction of test-supporting modules might help this number to grow rapidly.

Fortunatelly I have found that I can solve this particular problem differently this time, because only occured due to badly factored production classes. If I create abstract superclass for them, I can easily remove duplication of tests and add new test classes testing only logic that is different.

Somehow I am getting different moral conclusion that I planned with this post - "Look for real source of problems before attempting to fix them", but that's only because my case was special. In general, it is important to "Make sure your build system allows you to depend on tests from different modules" (only tests should depend, not production code). This allows you to do better tests regarding removing duplication by factoring common code to helpers and introduction of abstract interface tests.

Note about testing of intefaces

I was not able to google good example of inteface tests (Apart from that you can read about them in JUnit Recipes by J. B. Rainsberger, but that will not help you to get up to speed without having the book at hand. Still I encourage you to read it later.). So I am going to write a few lines about the idea. Let's have interface Stack.

public interface Stack {
    void push(Object data);
    Object pop();
    boolean isEmpty();
}

This interface has defined contract. It can be specified as Javadoc or any other way, but how can you make sure that all implementations comply to it?

The best way to achieve compliance is to provide abstract test case that implementor extends and provides method for creation of tested instance. Then she can execute test and everybody knows if it is compliant or not in fraction of second.

public abstract class StackTest {
    protected abstract Stack createInstance();

    public void testShouldPopLastPushedItem() {
        Stack tested = createInstance();
        //... rest of test
    }

    public void testShouldThrowExceptionWhenPoppingFromEmpty() {
        Stack tested = createInstance();
        //... rest of test
    }
}

Having these tests for contract it is quite easy to verify our cool implementation of Stack is working:

public class PersistentAndsSecureStackTest extends StackTest  {
    // add some setUp and tearDown if needed
    protected Stack createInstance() {
        return new PersistentAndsSecureStack();
    }
}

Back to the point

This is only one of the reasons why my tests need to be able to depend on tests from other modules. It is simply strong tool for making sure product is well tested and all duplication is removed.

I am still not able to set up dependency of tests between different modules. Fortunatelly I did not need it again.

When I was thinking what should be my first post in my new professional blog I have remembered test method (in one undisclosed code base) I have seen a few months ago. It was test for equals(). Rather amusing one, clearly not doing what it was intended for. (I have changed it a bit to protect author and I should state that it was written years ago, thus its author does not do these things anymore).

    public final void testEqualsObject() {
        final SomeObject objOne = // new ...
        final SomeObject objTwo = // new ...
    
        final SomeObject tmpObj = objOne;

        assertTrue("The equals() failed", objOne.equals(tmpObj));
        assertFalse("The equals() failed", objOne.equals(objTwo));
    }

That assertTrue can test only default implementation of Object.equals that uses ==, but nothing more. assertFalse is a bit better, but not enough for quite complicated contract equals and hashCode have.

I have fixed it, unfortunatelly not in way I would like to - by usage of EqualsTester, because this and all other implementations of equals() use instanceof that is controversial enough to start fighting about right now.

Do me a favor, any time you need to test equals and hashCode use EqualsTester, please. You will not regret it in the long term.

What is the most useless piece of unit test you have seen? Your code done in past counts too ;-) How did you fix it?