You Need Code Reviews

February 5, 2007

Look at this code:

(within a while body)

...
if (log.isDebugEnabled()) log.debug("Parsing line " + (lineNumber++));
...
lineNumber used here
...

You might be wondering: what´s wrong with it? Well, it turns out that this sort of construction it could become extremely difficult to spot in certain conditions. Imagine a hypothetical situation where this code is not working properly in the production environment, and meanwhile the developer in charge of this block of code -the guy as cool as a cucumber- is totally sure about the correctness of it. Okay, you got it, you may be thinking of me: “How noob this guy is!”; but Hey!, sometimes you are toiling late in the night and this kind of bugs pass unnoticed until you receive an email from your Development Manager muttering:

Subject: Potential bug

I have come across with this statement –incidentally- while surfing the code. Beware that the log will be printed out only if the log level is set to DEBUG, so the lineNumber variable won´t be incremented.

All the best,

Your Development Manager

Guess which level the Production Support had established in their environment? Fortunately, I started off saying “Imagine a hypothetical situation …“.


AbstractTransactionalSpringContextTests meets TestNG

December 12, 2006

Introduction

In the search of an alternative to JUnit, in relation to the testing of our classes, we -Mikel and I- decided to give TestNG a try. This testing framework takes advantage of a useful feature introduced in the release 5.0 of the Java Language: the annotations. In addition, it provides a rich set of facilities to ease the unit and integration testing of Java classes, like groups and data providers. Unfortunately, these are very well explained in the home page of the project, so let´s consider them as being out of the scope in regards to the main thread of this post.

The Problem

What we want to have: test cases running within a transactional context AND take advantage of the TestNG facilities. The former is neatly addressed by a class named AbstractTransactionalSpringContextTests, present in the Spring´s mock package. The latter, well, pretty obvious. What it isn´t so obvious is the way to combine both. Why? TestNG does not provide a direct way to surround the test cases with a transaction, so a rollback action is performed at the end of every test. This way, we are able to avoid side effects between test executions, because one test inserts a row and the next one didn´t expect it, or because a database row is updated between executions breaking some assertions, or because … you get the idea. When we decided to start using TestNG as our main framework for testing, this absence lead us to ask Cedric Beust, the TestNG project leader, about some way to accomplish with the transaction stuff. He responded us very kind and rapidly, recommending to implement the rollbacking in an @AfterMethod method.

TransactionRollbackTearDownTestCase

Note: for this class’s name origin, take a look at this site

Probably Cedric is completely right, but we like how AbstractTransactionalSpringContextTests does its business, and of course, the integration with the Spring´s IoC in order to inject the dependencies to the test classes. For that reason, we have come up with an adapter class, which permits us to run TestNG test cases within the transactional context provided by the Spring class. It is kinda adapter because we change the way the class AbstractTransactionalSpringContextTests is used (i.e. inheriting from it), right now we can extend from the new base class and define our test cases as the usual TestNG manner (i.e. through the use of annotations). It is not an adapter as in the software design patterns.

The class is named TransactionalRollbackTearDownTestCase, and following there is a diagram of its relationships:

TransactionalRollbackTeardownTestCase

The code of this class could prove more descriptive than me trying to explain how it does its job:

package com.centuryminds.test;

import org.springframework.test.AbstractDependencyInjectionSpringContextTests;
import org.springframework.test.AbstractTransactionalSpringContextTests;

import org.testng.annotations.AfterMethod;
import org.testng.annotations.BeforeMethod;

/**
 * Base test class which permits: 1) Inject dependencies defined in a Spring
 * context. The aforementioned dependencies must be declared as protected
 * members 2) Perform a rollback action after every test. Basically, it
 * acts as an adapter between the TestNG test case structure and the facilities
 * provided by the class {@link AbstractDependencyInjectionSpringContextTests}
 * in the Spring package
 *
 * @see AbstractDependencyInjectionSpringContextTests
 * @see AfterMethod
 * @see BeforeMethod
 */
public abstract class TransactionRollbackTearDownTestCase extends
		AbstractTransactionalSpringContextTests
{

	/**
	 * Test Fixture
	 *
	 * @throws Exception if a error occurred during the fixture execution
	 * @see {@link AbstractDependencyInjectionSpringContextTests#setUp()}
	 * @see {@link AbstractDependencyInjectionSpringContextTests#setPopulateProtectedVariables(boolean)}
	 */
	@BeforeMethod
	protected final void before() throws Exception
	{
		setPopulateProtectedVariables(true);
		super.setUp();
	}

	/**
	 * Test Teardown
	 *
	 * @throws Exception if a error occurred during the teardown execution
	 * @see {@link AbstractDependencyInjectionSpringContextTests#tearDown()}
	 */
	@AfterMethod
	protected final void after() throws Exception
	{
		super.tearDown();
	}
}

As it can be seen, this class is really simple. It defines two final methods, both TestNG-annotated which delegate the test fixture and teardown actions to the underlying parent implementation. Therefore, every time a test case method is going to be executed, before() will inject the dependencies defined in the Spring context and will create and start a new transaction context. The method tearDown() in turn will perform a rollback action just after the test ends.

Extending the Hierarchy

Obviously, the class above is not enough: where are the dependencies to be injected defined? We have to tell AbstractTransactionalSpringContextTests where to look for them. This can be achieved by a subclass of TransactionalRollbackTearDownTestCase, for example:

package com.centuryminds.test;

/**
 * Base test class that specifies the spring contexts containing the dao
 * definitions. The subclasses must provide an implementation of the method
 * {@link #getConfigLocations()}, specifying the files with the spring contexts
 * holding the definitions of the dao and its dependencies
 */
public class BaseDAOTestCase extends TransactionRollbackTearDownTestCase
{
	/**
	 * @see org.springframework.test.AbstractDependencyInjectionSpringContextTests#getConfigLocations()
	 */
	@Override
	protected String[] getConfigLocations()
	{
		return new String[] { "classpath:applicationContext-datasource.xml",
				"classpath:applicationContext-service.xml" };
	}
}

The Actual Test Class

One important benefit of the separation above is that we could define several BaseDAO* classes, each one of them declaring its own context set. Let´s see an example DAO test class extending from BaseDAOTestCase:

package com.centuryminds.test;

import java.util.HashSet;

import org.testng.annotations.Test;

import com.threefish.aquarium.dao.hibernate.GenericHibernateDAO;
import com.threefish.aquarium.model.Role;
import com.threefish.aquarium.model.User;

/**
 * Tests for RoleDAO
 */
public class RoleDAOTest extends BaseDAOTestCase
{
	/** role dao */
	protected GenericHibernateDAO roleDAO;

	/** role */
	private Role role;

	@Test
	public void testCreate() throws Exception
	{
		int amount = roleDAO.findAll().size();

		// let's create a new role to persist
		role = new Role();
		role.setName("testrole");
		role.setDescription("Master of Universe");
		role.setUsers(new HashSet());

		roleDAO.makePersistent(role);

		assertNotNull(role.getId());
		assertEquals(amount + 1, roleDAO.findAll().size());
	}

	@Test
	public void testSuccessfulGet() throws Exception
	{
		role = roleDAO.findById(Long.valueOf(1));

		assertEquals("admin", role.getName());
	}

	@Test
	public void testUpdate() throws Exception
	{
		role = roleDAO.findById(Long.valueOf(1));
		role.setDescription("Super Administrator Role");
		roleDAO.makePersistent(role);

		assertEquals("Super Administrator Role", role.getDescription());
	}
}

The RoleDAO test class makes use of a custom generic DAO framework, but despite that I think it is pretty self-explanatory. We shall try to explain the generic DAO related interfaces and classes in another post when we get a chance, but for now we have enough.

The test methods are executed within a transaction per test method, which is rollbacked at the very end of it. So, the role insert in the first test (testCreate()), would be rollbacked and future tests depending on the initial state (i.e. the state before testCreate() ran) wouldn´t be affected. We accomplished with the goal of executing the test cases in isolation. Thanks Spring! We accomplished with the goal of using some of the features offered by TestNG. Thanks to you as well!

Future

We are aware about the drawbacks of this solution, and we are pretty sure about the fact that there must exist a more sophisticated solution out there, hence we are very willing to listen to your suggestions to improve the general structure of our current one. We’ll stay tuned …


Jetty Embebido

November 8, 2006

Una característica muy interesante del servidor Jetty es que puede ser embebido en cualquier aplicación JAVA. Esto nos permite hacer diversas cosas: Desde dar una interfaz de configuración Web a una aplicación no Web, hasta cosas más exóticas como realizar testing de un cliente http.

Lo mejor que tiene es que es facilísimo poner un servlet a escuchar en una url, vamos, cosa de niños. Bueno, no me enrollo más, aquí va el código:

import java.io.IOException;
import javax.servlet.Servlet;
import javax.servlet.ServletConfig;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import org.mortbay.jetty.Server;
import org.mortbay.jetty.servlet.Context;
import org.mortbay.jetty.servlet.ServletHolder;
...
server= new Server(8080);
Context root = new Context(server,"/contexto",Context.SESSIONS);
root.addServlet(new ServletHolder(new Servlet(){
 public void destroy() {}
 public ServletConfig getServletConfig() {return null;}
 public String getServletInfo() {return null;}
 public void init(ServletConfig arg0) throws ServletException {}
 public void service(ServletRequest request, ServletResponse response)
         throws ServletException, IOException {
     response.getOutputStream().write"HOLA MUNDO"getBytes());
 }
}), "/servlet");
server.start();
 ...

Y ya tenemos nuestro servlet escuchando en http://localhost:8080/contexto/servlet. Este servlet tampoco hace mucho, pero da una idea de lo fácil que puede ser poner a escuchar un servlet en un Jetty embebido.


Exceptions y JDK 1.3

October 11, 2006

Yo soy el primero al que le encantaría el poder utilizar las características de la versión 1.5 de Java, en concreto encuentro muy útiles los static imports y los generics, y por supuesto las annotations. Sin embargo, durante mi carrera, y por diversas razones, me he encontrado en situaciones de no poder utilizar siquiera la versión 1.4 del JDK. Es triste, pero cierto, y me temo que es muy habitual encontrarse con la restricción tan temida de: “Tenemos que programar contra el JDK 1.3“. Es algo así como una especie de maldición, cuando estás habituado a utilizar los métodos de la clase String añadidos a partir de la versión 1.4 –replaceAll(), split(), etc-, o la facilidad de la exception chaining. Por fortuna, el proyecto Commons de la fundación Apache puede ayudarnos a solventar algunos de esos obstáculos. Vayamos con la exception chaining. Esta es la interfaz de la clase java.lang.RuntimeException* en la versión 1.4:

public RuntimeException()
public RuntimeException(String message)
public RuntimeException(String message, Throwable cause)
public RuntimeException(Throwable cause)

Como se puede observar, dos de los constructores de esta clase admiten un argumento java.lang.Throwable, por tanto cuando usemos esta clase en la versión 1.4 no perderemos la traza de llamadas de las excepciones. Ahora veamos las signaturas de los constructores de esta misma clase en la versión 1.3:

public RuntimeException()
public RuntimeException(String message)

Ahora ya sabemos que la exception chaining fue un añadido a la versión 1.4 del JDK a la clase RuntimeException. En el subproyecto Commons Lang existe una jerarquía de clases que nos ayudan a establecer una pila de llamadas previa en el constructor de una excepción. En concreto, son las clases NestableRuntimeException y NestableException del paquete org.apache.commons.lang.exception:

Jakarta Commons Exception Hierarchy

Estos son los constructores de la clase NestableRuntimeException:

public NestableRuntimeException()
public NestableRuntimeException(String message)
public NestableRuntimeException(String message, Throwable cause)
public NestableRuntimeException(Throwable cause)

Equivalentes a los de la clase RuntimeException, de forma que funciona como tal y además mantiene una referencia a un objeto Throwable, con lo que simulamos la funcionalidad añadida a la versión 1.4 pero en un entorno compilado para la 1.3. Ahora simplemente podremos definir nuestras propias subclases de NestableRuntimeException o NestableException, sin miedo a que obtengamos errores de compilación debido al overriding de los constructores no existentes en la 1.3, pero sí en la 1.4. Ahora que lo digo, yo he sufrido dichos errores debido a que en mi IDE (Eclipse normalmente) no configuro el proyecto para compilar las fuentes contra la 1.3 (cuando ha de desplegarse en un entorno de JDK 1.3, claro). Aunque ya voy aprendiendo. Considero siempre una buena práctica configurar el entorno de desarrollo a semejanza del entorno destino, en la medida de lo posible.

* También se aplica a la clase java.lang.Exception


Rompiendo Dependencias

October 3, 2006

5 Freestyler (Boomfunk MC)

Introducción

En un mundo ideal, todos comenzaríamos los proyectos desde cero, podríamos elegir las tecnologías que consideráramos más apropiadas para cada caso, los jefes nos entenderían, nosotros entenderíamos a los jefes, etc. En fin, un Mundo Feliz como el de Aldous Huxley. Sin embargo, para todo el que lleva algo de tiempo en el mundo del desarrollo de software (más o menos con su primer día de trabajo después acabar los estudios ya es suficiente) sabe lo que le toca: mantener código. Según lo veo yo, a esta actividad normalmente se la considera de segundo orden, y no lo es. La confusión es que muchos creen que mantener código pasa casi exclusivamente por cambiar cosas ya hechas o arreglar errores en el código, ya sea tuyo o de otros. Vamos, un sufrimiento. Pero mantener código es mucho más que eso, ya que la propia expresión cambiar cosas ya hechas es demasiado general. Puede abarcar desde cambios en la interfaz de usuario, añadir funcionalidad nueva a la ya existente, eliminar funcionalidad no requerida, mejora del rendimiento, separación y creación de componentes a partir del código actual para ser reutilizado, cambios en el sistema de construcción o despliegue de la aplicación, etcétera etcétera
Como se puede observar, las habilidades requeridas pueden ser muy heterogéneas y abarcar varios campos, e incluso podría decir que es hasta fascinante. Porque al fin y al cabo, la gran mayoría de las aplicaciones evolucionan una vez implantadas hasta que ya no son necesarias, y alguien tiene que hacerse cargo de esa evolución.

Mantenimiento en la Práctica

Muchas veces nos encontramos con código más o menos antiguo, al cual tenemos que añadir funcionalidad nueva o modificar la existente. El problema es que este código es totalmente nuevo para nosotros y no queremos romper nada. Normalmente esto sucede debido a un fuerte acoplamiento entre los componentes del sistema, en un sentido más o menos general: clases, métodos, etc. Un cambio simple en la implementación de un método de una clase puede afectar, transitivamente, a una clase de la que ni siquiera tenemos constancia de su existencia. Puede que el comportamiento erróneo introducido lo descubramos cuando la aplicación está en ejecución, en entornos de QA o Producción. Dependiendo de la tolerancia del proyecto al ciclo de feedback, podemos encontrarnos con una situación inaceptable. Y he aquí la cuestión principal a tener en cuenta: ¿cómo podemos saber si hemos introducido efectos colaterales no deseados en otras partes del sistema? Evidentemente, un sistema software actual puede llegar a ser muy complejo, pero existen formas de mitigar los efectos no deseados de nuestras modificaciones al código. En concreto, estoy hablando de los tests, tanto unitarios como de integración en este caso. Si tenemos un conjunto de tests que prueban la corrección del código existente, podremos sentirnos más seguros a la hora de añadir o modificar funcionalidad. Es por eso que la recomendación en este caso es la de crear ANTES tests para el código sobre el que debemos trabajar. Nos lo agradeceremos nosotros mismos, y nos lo agradecerá el resto del equipo.

Dependencias

Después de haber intentado dejar claro que un buen juego de tests son una especie de seguro de vida, vayamos a su vez con uno de los problemas que nos encontramos cuando queremos crear un test unitario para una clase de este sistema: las dependencias. Algo tan simple como este código puede ser un obstáculo tremendo para acometer lo que nos proponemos:

class A
{
  private B b = new B();

  public void doWhatever()
  {
    b.doBTask();
  }
  ...
}

La clase A tiene una relación de dependencia con la clase B, lo que significa que en el método doWhatever() de A se utiliza la funcionalidad de B. Si la dependencia fuera de uso estaríamos en un caso parecido. Ahora, si queremos crear un test unitario para dicho método de A, estaremos indirectamente invocando al método doBTask() de B. En dos palabras, ya no sería un test unitario. Por tanto, centremos nuestro objetivo en el título de este post: romper dependencias. Aunque primero presentemos un ejemplo un poco más real, que a veces eso de tener clases A y B, más que ayudar, complica la explicación.

De camiones, ruedas y motores

Imaginemos un sistema de información de una empresa de transportes. Cada cierto período de tiempo, esta empresa realiza exhaustivos controles mecánicos a su flota de camiones. Esta podría ser la parte del sistema que nos interesa (obviamente muy simplista):

Trucks, Wheels ...

La clase Truck podría tomar la siguiente forma en el código:

class Truck
{
  private Wheel[] wheels;

  private Engine engine;  public Truck()
  {
    wheels = new Wheel[4]{new Wheel(), new Wheel(), new Wheel(), new Wheel()};
    engine = new Engine();
  }

  public boolean review()
  {
     boolean firstWheelOK = wheels[0].check();
     boolean secondWheelOK = wheels[1].check();
     boolean thirdWheelOK = wheels[2].check();
     boolean fourthWheelOK = wheels[3].check();
     boolean engineOK = engine.check();

     return (engineOK && firstWheelOK && secondWheelOK && thirdEngineOK && fourthEngineOK);
  }
}

La clase anterior, evidentemente, es mejorable en muchos aspectos: ¿4 ruedas fijas?, ¿clase inmutable?, etc. Pero a efectos de ilustrar la rotura parcial de dependencias, nos sirve. Vayamos con el test. Esta técnica es independiente del framework de testing que utilicemos (JUnit en este ejemplo).

class TruckTest extends Test
{
  private Truck truck;

  public void setUp() throws Exception
  {
    truck = new Truck();
  }

  public void testCheck() throws Exception
  {
    assertTrue(truck.review());
  }
}

La llamada truck.review(), implica que se llaman los métodos check() de las ruedas y del motor del camión. Esto sería correcto para un test de integración, pero no para uno unitario. ¿Qué podemos hacer? Pues bien, para todos aquellos afortunados que en su empresa utilizan un framework de IoC, podéis dar por finalizada la lectura de esto aquí. Delegar en Spring por ejemplo la creación y asignación de los objetos miembros de la clase Truck (wheels y engine) es ciertamente algo deseable. Sin embargo, si una aplicación no hace uso de la inversión del control, ya sea por antigüedad o por la razón que sea, mi predicción es que en muchos casos migrar el código para delegar el ciclo de vida de las dependencias a un framework externo como Spring puede ser complicado y producir más de un dolor de cabeza. Así que valgámonos de una propiedad de la OO como es el polimorfismo para crear un test unitario para la clase Truck.

El primer paso va a ser refactorizar la clase para que obtenga las referencias de sus miembros a través de un método accesor (getWheels() y getEngine()):

class Truck
{
  private Wheel[] wheels;

  private Engine engine;  public Truck()
  {
    wheels = getWheels();
    engine = getEngine();
  }

  public boolean review()
  {
    boolean firstWheelOK = wheels[0].check();
    boolean secondWheelOK = wheels[1].check();
    boolean thirdWheelOK = wheels[2].check();
    boolean fourthWheelOK = wheels[3].check();
    boolean engineOK = engine.check();

    return (engineOK && firstWheelOK && secondWheelOK && thirdEngineOK && fourthEngineOK);
  }

  protected Wheel[] getWheels()
  {
    return new Wheel[4]{new Wheel(), new Wheel(), new Wheel(), new Wheel()};
  }

  protected Engine getEngine()
  {
    return new Engine();
  }
}

El segundo paso va a ser crear clases mock de dichas dependencias, de forma que estáticamente podemos definir el comportamiento de éstas y poder así probar la clase Truck en base a este comportamiento definido:

class TruckTest extends Test
{
  private Truck truck;  public void setUp() throws Exception
  {
    truck = new MockTruck();
  }

  public void testCheck() throws Exception
  {
    assertTrue(truck.review());
  }

  protected class MockTruck extends Truck
  {
    protected Wheel[] getWheels()
    {
      /* 1 */
      create and return my own wheels!!!
    }

    protected Engine getEngine()
    {
      /* 2 */
      create and return my own engine!!!
    }
  }
}

Como se puede ver, la clase interna MockTruck es idéntica en comportamiento y estructura a la clase Truck, excepto que sobreescribe los métodos accesores de las dependencias creando y devolviendo las ruedas y el motor cuyos estados definimos nosotros (/* 1 */ y /* 2 */). Con esto hemos conseguido aislar parcialmente la creación dichas dependencias, por lo tanto los tests sobre la clase Truck que definamos suponen un estado y comportamiendo fijo de dichas dependencias. Hemos aislado parcialmente a Truck y ahora sí que TruckTest puede considerarse un test unitario.

Conclusiones

Un problema de uilizar esta técnica de cara al futuro puede ser que queramos crear tests de Truck para diferentes estados de sus dependencias. Tendríamos que crear una clase MockTruck por cada juego de comportamientos, o externalizar la creación de dichas dependencias en los accesores de la clase interna MockTruck, o podríamos … En fin, varias posibilidades. Por supuesto, existen frameworks muy útiles para crear mocks dinámicamente, como jMock o rMock, pero puede ser también complicada su introducción en un sistema no concebido inicialmente para su testing unitario. Por ejemplo, si no se ha seguido la máxima programar contra interfaces, esto es, que no existan o que se programe a través de ellas. Pero vamos, que se decida lo que se decida son respecto al código legado, es preferible tener al menos algo como lo de arriba, a no tener anda.

Algo como Mantenimiento Dirigido por Test, puestos a aportar algo a todo ese océano de siglas con que nos abruman cada día a los programadores.