Lives in the UK. Likes blogging, cycling and eating lemon drizzle cake. Roger is a DZone MVB and is not an employee of DZone and has posted 143 posts at DZone. You can read more from them at their website. View Full User Profile

What Should you Unit Test? - Testing Techniques 3

11.21.2011
| 5337 views |
  • submit to reddit

I was in the office yesterday, talking about testing to one of my colleagues who was a little unconvinced by writing unit tests. One of the reasons that he was using was that some tests seem meaningless, which brings me on the the subject of what exactly you unit test, and what you don’t need to bother with.

Consider a simple immutable Name bean below with a constructor and a bunch of getters. In this example I’m going to let the code speak for itself as I hope that it’s obvious that any testing would be pointless.

public class Name {

  private final String firstName;
  private final String middleName;
  private final String surname;

  public Name(String christianName, String middleName, String surname) {
    this.firstName = christianName;
    this.middleName = middleName;
    this.surname = surname;
  }

  public String getFirstName() {
    return firstName;
  }

  public String getMiddleName() {
    return middleName;
  }

  public String getSurname() {
    return surname;
  }
}

 

...and just to underline the point, here is the pointless test code:
public class NameTest {

  private Name instance;

  @Before
  public void setUp() {
    instance = new Name("John", "Stephen", "Smith");
  }

  @Test
  public void testGetFirstName() {
    String result = instance.getFirstName();
    assertEquals("John", result);
  }

  @Test
  public void testGetMiddleName() {
    String result = instance.getMiddleName();
    assertEquals("Stephen", result);
  }

  @Test
  public void testGetSurname() {
    String result = instance.getSurname();
    assertEquals("Smith", result);
  }
}
The reason it’s pointless testing this class is that the code doesn’t contain any logic; however, the moment you add something like this:
  public String getFullName() {

    if (isValidString(firstName) && isValidString(middleName) && isValidString(surname)) {
      return firstName + " " + middleName + " " + surname;
    } else {
      throw new RuntimeException("Invalid Name Values");
    }
  }

  private boolean isValidString(String str) {
    return isNotNull(str) && str.length() > 0;
  }

  private boolean isNotNull(Object obj) {
    return obj != null;
  }
...to your class then the whole situation changes. Adding some logic in the form of an if statement generates a whole bunch of tests:
  @Test
  public void testGetFullName_with_valid_input() {

    instance = new Name("John", "Stephen", "Smith");

    final String expected = "John Stephen Smith";

    String result = instance.getFullName();
    assertEquals(expected, result);
  }

  @Test(expected = RuntimeException.class)
  public void testGetFullName_with_null_firstName() {

    instance = new Name(null, "Stephen", "Smith");
    instance.getFullName();
  }

  @Test(expected = RuntimeException.class)
  public void testGetFullName_with_null_middleName() {

    instance = new Name("John", null, "Smith");
    instance.getFullName();
  }

  @Test(expected = RuntimeException.class)
  public void testGetFullName_with_null_surname() {

    instance = new Name("John", "Stephen", null);
    instance.getFullName();
  }

  @Test(expected = RuntimeException.class)
  public void testGetFullName_with_no_firstName() {

    instance = new Name("", "Stephen", "Smith");
    instance.getFullName();
  }

  @Test(expected = RuntimeException.class)
  public void testGetFullName_with_no_middleName() {

    instance = new Name("John", "", "Smith");
    instance.getFullName();
  }

  @Test(expected = RuntimeException.class)
  public void testGetFullName_with_no_surname() {

    instance = new Name("John", "Stephen", "");
    instance.getFullName();
  }
So, given that I’ve just said that you shouldn't need to test objects that do not contain any logic statements, and in a list of logic statements I’d include if and switch together with all the operators (+-*-), and a whole bundle of things that could change and objects state.

Given this premise, I’d then suggest that it’s pointless writing a unit test for the address data access object (DAO) in the Address1 project I’ve been talking about in my last couple of blogs. The DAO is defined by the AddressDao interface and implemented by the JdbcAddress class:
public class JdbcAddress extends JdbcDaoSupport implements AddressDao {

  /**
   * This is an instance of the query object that'll sort out the results of
   * the SQL and produce whatever values objects are required
   */
  private MyQueryClass query;

  /** This is the SQL with which to run this DAO */
  private static final String sql = "select * from addresses where id = ?";

  /**
   * A class that does the mapping of row data into a value object.
   */
  class MyQueryClass extends MappingSqlQuery<Address> {

    public MyQueryClass(DataSource dataSource, String sql) {
      super(dataSource, sql);
      this.declareParameter(new SqlParameter(Types.INTEGER));
    }

    /**
     * This the implementation of the MappingSqlQuery abstract method. This
     * method creates and returns a instance of our value object associated
     * with the table / select statement.
     *
     * @param rs
     *            This is the current ResultSet
     * @param rowNum
     *            The rowNum
     * @throws SQLException
     *             This is taken care of by the Spring stuff...
     */
    @Override
    protected Address mapRow(ResultSet rs, int rowNum) throws SQLException {

      return new Address(rs.getInt("id"), rs.getString("street"),
          rs.getString("town"), rs.getString("post_code"),
          rs.getString("country"));
    }
  }

  /**
   * Override the JdbcDaoSupport method of this name, calling the super class
   * so that things get set-up correctly and then create the inner query
   * class.
   */
  @Override
  protected void initDao() throws Exception {
    super.initDao();
    query = new MyQueryClass(getDataSource(), sql);
  }

  /**
   * Return an address object based upon it's id
   */
  @Override
  public Address findAddress(int id) {
    return query.findObject(id);
  }

}
In the code above, the only method in the interface is:
  @Override
  public Address findAddress(int id) {
    return query.findObject(id);
  }
...which is really a simple getter method. This seems okay to me as there really should not be any business logic in a DAO, that belongs in the AddressService, which should have a plentiful supply of unit tests.

You may want to make a decision on whether or not you want to write unit tests for the MyQueryClass. To me this is a borderline case, so I look forward to any comments...

I’m guessing that someone will disagree with this approach, say you should test the JdbcAddress object and that’s true, I’d personally write an integration test for it to make sure that the database I’m using is okay, that it understands my the SQL and that the two entities (DAO and database) can talk to each other, but I won’t bother unit testing it.

To conclude, unit tests must be meaningful, and a good a definition of ‘meaningful’ is that object under test must contain some independent logic.

1The source code is available from GitHub at:

git://github.com/roghughe/captaindebug.git

 

From http://www.captaindebug.com/2011/11/what-should-you-unit-test-testing.html

Published at DZone with permission of Roger Hughes, author and DZone MVB.

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Tags:

Comments

Tormod Overlier replied on Mon, 2011/11/21 - 2:33am

I do not agree that it's pointless to test the initial Name class. The constructor changes the state of "this" and therefore, it needs to be tested. I would create a test that assures that the constructor sets the correct values in the correct fields. This test would be using the getters. In the end that means you get 100% test coverage on the class.

Also, I would consider adding checks in the constructor for valid input. For instance, is it valid to have a Name without (null) a first name and surname? I think not. This logic would also need to be tested.

Roger Hughes replied on Mon, 2011/11/21 - 4:13am

This is where testing gets contentious. The original Name class doesn't contain any logic and any input values are allowed when calling the constructor including nulls and other junk. All you're testing in the initial version is that the equals symbol works. To demonstrate this further, lets strip away Name and the call to the constructor. All you're left with is a test that goes like this:

firstName = "John";
assertEquals("John", firstName);
middleName = "Stephen"; 
assertEquals("Stephen", middleName); 
surname = "Smith"; 
assertEquals("Smith", surname);  

In my mind this is not worth testing, but don't take my word for it... for more information on this also see The Art of Unit Testing by Roy Osherove.

Steve Brickman replied on Mon, 2011/11/21 - 5:21am in response to: Roger Hughes

Yes, you are just testing the equal sign. Yet it still catches c&p errors like

public Name(String christianName, String middleName, String surname) {
  this.firstName = christianName;
  this.middleName = middleName;
  this.surname = middleName;
}

They may not be relevant in working code, since the error would be catched and fixed hopefully quickly. But when working test driven, the test is indeed the the first usage of the code which should verify correctness. In those cases the only reason I see to skip those quick to implement tests would be their coverage by other tests, which is likely the case for simple bean accessors (what is the reason for accessors anyway, if they are not used elsewhere).

Tormod Overlier replied on Mon, 2011/11/21 - 5:32am

I think maybe we define logic in different ways. To me, anything that alters state is logic. I tend to use this kind of thinking to challenge my test writing: "What if I change this line of code? Will any tests fail?". If the answer is no, my tests are not good enough. This is the same kind of thinking that is used in mutation testing: http://en.wikipedia.org/wiki/Mutation_testing. For instance, if I comment out the line this.firstName = christianName in the constructor, no tests will fail but any code that uses Name will be broken.

Tormod Overlier replied on Mon, 2011/11/21 - 5:39am in response to: Steve Brickman

I agree a 100%.

Mladen Girazovski replied on Mon, 2011/11/21 - 8:59am

Your first example looks somewhat "contructed" to me ;)

First, why would you want to throw an generic RuntimeException when there is a better fitting, more specific Exception called IllegalArgumentExecption?

Second, you have redundancies in your test: the String literals.

As mentioned before, if you drive TDD, you would have written a test for it.

 

As for your second example, it depends on how you define "unit test", ie. integration test, isolated unit test, etc. pp.

If you're talking about an integration test, then there is plenty to test when it comes to the DAO.

The SQL Schema(tables, fields, contraints/foreign fields, etc. pp.),  configuration of the DB (User, permission, etc. pp.), the mapping of the objects to the DB and vice versa, including the SQL that the DAO is going to use.

Lots of code and configuration to be tested, right?

So, if you wanted to test all the important parts of your system, then you would definitaly need to test the DAO.

IMHO the best book about testing is "xUnit Test Patterns: Refactoring Test Code".

This seems okay to me as there really should not be any business logic in a DAO, that belongs in the AddressService, which should have a plentiful supply of unit tests.

 I don't agree with your comment, the "bigger" your subject under test, the smaller is the defect localisation abbility of your test, the less usefull is the test if it fails.

Apart from that, Business Logic tends to end up in the DAOs one way or another, that is one reason why their successor, Repositories, are part of the domain model, not of another layer.

Ramzi Maalej replied on Mon, 2011/11/21 - 12:15pm

But what if another develper change the implementation of the constructor and getters/setters. How would you be sure that they behave the same way you designed them. Although it seems meaningless, it's worth writing unit tests to ensure that your class does what you're expecting from it. I even recommend people writing unit test to just check that a constant value is never changed.

Loren Kratzke replied on Mon, 2011/11/21 - 2:39pm in response to: Ramzi Maalej

But what if that same developer changed the unit test so that it passed anyway?

I see far too much effort going into unit testing these days. In my experience, a unit test is about 97x more likely to break due to trivial refactoring than it is to detect broken funtionality caused by the same. Just my experience. I see very little real world ROI in unit testing. I know, sounds shocking, but true.

Ramzi Maalej replied on Mon, 2011/11/21 - 4:12pm

Well he has to fix the unit test anyways :). Without unit test in place, developer could not be aware that he broke something.

Loren Kratzke replied on Mon, 2011/11/21 - 5:08pm in response to: Ramzi Maalej

The "something" being the unit test? Case in point. :-)

Ramzi Maalej replied on Tue, 2011/11/22 - 9:25am in response to: Loren Kratzke

Not only a unit test, but also a functionnality (a requirement). When you break a unit test, you'll get a chance to verify you implementation based on your analysis of the test you broke. I remember many times when I had to change my imlementation because broken unit tests pointed me out that what I did is a solution that doesn't meet all the requirements. In short, well written unit tests provide documentation and help you better understand the requirements.

Ant Kutschera replied on Tue, 2011/11/22 - 12:11pm

Another criterion you can use in deciding whether you want to write tests is your budget. If you can't afford 110% coverage (100% won't find all bugs!) then you need to prioritise. Perhaps testing the name class won't be worth it. I like my DAOs to provide data access and not business logic. Isn't that why they are named DAO?

Loren Kratzke replied on Tue, 2011/11/22 - 7:30pm in response to: Ramzi Maalej

The only time in the last 5 years that a unit test saved my ass was with a class that performed a very complex algorythm. It was too complex to fully understand just by glancing at it (even for me and I wrote it). I made a change, the unit test broke, investigated, payoff.

But the other 99 times in the last 5 years (that is how long I have been on this code base, about 500,000 lines) the unit tests were the only thing that broke. Yes, the functionality changed. Unit tests can't handle that. Even if you perform a perfect refactor and account for every class that uses the refactored code, your reward is a pile of broken unit tests.

Unit tests in this case are nothing more than change tests. Change the code and the change test breaks. I feel like I have to write a bunch of unit test code just to prove that I have been informed that business code has changed. I just don't think the ROI is there in the vast majority of cases.

My rule is to only unit test stuff that is either too complex to understand at a glance or is rigid (such as a published interface) and two or more implementations must strictly implement that interface. Everything else seems like a bit of a waste of precious time that could be better spent writing better, faster, or more robust code.

I know, I am in the minority, but I have been doing this for over 20 years and I know a thing or two about a thing or two. I know a bad smell when I encounter it.

Unit testing of the extreme variety is indeed an extreme waste of resources in my experience, outside of the highly rational exceptions that I have mentioned.

One last point - There is no correct percentage of test coverage. Every code base has unique requirements, so the coverage metric is a horrible and trivial metric that measures nothing of value, really.

Ant Kutschera replied on Wed, 2011/11/23 - 6:50am

You're not as crazy as some people might think. Sonar tells me that only 78% of our unit tests are successful and we only have 12% coverage. We too have many hundreds of thousands of lines of code. Am I really worried? Not really, the systems have a turn over in the region of 4 billion US bucks a year and are in production. They appear to work. Sure, we get bugs. We just patched prod with 90 odd patches. Have we saved money by not writing tests? That's hard to say. Our integration testing, end to end testing and acceptance testing definitely go through more cycles than they should and we seem to patch our patches. I can't measure it, but I'd say the money we save in unit testing is spent later on. We are trying to shift the culture from todays laissez faire attitude to one where developers want to, and are given more time to test. We won't unit test everything, but we will unit test the stuff which needs testing. I recently reviewed a patch and told the programmer I didn't understand his changes. I asked for a unit test and I got one. It didn't show any other bugs, and it means we have more code to maintain. But for me it was important for the developer to document his expectation of the methods behaviour in a way which future developers can verify (unlike say javadoc). The alternative would have been to take badly rotten code and rewrite it to be readable and reviewable. But without tests to ensure that wouldn't have broken anything, rewriting carried more risk and so was less desirable for patching. I don't believe in TDD. I believe in testing as much as it is needed, within a budget. That tends to be complex algorithms and core code used by many. Unit tests as documentation is also good. Don't rely on coverage as a measure of success.

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.