Wednesday, May 25, 2016

Avoid and testing boilerplate code using Java 8 lambdas

Earlier this week I was doing some code review and found a piece of boilerplate code that kept showing up all the time.

The repeating code looks something like this:

 LockManager lockManager = lockManagerFactory.createManager();  
 try {  
   lockManager.acquireLock(objectUuid);  
   // business logic in here  
 } catch (LockAcquirementException e) {  
   throw new LockException(objectUuid, e);  
 } finally {  
   lock.release();  
 }  

Having a Groovy background, the first thing that came to the top of my mind was moving all of this into a separate class and pass in the business logic as closure, and have something like:

 class SecureExecutor {  
   void lockAndExecute (def objectUuid, def businessLogicClosure) {  
     LockManager lockManager = lockManagerFactory.createManager();  
     try {  
       lockManager.acquireLock(objectUuid);  
       businessLogicClosure()  
     } catch (LockAcquirementException e) {  
       throw new LockException(objectUuid, e);  
     } finally {  
       lock.release();  
     }  
   }  
 }  

But then the question stroke me: how can I do the same with Java 8?

After spending a couple of minutes in googling, I came up with this approach:

 public class SecureExecutor {  
   void lockAndExecute (String objectUuid, Runnable businessLogic) {  
     LockManager lockManager = lockManagerFactory.createManager();  
     try {  
       lockManager.acquireLock(objectUuid);  
       businessLogic.run();  
     } catch (LockAcquirementException e) {  
       throw new LockException(objectUuid, e);  
     } finally {  
       lock.release();  
     }  
   }  
 }  

This way, a consumer of SecureExecutor would work like this:

 public class SecureExecutorConsumer {  
   private SecureExecutor executor = new SecureExecutor();  
   public void executeBusinessLogic() {  
     executor.lockAndExecute("uuid", () -> {  
       // business logic goes in here  
     });  
   }    
 }  

I'm not sure this is the best way to achieve the purpose I had in mind, but it's one that works.

But then the next question is: how do I unit test this? Assume SecureExecutorConsumer's business logic delegates activities to dependent classes and posts to a JMS queue, for instance, how do I make sure my business logic is actually doing what it's supposed to do? Ok, some might argue we should consider SecureExecutorConsumer to be a black box and not test its internals, but, in case I want to test it as a white box, how do I guarantee everything works as expected?

I've put together a couple of classes to illustrate how this could be solved on https://github.com/felipecao/java8-lamba-boilerplate.

Take a look at BusinessCodeThatWantsToAvoidBoilerplate class. As the name says, this is the class I want to contain just business code and avoid having boilerplate. Notice it has two dependencies: BoilerplateCode interface, which implementation will hold the code we want to prevent from duplicating and that will execute the business logic from within it; and a Dependency class, just like we'd have in any other business class.

 public class BusinessCodeThatWantsToAvoidBoilerplate  
 {  
   BoilerplateCode boilerplate;  
   Dependency dependency;  

   public BusinessCodeThatWantsToAvoidBoilerplate(BoilerplateCode boilerplate, Dependency dependency)  
   {  
     this.boilerplate = boilerplate;  
     this.dependency = dependency;  
   }  

   public void executeBusinessLogic() {  
     boilerplate.executeBusinessLogic("Starting my business logic", () -> {  
       System.out.println("This is my business logic");  
       dependency.doSomething();  
     });  
   }  

   public static void main(String[] args)  
   {  
     BoilerplateContainer boilerplate = new BoilerplateContainer();  
     Dependency dependency = new Dependency();  
     BusinessCodeThatWantsToAvoidBoilerplate b = new BusinessCodeThatWantsToAvoidBoilerplate(boilerplate, dependency);  
     b.executeBusinessLogic();  
     System.exit(0);  
   }  
 }  

Now, when we write a unit test for BusinessCodeThatWantsToAvoidBoilerplate#executeBusinessLogic, we want the test to make sure that the Dependency method is being invoked, and not just checking if boilerplate#executeBusinessLogic was used.

The problem is: if we just mock the BoilerplateCode dependency, we'd never be able to know whether dependency#doSomething got invoked, since it's internally called by boilerplate.

On the other hand, if we use a BoilerplateContainer Spy, the actual production code would get invoked, which would require the test to setup everything the production code needs to work flawlessly (in the original code in the beginning of this post, for example, the test would need to setup all the conditions for a LockException not to be raised).

And so I remembered the concept of Test Doubles. And so what I basically did was adding creating a new class under src/test/java called FakeBoilerplateCode, which implements the same BoilerplateCode interface, but, instead of running lock calls, handling exceptions and so on, it simply executes the business logic, allowing the test to worry specifically about the conditions that surround the business logic.

From there on, instead of using a mock for BoilerplateCode, I can just make a spy out of a FakeBoilerplateCode instance, enabling me to check both that:

  • the execution of my business logic was handled by an implementation of BoilerplateCode, and; 
  • all dependencies were properly invoked.
The test class looks like below:

 import org.junit.Before;  
 import org.junit.Test;  
 import static org.mockito.Mockito.*;  

 public class BusinessCodeThatWantsToAvoidBoilerplateTest  
 {  
   BusinessCodeThatWantsToAvoidBoilerplate businessCode;  
   BoilerplateCode boilerplateContainerSpy;  
   Dependency dependencyMock;  

   @Before  
   public void setup() {  
     boilerplateContainerSpy = spy(new FakeBoilerplateCode());  
     dependencyMock = mock(Dependency.class);  
     businessCode = new BusinessCodeThatWantsToAvoidBoilerplate(boilerplateContainerSpy, dependencyMock);  
   }  

   @Test  
   public void testIt() {  
     businessCode.executeBusinessLogic();  
     verify(boilerplateContainerSpy, times(1)).executeBusinessLogic(eq("Starting my business logic"), any(Runnable.class));  
     verify(dependencyMock, times(1)).doSomething();  
   }  
 }  


I'm not sure this was the best possible approach, but at least I achieved the goal I had in mind :)

Any suggestions? Could this be improved somehow? Is Runnable the best interface to use to "emulate" a Groovy closure? Leave a comment! I'd love to talk about it!

Friday, April 1, 2016

Refactoring to understand what a piece of code does

Recently, I had to change something in the piece of code below:

@Override
    public List<Job> getFilteredJobsForPayroll(long companyId, boolean isQualifyForPayrollPeriod, boolean filterByEmployeeLeaving, boolean filterByPayrollException, boolean filterByNewEmployees) {

        //Get the specifications
        ArrayList<Job> filteredJobs = new ArrayList<>();
        Specification where =
            createSpecification(companyId, isQualifyForPayrollPeriod, filterByEmployeeLeaving, filterByPayrollException, filterByNewEmployees);
        List<PayrollReplicatedJobData> jobData = payrollReplicatedJobDataRepository.findAll(where);

        if (jobData == null || jobData.size() == 0) {
            return filteredJobs;
        }

        HashSet<String> validJobs = new HashSet<>();
        for (PayrollReplicatedJobData payrollReplicatedJobData : jobData) {
            validJobs.add(idCryptoTool.encryptId(payrollReplicatedJobData.getJobId()));
        }

        //Get the valid jobs from company server
        PagedList<Job> result = null;
        try {
            companyClient.getSession().securityToken = tokenUtil.getTokenIfAvailable();
            result = companyClient.getJobController()
                .listFullViaAdmin(null, 0, 1000);
        } catch (IOException e) {
            LOG.error("Exception retrieving job info", e);
        }

        if (result == null) {
            return filteredJobs;
        }

        filteredJobs = new ArrayList<>(result);
        List<Job> unqualifiedForPayroll = new ArrayList<>();
        for (Job job : result) {
            if (!validJobs.contains(job.getJobId())) {
                unqualifiedForPayroll.add(job);
            }
        }
        filteredJobs.removeAll(unqualifiedForPayroll);
        return filteredJobs;
    }

Quite tricky to figure out what's going on in there, isn't it? The first time I looked at it I got quite confused with all the noise generated by if's, for's, try/catches, etc. How could anyone understand the purpose of such piece of code?

After hopeless staring at this code for quite some time, I decided to try to understand it piece by piece.

The first thing I noticed is that a lot of code was written to fill-in validJobs collection, which would only be used in the end of the method, to filter things out. So, the first thing I did was extracting this to a separate method, called retrieveReplicatedJobIdsThatMatchFilterCriteria:

    private Set<String> retrieveReplicatedJobIdsThatMatchFilterCriteria(long companyId, boolean qualifiesForPayrollPeriod, boolean filterByEmployeeLeaving, boolean filterByPayrollException, boolean filterByNewEmployees) {

        Specification where = createSpecification(companyId,
                qualifiesForPayrollPeriod,
                filterByEmployeeLeaving,
                filterByPayrollException,
                filterByNewEmployees
        );
        List<PayrollReplicatedJobData> jobData = payrollReplicatedJobDataRepository.findAll(where);

        if (jobData == null || jobData.size() == 0) {
            return new HashSet<>();
        }

        return jobData.stream()
                .map( jd -> idCryptoTool.encryptId(jd.getJobId()) )
                .collect(Collectors.toSet());
    }

Notice that I did a small improvement on top of it, using Java 8's Stream API, instead of doing a for loop. In the end, both do the same, the latter just feels more elegant.

Then I noticed the second block of logic was doing a remote call to retrieve information about Job from another service. It gets hard to spot such a simple purpose with all the try/catch code in there. It urges for a separate method, to be named retrieveAllJobsFromCompanyService:

    private List<Job> retrieveAllJobsFromCompanyService() {
        List<Job> jobs = null;

        try {
            companyClient.getSession().setSecurityToken(tokenUtil.getTokenIfAvailable());
            jobs = companyClient.getJobController().listFullViaAdmin(null, 0, 1000);
        } catch (IOException e) {
            LOG.error("Exception retrieving job info", e);
        }

        return jobs;
    }

The last thing I did was another improvement in the last section of the code, to use Java 8 features. Here's the final code:

@Override
    public List<Job> getFilteredJobsForPayroll(long companyId, boolean qualifiesForPayrollPeriod, boolean filterByEmployeeLeaving, boolean filterByPayrollException, boolean filterByNewEmployees) {

        Set<String> validJobIds = retrieveReplicatedJobIdsThatMatchFilterCriteria(companyId,
                qualifiesForPayrollPeriod,
                filterByEmployeeLeaving,
                filterByPayrollException,
                filterByNewEmployees
        );

        //Get the valid jobs from company server
        List<Job> allJobs = retrieveAllJobsFromCompanyService();

        if (allJobs == null) {
            return new ArrayList<>();
        }

        List<Job> validJobs = allJobs.stream()
                .filter( j -> validJobIds.contains(j.getJobId()) )
                .collect(Collectors.toList());

        return validJobs;
    }

    private Set<String> retrieveReplicatedJobIdsThatMatchFilterCriteria(long companyId, boolean qualifiesForPayrollPeriod, boolean filterByEmployeeLeaving, boolean filterByPayrollException, boolean filterByNewEmployees) {

        Specification where = createSpecification(companyId,
                qualifiesForPayrollPeriod,
                filterByEmployeeLeaving,
                filterByPayrollException,
                filterByNewEmployees
        );
        List<PayrollReplicatedJobData> jobData = payrollReplicatedJobDataRepository.findAll(where);

        if (jobData == null || jobData.size() == 0) {
            return new HashSet<>();
        }

        return jobData.stream()
                .map( jd -> idCryptoTool.encryptId(jd.getJobId()) )
                .collect(Collectors.toSet());
    }

    private List<Job> retrieveAllJobsFromCompanyService() {
        List<Job> jobs = null;

        try {
            companyClient.getSession().setSecurityToken(tokenUtil.getTokenIfAvailable());
            jobs = companyClient.getJobController().listFullViaAdmin(null, 0, 1000);
        } catch (IOException e) {
            LOG.error("Exception retrieving job info", e);
        }

        return jobs;
    }

Things to notice:

  • the main logic of the original method is now much easier to read and understand at a glance. The details about how to accomplish each part of the main logic are just that, details, which are hidden from the main logic.
  • There's no more error handling noise, just the main logic in there.
  • The usage of Java 8 filter allows us to skip the original "add to collection / remove from collection" fuzz.
What do you think? Is it really better? Did it get any easier to read? Could it be even better?

Tuesday, March 29, 2016

Saving execution time by using the proper tools

A few weeks ago in my current job, I picked a ticket about fixing an issue with one of the reports we generate. While debugging and looking into the code, I found a class called TranslationService, which was basically responsible for handling i18n. In other words, it would take a message key and retrieve the proper translation from a message bundle.

Here's the actual code:

@Service
@Slf4j
@Service
public class TranslationService {

    private static final String BASENAME = "messages";
    private static final String FILE_EXT = ".json";
    private static final String YAML_FILE = "translation.yml";
    private String rootPath;

    public String getMessage(final String key, final Locale locale) {
        if (key == null) return "";

        JsonMapperWrapper wrapper = new JsonMapperWrapper();
        InputStream inputStream = getClass().getClassLoader().getResourceAsStream(getResourceName(locale));

        if (inputStream == null) {
            LOG.warn("Not found message file: " + getResourceName(locale));
            return key;
        }

        Map messages;
        try {
            messages = wrapper.readValue(inputStream, Map.class);
        } catch (IOException e) {
            LOG.warn("Error parsing the message file: " + getResourceName(locale));
            return key;
        }

        String messageKey = verifyKeyValue(key);
        return messages.containsKey(messageKey) ? messages.get(messageKey).toString() : key;
    }

    private String verifyKeyValue(final String key) {
        String newKey = key;

        if (!key.contains(".")) {
            newKey = "attribute." + key + ".label";
        }
        return newKey;
    }

    private String getResourceName(final Locale locale) {
        Locale newLocale = locale;
        if (locale == null) {
            newLocale = Locale.GERMANY;
        }

        if (rootPath == null) loadRootPath();

        return rootPath + "/" + BASENAME + "." + newLocale.getLanguage() + "-" + newLocale.getCountry() + FILE_EXT;
    }

    private void loadRootPath() {
        YamlPropertySourceLoader sourceLoader = new YamlPropertySourceLoader();
        try {
            PropertySource<?> propertySource = sourceLoader
                    .load(YAML_FILE, new ClassPathResource(YAML_FILE), null);
            rootPath = propertySource.getProperty("messageFiles.path").toString();

        } catch (IOException e) {
            LOG.warn("Error reading the configuration file: " + YAML_FILE);
        }
    }
}

At first, this might seem like a reasonable piece of code, but take a closer look. Take a look at getMessage method. Notice that, every time this method is called the entire message bundle will be fully loaded into memory, parsed and put into a Map, just for the sake of retrieving one message key.

Now consider the scenario where this is invoked multiple times for each page of the report that is being generated. I guess you can imagine how slow this can get, right?

The first question I asked myself when I looked at this was: "why are we not using Spring's built-in i18n facilities for this?" (a tiny bit of background, this is a RESTful Web API, powered by Spring Boot and consumed by a PHP frontend). The next one was: "why are we using JSON files for i18n, instead of .properties?"

I asked around and no one actually knew the answer for neither, but my PO had a hint about it: both backend and frontend had a copy of the message bundle, because someone wanted to be able to simply copy the JSON i18n file used by frontend and paste it on backend codebase. Which, in turn, probably lead to all of this boilerplate code designed to deal with this unnecessary message bundle (after all, it isn't that hard to transform a simple key-value JSON file into a properties file, even if manually).

This smelled funky, and I thought to myself that being able to cope with this "requirement" wasn't so strong of a reason to harm backend's performance so much. And so I decided to spend some time refactoring TranslationService so that it'd internally use Spring's MessageSource instead of all of the previous mess.

After some changes, this was the final state of the code:

@Service
public class TranslationService {

    @Autowired
    private MessageSource messageSource;

    public String getMessage(final String key, final Locale locale) {
        if(StringUtils.isBlank(key)) {
            return "";
        }

        String messageKey = appendPreffixAndSuffixIfNecessary(key);
        Optional<String> messageInABundle = retrieveMessageFromBundle(messageKey, locale); // I'm a The Police fan

        if(messageInABundle.isPresent()){
            return messageInABundle.get();
        }

        return key;
    }

    private String appendPreffixAndSuffixIfNecessary(final String key) {
        String newKey = key;

        if (!key.contains(".")) {
            newKey = "attribute." + key + ".label";
        }
        return newKey;
    }

    private Optional<String> retrieveMessageFromBundle(String messageKey, Locale locale) {
        try {
            return Optional.of(messageSource.getMessage(messageKey, new Object[]{}, locale));
        }
        catch (NoSuchMessageException e) {
            return Optional.empty();
        }
    }

}

Besides shorter and cleaner, this very simple change saved about 12% of total execution time when generating a 2 pages report. I didn't actually measure how much time it'd save for a bigger report, but we can safely expect this economy rate to increase as the number of pages increase.

The lesson to be learned (or relearned) in today's episode is: be very careful with requirements that aim to make Dev team's life easier. Requirements should, in general, be about making user's life easier. If someone in the team comes up with an idea that is supposed to make her life easier, regardless of adding value to the customer or user, be really careful about it and try to measure the positive and/or negative impacts for whoever uses your software. In this case, report generation is already a slow enough process, for which we already have customer complaints. Implementing a customized i18n handling solution just to save a couple of minutes converting JSON to properties only led us to deliver an even slower report.

Thursday, October 22, 2015

Dealing with bugs without stop delivering features

What can you do when you have multiple bugs to fix in your production environment but also need to keep your features moving forward?

Fixing multiple bugs is a mandatory activity, after all, you have to keep earning the money people pay for your product. But you also need to constantly add new features in order to keep those customers with you and attract new ones.

I once went through a situation where almost everyone on the team would ask each other: "Have you been able to work on any feature this sprint? I've only worked on bugs for the last two weeks!". Such a situation sucks. Not only for developers, who can't get their hands onto new stuff, but also for your product. Having such a huge amount of bugs out there indicates issues with your products internal quality and blocks you from adding new value to your customers.

So, our approach to deal with that was split into two steps:

Don't accept tickets to be closed without tests

This was a major change in the way we used to work, and it took really long for everyone to get used to it and to eventually see its results.

Up to that moment, every time a bug ticket was created on our Jira, someone was responsible for writing the fix and release it on production environment. Someone else would take a look at the changes and, optionally, run it locally to see if it worked. Given our bugs statistics this was clearly not working. Also, as I explained in a previous post, our test suite was quite problematic by that time, so, we needed to increase our test coverage immediately.

So, the first thing we agreed upon was that every ticket had to have tests. You didn't need to do TDD (although it'd have been great), just provided tests that check at least the positive and negative scenarios of the change you pushed.

Results didn't took too long to show up. One of the developers that was most resistant to write tests was also one of the first ones to say: "Ok, I just witnessed a live bug being prevented by a test that is running in our development branch, now I see the value of tests". This person still is, up to this moment, quite resistant to write tests, but it was nice to hear him saying that.

And this was a long road to go through. We've repeatedly agreed on taking this approach in several retrospectives, and it took a couple of months until this was no longer an issue. This makes me quite happy to be honest. To look back and see how resistant people were to write tests, and see how well they do it now is a big victory to me.

Once our test suite was recovered and the "please add tests to your ticket" policy was more or less established, we moved on to the next step.


Stop creating tickets for things that aren't bugs

At some point, we noticed that approximately 1/3 of our tickets were closed as "duplicated" or "won't fix". There were multiple reasons for that, such as:

  • Customer support team would sometimes receive questions they couldn't quickly find an answer for (because they were in the phone with a customer) and ended up creating tickets for dev team to check something. And that something wasn't really a bug, it was a known problem that was already being taken care of or simply had no solution.
  • Product owners sometimes had questions about how some part of the system worked, but they didn't know who to ask, and hence they created tickets that would afterwards be instantly closed after a 5 minutes conversation that could have previously taken place anyway.

This was clearly a communication issue. What we did to deal with that was:

  1. Make information more promptly available for customer support, so that they wouldn't need to create tickets about things that weren't broken;
  2. Make sure we updated that information every time (or, at least, as frequently as possible) our business rules changed;
  3. Assign people as focal points for parts of that system and make that information reachable for everyone, so that product owners could direct their questions and avoid creating unnecessary tickets.
  4. Make a team leader from dev team join a weekly meeting with a product owner and a customer support representative, to go through their issues and quickly talk about what had recently changed.

That already decreased a lot the rate of "duplicated" and "won't fix" tickets. Instead of having to spend time looking into several tickets that weren't really problems, developers started to have more time to focus on what really needed their attention.

With a lower defect creation rate, we were able to take a step further.

Choose someone to be in charge of bugs

We needed to find a proper workload balance between bugfixes and features, to make sure we wouldn't be stalled without delivering features for too long.

So, what we did was assigning someone in each team to be the sole responsible for taking care of not-so-urgent bugs. For one week, this person would be responsible for fixing bugs, leaving the rest of the team free to work on features, unless a priority bug would come in, in which case anyone from the team (not only that week's bug fixer) would be responsible for tackling that bug.

The results of such separation were amazing. Features started to disappear from the board, and the teams began to suffer from other problem: not having enough "Ready" tickets fast enough. Now, instead of complaining about not working on a single feature for 2 weeks, people would actually complain about not having anything to do on that particular day. This (not so bad) problem remains currently unsolved :)

One interesting point to observe here is that all of these changes have been identified and dealt with by the team as outcome of retrospectives. To me, it's amazing to observe how much we achieved in a reasonable amount of time by just talking about problems and taking action to fix them. Retrospectives are really a very powerful tool.

Wednesday, October 21, 2015

Beginner's guide to automated testing

A few years ago, I was lucky enough to cross the way of a bunch of guys which were very excited about putting the Agile mindset in action. I didn't know the first thing about testing, and I felt bad about that. Knowing how little I knew and practiced testing made me feel a very incomplete IT professional.

Ever since, I've learned a few things along the way, and have developed my own small set of ideas about how to write automated tests, and how to make the best use of them in many aspects. I got to understand that tests sometimes work not only at the machine level, but also at the psychological level, making programmers feel safer to do their jobs.

So, I'd like to use this post to share some ideas of my own about automated tests. I don't mean to say I'm a test master at all. Many people have way more knowledge than me in this matter, I am probably wrong in many (if not all) of my ideas, most of them are probably quite simple and obvious, and I fully recognise there still is a long way ahead of me. 

Also, automated testing is a really broad subject. There are whole conferences where people from all around the world gather to talk about different aspects of this discipline: distributed testing, automated acceptance testing, mobile testing, and so on, so forth. I have no intention to cover everything here. I'll mostly focus on the smallest and most basic kind of testing: automated unit testing. Most of the concepts presented in the next paragraphs can be adapted and expanded to other situations, scenarios and kinds of testing, but I don't want to bore you with my extensive lack of knowledge, at least not too much.

Nonetheless I'd like to share what I've learned so far with you, dear reader. I hope you enjoy it. If you happen to have any complaints, questions, comments or suggestions, please feel free to add comments to this post.

So, the very first thing is:


Automated tests must have control over dependencies

The whole idea of a test of any kind (automated, manual, hand written on a piece of paper) is verifying whether the outcome of a piece code matches your expectations. 

At a unit level, to achieve that, your test must have control of all variables, to make sure your code will do what is supposed to do under the foreseen circumstances. If you have no control over what surrounds your code during a test, then your code becomes unpredictable and your test becomes worthless.

So, generally speaking, if you're testing a method that depends on, or delegates execution to another class or component, you should make sure this other component will behave as expected. There are several ways of doing it. The most usual is to create a mock object that fakes that dependency, and reacts in whatever way you define. 

On the other hand, faking a dependency might be extremely expensive or even not make any sense at all. For instance, consider you have some persistence layer code you want to test, for instance, a DAO. Creating support code to fake the database can be quite error prone and expensive. Besides, if the sole responsibility of that piece of code is to interact with the database, it wouldn't make much sense not to hit the database and make sure your query works as expected. In this case, the concept is the same: you must have control over external dependencies. The difference is that now you have a big fat dependency: a database. 

How do you keep control of such a thing? By making sure you always know the state of the database before and after your code runs. That means creating all tables from scratch before running, and dropping everything after the execution finishes. Or, at least, making sure all tables are empty before and after your test runs. 

But why is it so important to have control over external dependencies?

Consider you have tests running against a given database. Let's call it "Dev DB". This DB keeps changing: tables and rows are added and deleted by multiple users or processes. You'll end up in at least one of these situations:
  • Your tests won't ever be able to assess the DB state before its execution, preventing them to know what to expect afterwards, resulting in tests that can't possibly test anything;
  • Your tests constantly break, since the pre-conditions and post-conditions expected by them keep changing;
  • As to avoid breaking all the time, your tests are so vague that even when you introduce a serious disruption or a major bug, no tests break, everything remains green;
  • Since your tests break all the time anyway, no one really cares when your CI server gets red: it could either mean you introduced a new bug or new rows have been added to DB, changing the result. Recent history says the odds are the second one is more likely, so you just sit and wait for someone to fix that line of test code;
So, in the end, having no control over external dependencies makes the team lose confidence on the tests and, with time, they become obsolete and useless.


Automated tests should test one thing at a time

A good pattern to follow when writing tests is the so-called "Build-Operate-Check". In BDD terms, it translates to the "Given-When-Then" triad. Meaning a test should ideally have 3 sections:

  1. Prepare everything your needs to test to run. Set the ground and the pre-conditions that your test and your production code need for the scenario you'll be testing;
  2. Run the target method;
  3. Check if the post conditions have been met, or, in other words, verify if the outcome of your production code meets your expectations and if the expected post-conditions have taken place.
On top of those 3 sections, a test should also exercise only one scenario and a method at a time. But why only one method at a time?

Consider you have a test like this:

public class MyTests {

    MyTargetClass targetClass;

    @Before
    public void setup(){
        targetClass = new MyTargetClass();
    }

    // ... other tests omitted

    @Test
    public void testMultipleMethods(){
        //given
        targetClass.createPreconditions();

        // when
        String outcome = targetClass.targetMethod();

        // then
        assertEquals("outcome", outcome);
    }
}

Now, what happens if a bug is introduced into createPreconditions()? You'll end up with at least two failing tests: the one that checks whether createPreconditions() works as expected, and the one above. So, even though targetMethod() still works as expected, a bug introduced into a production method that shouldn't have anything to do with the method being tested here will still break it. You end up creating a dependency between your tests, which is not at all desirable.

And why one scenario per test? 

Let's use another example. Typically, your production code may have to deal with several scenarios, such as: 
  • the input is null; 
  • the input is not null but also not valid; 
  • the input is valid and generates outcome A;
  • the input is valid and generates outcome B;
  • ...
Now, what happens if you test multiple scenarios within the same test method? The very same thing: a bug that just affects the first scenario may start breaking tests that aim at verifying something completely different, and you create a dependency between your tests.

This goes along with some of the things Uncle Bob says in Clean Code
  • there should be only one concept for test;
  • tests should be independent;
  • tests should be self-validating;
And why do we want to be so specific about what do we test? Well, is there anything more frustrating than not knowing why your test is broken? 

We want tests to be small, independent and self contained so that, whenever it breaks, we can quickly and safely spot out what went wrong and get it fixed. If you have to spend hours trying to figure out why a test doesn't work, people will start disregarding and not fixing it, which, in turn, will lead to a useless test suite.



Be specific about the outcome

Along the years, I have seen many tests written like this:

public class MyTests {

    MyTargetClass targetClass;

    @Before
    public void setup(){
        targetClass = new MyTargetClass();
    }

    @Test
    public void testOutcomeSize(){
        // when
        List<String> outcome = targetClass.targetMethod();

        // then
        assertEquals(2, outcome.size());
    }

    @Test
    public void testOutcomeIsNotNull(){
        // when
        String outcome = targetClass.otherTargetMethod();

        // then
        assertNotNull(outcome);
    }
}

Can you spot the problem within these two tests? At first, they seem to be OK, right?

But are they really checking if their target methods work as expected? Or are they just testing if the target methods provide any outcome at all? Take a look at this improved version:

public class MyTests {

    MyTargetClass targetClass;

    @Before
    public void setup(){
        targetClass = new MyTargetClass();
    }

    @Test
    public void testOutcomeSize(){
        // when
        List<String> outcome = targetClass.targetMethod();

        // then
        assertEquals(2, outcome.size());
        assertEquals("first", outcome.get(0));
        assertEquals("second", outcome.get(1));
    }

    @Test
    public void testOutcomeIsNotNull(){
        // when
        String outcome = targetClass.otherTargetMethod();

        // then
        assertNotNull(outcome);
        assertEquals("expected outcome", outcome);
    }
}

Can you see what a huge difference the new lines make?

The first version checks if an outcome is generated, whereas the second one checks if the proper outcome is generated.

The first version would consider a list containing "something" and "nothing" to be valid, regardless of the order they show up. But what we really want is to make sure that targetMethod() returns a sorted list, where "first" is the first element and "second" comes last. And that's a huge difference from a functional perspective.

Imagine your customer logging into your system to retrieve the invoices from the last two months in order to pay you, and getting instead the last two songs you have heard in your iPod? That's money going down the drain!

The same thing apply to the second test. Imagine your user tries to visualize information about his most important customer and gets information about a customer from his competitor. That could cause serious issues to both of your customers, not to mention their confidence on your product.

The same principle applies to checking exceptions. What is wrong with the test below?

public class MyTests {

    MyTargetClass targetClass;

    @Before
    public void setup(){
        targetClass = new MyTargetClass();
    }

    @Test(expected = Throwable.class)
    public void testOutcomeSize(){
        // given
        targetClass.setPrecondition(
            Preconditions.GENERATE_BUSINESS_EXCEPTION);

        // when
        targetClass.targetMethod();
    }
}

At first, it seems alright, no? But can you really be sure that your production code is failing for the reason you wanted it to fail? Can you differentiate a NullPointerException from the business specific exception that your code is supposed to raise? What about improving the test a little bit to look more like this?

public class MyTests {

    MyTargetClass targetClass;

    @Before
    public void setup(){
        targetClass = new MyTargetClass();
    }

    @Test(expected = MyBusinessException.class)
    public void testOutcomeSize(){
        // given
        targetClass.setPrecondition(
            Preconditions.GENERATE_BUSINESS_EXCEPTION);

        // when
        targetClass.targetMethod();
    }
}

Looks better, right? Now I can be completely sure that my code is throwing the exception it's supposed to throw on that specific scenario. And, this way, if someone changes the code in order to not throw that exception anymore, I will have a test telling that person to think twice about how much sense that makes. And I can also be sure that, if a is NullPointerException thrown, my tests will not comply with it and will also break.

For the same reason, don't write tests that interact with more than one target class. If something breaks on class A, you want to be precise about the impact of that change, you don't want to fix a million tests that were checking something completely different and which context had nothing to do with class A. Always prefer being minimalist about the scope of your tests: test one class and one method at a time; use different test classes for different target classes.

Some people will even go further and state that tests should have a single assertion. Although I respect this point of view, I don't really like it. And the reason for that is a piece of production code may have several outcomes. For instance, a given method may be responsible for storing a change in the database and invoke a notification component to send an e-mail. One action fired two consequences.

To me, tests are also about context. So, if in the context of a given call to a target method, two consequences have taken place, I'd rather have this properly checked under the same test and context, instead of having two almost identical tests (with same preconditions and action being fired) with just different checks for the expected outcomes. But this is really just a matter of personal preference.


Tests should be fast

An often neglected aspect of automated tests is that they also influence on the psychological side of IT. Fast feedback is a powerful tool to stimulate a team into using tests. Whenever someone changes anything, that someone can quickly know the side effects of that change. If that developer would have to wait half an hour to know that, he/she would most likely just push the change before the tests are done and let the rest of the team down, having to deal with a break they didn't do neither asked for.

Fast feedback is also very powerful when you have a large collection of broken tests: when people can quickly see the positive outcome of the effort they spent on fixing a couple of tests, it makes them feel good about it. When that large broken test suite is finally completely fixed, it's a victory for the team, as they will start to have a very reliable way of knowing what is the impact they didn't think of in the rest of the codebase.

That's one more reason to minimize integration and functional tests. Those kinds of tests add great value, but the thing is that they can be really slow, specially when there are multiple of them, and, therefore, they should be kept to be a bare minimum.

If you cannot give up on a large suite of integration and functional tests, what you can do is make unit, integration and functional tests run in separate moments.

In a scenario where it takes more than 10 minutes to run you entire test suite and then push some code changes, odds are your team will give up on waiting and will push untested code all the time, letting Jenkins do the hard work and, hence, breaking up the code base completely.

In this case, what you can do is have separate commands for running each type of test and agree with your team what is the bare minimum that needs to be run before pushing something. In teams I worked, the general agreement was that running unit tests before pushing was mandatory, since we had not so many integration and functional tests, and they happened to be quite stable. But this should be really agreed upon on a case by case basis, depending on what the team has to deal daily.


There's so much more out there!

I guess that's all I had to share. As I said, there's a lot I don't know, and I have no intention to pretend otherwise. But you don't really need to worry about using all (or any) of this. If you just use TDD, you get all of these topics instantly covered for free. But TDD is a completely different topic, which I'm very far away from mastering.

What about you? What other test related good practices do you have? What has worked or not for your team under a particular circumstance? Feel free to add comments and enrich the discussion. 

Monday, September 28, 2015

Recovering a broken test suite

In one of my most recent assignment, I joined a team that was made of fantastic developers. They knew everything about their product by heart and were extremely nice, the team had a very good mood. Nevertheless, the first time I looked at their (and, from that point on, our) Jenkins, I was terrified.

We had something people had agreed on calling a test suite, but that was completely worthless. At that point, we only had integration tests running, and they had been broken for quite a while. I spent a couple of minutes looking at our unit tests and found out some of them didn't even compile, because they referred to production code that had changed ages ago.

I was really concerned about that. I really value internal and external quality and, for some reason, I had created the expectation that that team shared the same values. On the other hand, I really wanted to be there, I didn't want to quit that team so fast. What to do, then?

Well, if life gives you a lemon, grab a bottle of tequila and some salt, and make a hell of a party out of it!

Open your heart

The first thing I did back then was quite simple: talk about it. In my first retro, I honestly thought I would be ejected from the team by the guys. I spoke my mind and expressed my concerns. I let the team know I cared a lot about tests and that it'd be awesome if we fixed that. And, in fact, we could easily do it, because we were so much better than that.

For my surprise, most people agreed with me. Some said they just didn't know how to write good tests. The most resisting ones said they didn't see the value added by tests, while others said they found it more comfortable to write integration tests than unit tests, since they didn't need to write mocks, etc.

Step one: training and knowledge sharing

I was quite surprised by this feedback. I had those guys in such high standards (and myself in such low ones) that I actually got amazed of actually hearing them asking for help to write good tests. And so I prepared a brown bag session, along with a Google Doc, with examples, default configurations, best practices, etc, about tests. And the feedback on that was quite fast too: on the very next retro, one of the guys said he was quite happy to have some kind of knowledge being shared. But we couldn't stop there, we had to fix those tests somehow.

Step two: set the example

We had to start fixing somewhere. And so, as everybody agreed tests were desirable, I started by simply writing tests for my stuff. If I wanted to have those tests so bad, I had to set the example. Every task I would take, no matter how big or how small it was, I'd write tests that covered 100% of the associated production code. This way, I also started to create a codebase of examples when people needed help writing their own tests.

Step three: take responsibility

I also started failing tasks that didn't provide tests. No matter how big or how small a task was,  no matter who had written that code, I'd reprove it and nicely ask the author to write tests to validate the changes. This way, people would start to see what their decision to fix our test suite really meant and (hopefully) do the same to others, pushing everyone into worrying about tests.

Step four: separate execution cycles

The thing about software development is that we always think we're dealing with software. But, in the end, we're not. We're dealing with people. People write code, there's no code without a human striking the keys of a keyboard. Hence, psychology is of essence when you're trying to push changes in an IT environment. But how does psychology fit in here?

The thing is: nobody likes huge tasks that seem to never end. No one likes to play Hercules for ages. And fixing our entire test suite would take ages. So we had to break it down into smaller pieces, as not to bore people to death and get nowhere in our goal to fix our test suite.

So, what we did next was splitting test according to their "cycle". What do I mean by "cycle"?

Our test suite had different kinds of tests. We had:

  • unit tests;
  • integration tests, that tested several "business" classes at once;
  • "remote" tests, i.e., tests that ran HTTP calls against other API's like Facebook, Foursquare, Yelp, etc (in other words, contract tests);
  • functional tests, that started a Geb session in a chromedriver instance, and tested the app running in our dev server;
  • API functional tests, that ran HTTP calls againt our API running in our dev server.
Not all of those types were executed independently. If I remember correctly, we ran unit tests in the same "cycle" as remote tests, meaning they were ran together.

Needless to say, our functional and API functional tests were as unstable as possible, since they used the same version of the app in dev server the rest of the team would use to work and test, leading to extremely unreliable test data and easily breaking tests. 


And so we began our Herculean task by separating those "cycles". Meaning, we created separate test folders, separate Jenkins jobs, everything separated so that whenever we ran unit tests, we would only run unit tests. Same for integration tests. Same for remote tests, etc. And we focused on first fixing unit tests, since they were smaller and provided fastest feedback. The mind games had began.

Step five: unit test challenge

And so, to stimulate people into helping fixing our test suite, we made a kind of game out of it. Everyday, someone would fix between 2 or 5 unit tests. Not 5 test files, just 5 test methods in any file. Just that. On top of that, we kept pushing for tests being provided along with the new code we delivered. And everyday that person would pass the "test fixer" hat to someone else. And then the game started to change.

With so little responsibility, there was no possible argument against doing it. And as everybody else was doing it, you couldn't just run away from it (and some people tried to to). Also, feedback plays an important role here: when you're able to quickly see the result of your work, you feel more motivated, and I believe people started to feel good about it. If they didn't get to feel good about improving our tests, they would at least feel good for getting rid of it pretty fast.

When you read it like this, it seems like it was piece of cake, but actually it was not. For two months I had to play the bad cop, reminding people about doing it and passing the hat along to the next one. It was frustrating sometimes, but quite rewarding in others, such as when a guy fixed 30+ tests in his round. Or when one of the most resistant guys said "ok, I just saw a production code that got self fixed by a test someone added yesterday, now I see the value of it". But most of the time, it requires patience and discipline. It took us two months, but eventually we got there and finally made our unit tests look green. Finally we had some kind of a minimal security net that would allow us to do some safer refactoring.

Step six: the challenge continues

We still had a long way ahead of us: our integration test were still broken, and we also had remote tests to take care of. So, we changed the game a little bit: now the goal was not just fix integration tests, but also refactor them into unit tests where applicable, as to decrease the size of our integration tests suite and let it run faster.

Not only that, we also had to stop using random data from dev database. So what we did was configuring the integration tests runner to always create all tables from scratch before running the tests and drop them all in the end. We also configured a script to only insert very basic test data that absolutely all integration tests would need. If a single test needed test data, it would just have to take care of inserting it, it wouldn't be available for general use, since we wanted to make sure we knew what was lying in the DB and prevent issues with random data being used.

But this phase was much easier. By the time we got here, writing tests was already running through everyone's veins and with a little bit of examples and default configurations on people's machines, it went much much faster. And when we finished it, the celebration was huge, too: for the first time in ages, we had both unit and integration tests running smooth.

Step seven: preparation runs in parallel

While we were all working on fixing unit and integration tests, some of us saved some time to pave the way for next steps, and started fixing, refactoring and creating examples of remote and API functional tests for the others to build on top whenever they got there. And so life started to get easier for everyone.

A still unfinished job

Many months after the start of this saga, there are still open issues. There are still open tickets in Jira for refactoring API functional specs, but those are not so critical because the ones that are still there are pretty stable, the most unstable ones have already been refactored. And the functional tests are still not very reliable, they're the next goal.

The results we achieved with this approach were quite impressive.

A quick explanation about the product: its core purpose is to integrate with social networks, meaning it runs HTTP calls all day long. In the beginning of this adventure, the success rate of such HTTP calls was highly unstable: on one day, 90% of HTTP calls would be successful, just to fall under 30% on the very next day, leading to an average success rate of ~30%. After four months of test fixing, the average success rate increased to around 80%. The most practical and visible consequences of this change was less time (and stress) spent fixing production bugs, leaving more time to work on new features.

Even now, I feel really happy for having joined this and see Jenkins statistics showing there are more than 2k unit tests now, the triple of what we had 6 months ago :)

Tuesday, May 26, 2015

Building a simple (and incomplete) RESTful API with Spring Boot and Hibernate

Today I wanted to try Spring Boot and see how easy it's supposed to be. All I wanted was to implement a simple RESTful Web API, with one or two operations, just to see persistence working, and the result was much better than I expected.

I took a look at a few tutorials / blog posts to help me in this task:

  • http://spring.io/guides/tutorials/bookmarks/
  • http://shruubi.com/2014/12/03/spring-boot-hibernate-and-spring-security-a-step-in-the-right-direction-for-java/
  • http://docs.spring.io/spring-boot/docs/current/reference/html/boot-features-sql.html#boot-features-configure-datasource
  • http://blog.netgloo.com/2014/10/27/using-mysql-in-spring-boot-via-spring-data-jpa-and-hibernate/
  • http://spring.io/guides/
And the results were soooo much better than I had expected!

First of all, I'm used to work with Grails. Hence, I didn't want to have too much hassle, trying to figure JPA configurations, etc, all I wanted to do is write code the way I was used to do. And I pretty much managed to do that!

The first good thing is my entity class was 100% Hibernate as I used to know it, which is good:


@Entity
class Doctor {

    @Id
    @GeneratedValue
    Long id

    @NotBlank
    String name

    @NotBlank
    String street

    String streetNo
}

My repository, on the other hand, is amazingly much easier, thanks to Spring Data! Grails users will quickly understand what's going on here. 

I didn't want to write a full blown example, just really wanted to show off entities creation and retrieval at work, hence the two methods below (and the "incomplete" on this post's title):

@Repository
interface IDoctorRepository extends CrudRepository<Doctor, Long> {

    Iterable<Doctor> findAll()
    Doctor save(Doctor d)
}

My controller class is also pretty much standard, REST-wise:

@RestController("/doctors")
class DoctorController {

    @Autowired
    IDoctorRepository doctorDao

    @RequestMapping(method = [RequestMethod.GET])
    public List<Doctor> findAll(){
        return doctorDao.findAll()
    }

    @RequestMapping(method = [RequestMethod.POST])
    public Doctor create(@RequestBody Doctor d){
        return doctorDao.save(d)
    }

}

For persistence to work, all I had to do was create a file called src/main/resources/application.properties, with some Spring data configs (this example uses HSQLDB):

spring.datasource.url=jdbc:hsqldb:mem:kesehatan
spring.datasource.username=sa
spring.datasource.password=
spring.datasource.driverClassName=org.hsqldb.jdbcDriver
spring.jpa.hibernate.ddl-auto=create-drop
spring.jpa.hibernate.dialect=org.hibernate.dialect.HSQLDialect
spring.jpa.hibernate.show_sql=true

And, for the grand finale (yes, we're already there!), my Application class:

@SpringBootApplication
@EnableJpaRepositories
@EntityScan
class KesehatanApp {

    public static void main(String[] args) {
        SpringApplication.run(KesehatanApp.class, args);
    }
}

And that's it! It just works!

Just for the sake of a better understanding, the first annotation declares this is a Spring Boot app. The second one, as it says, enables our repository class to work, while @EntityScan enables Hibernate entities to be used.

Really, really easy, really nice to use. It's nice to see some of Grails stuff being re-used around :)

For those interested in running it, feel free to clone it at https://github.com/felipecao/kesehatan.