Understanding my permalink service tests

Question

I started writing unit tests cases recently. For now, I created only "perfect case" where there is no error. However, the test seems to me difficult to maintain and difficult to understand.

How can I make it better for other people to understand?

All permalink attributes are set within the service itself I'm testing (PermalinkService.java).

My test

@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration(classes = Configuration.class)
@FixMethodOrder(MethodSorters.NAME_ASCENDING)

public class PermalinkServiceTest {

    @Autowired
    PermalinkService permalinkService;

    private static Permalink PERMALINK;
    private static Validacao VALIDATION;
    private static DBObject PECA;
    private static BasicDBObject COMMENTS;

    @BeforeClass
    public static void init(){
        PERMALINK = new Permalink();
        VALIDATION = new Validacao().setId("55534e63ccf2f879efcbd2a3");
    }

    @Test
    public void testPermalinkServiceA_createPermalinkWithoutValidation_permalinkWithAllFilesAndWithoutValidation() throws Exception {
        PERMALINK = permalinkService.createPermalink(VALIDATION.getId(), "Permalink sem validação");
        Assert.notNull(PERMALINK);
    }

    @Test
    public void testPermalinkServiceB_createPermalinkWithValidation_permalinkWithAllFilesAndValidation() throws Exception {
        PERMALINK = permalinkService.createPermalink(VALIDATION.getId(), "Permalink com validação");
        PECA = (DBObject) JSON.parse(PERMALINK.getPecas().get(0).toString());
        Assert.notNull(PERMALINK);
    }


    @Test
    public void testPermalinkServiceB_getAllPermalinks_getAllPermalinks() throws Exception {
        Assert.notNull(permalinkService.getPermalinks(VALIDATION.getId()));
    }

    @Test
    public void testPermalinkServiceC_getPermalink_getPermalink() throws Exception {
        Assert.notNull(permalinkService.getPermalinks(VALIDATION.getId(), PERMALINK.getId()));
    }

    @Test
    public void testPermalinkServiceD_setStatus_setNokStatus() throws Exception {
        String status = "nok";
        String email = "[email protected]";
        permalinkService.setStatus(VALIDATION.getId(), PERMALINK.getId(),
                String.valueOf(PECA.get("id")), status, email);
    }

    @Test
    public void testPermalinkServiceE_addComment_addComment() throws Exception {
        String comment = "comentario teste";
        String email = "[email protected]";
        PERMALINK = permalinkService.addComment(VALIDATION.getId(), PERMALINK.getId(), String.valueOf(PECA.get("id")),
                comment, email);
        DBObject pecas = (DBObject) JSON.parse(PERMALINK.getPecas().toString());
        DBObject peca = (DBObject) JSON.parse(pecas.get("0").toString());
        DBObject comentarios = (DBObject) JSON.parse(peca.get("comentarios").toString());
        COMMENTS = (BasicDBObject) JSON.parse(comentarios.get("0").toString());
        Assert.notNull(PERMALINK);
    }

    @Test
    public void testPermalinkServiceF_removeComment_removeComment() throws Exception {
        permalinkService.removeComment(VALIDATION.getId(), String.valueOf(COMMENTS.get("id")), PERMALINK.getId(), "");
    }

    @AfterClass
    public static void testPermalinkService_removeFromMongo_removeFromMongo() throws Exception {
        MongoCollection mongoCollection = DBSingleton.getJongo().getCollection("validacao_permalinks");
        mongoCollection.remove("{ _id : #}", PERMALINK.getId());
    }
}

Answer 1

You're right, we can clean things up a little bit.

First of all the method naming: I like to use a schema which tells me the unit under test, the scenario I test and the expected outcome. More specifically: [unitUnderTest_scenarioTested_expectedOutcome].

This would turn something like this:

testPermalinkServiceC_getPermalink_getPermalink()

into this:

permalinkService_withValidIds_returnsPermalink

When I look at the results of the test I can immediately see everything relevant to me:

• What isn't working?
• What am I doing when it isn't working?
• What did I expect to happen?
• What actually happens?

2 more notes about your method naming:

• Don't prepend them with test: you already you know it's a test because of the @Test attribute. The only exception to this is if you have to do this because of legacy reasons (though I don't think this applies here).
• Don't force an order on your methods using the @FixMethodOrder and ServiceA, ServiceB, etc. This brings me to my next, very important, point:

GOOD TESTS ARE INDEPENDENT

The outcome of one test should never impact another one. You bypass this by forcing them to be executed in a determined order but this is as brittle as it can possibly be. If you're going to change one thing, it should be this. When I want to know how a test works, I only look at 4 places:

• Fields
• Constructor
• @Before(Class)
• The test itself

I will never look at another test to see how one test works because that would essentially force me to read and understand every preceding test.

I don't like @BeforeClass when you're not actually doing something that can't be done in the constructor. You're forcing your fields to be static for no reason: just put the initialization in your constructor and remove the static modifiers. static is the enemy of unit tests.

Your tests testPermalinkServiceF_removeComment_removeComment() and testPermalinkServiceD_setStatus_setNokStatus() don't contains any asserts. This is not a test! Tests verify an outcome.

At the same time you should also restrict the variables to their least accessible scope. I see a field PECA and COMMENTS which are only used in a a few tests -- make them local variables instead. The less I have to look away from the test, the better.

On the topic of naming: I'd change PermalinkServiceTest to PermalinkServiceTests (you're talking about multiple tests after all). Furthermore: private fields are lowerCamelCase, not ALLCAPS.

Lastly: stick to one English, preferably English! This counts for both messages and variable/class names.

For an introduction to unit testing, take a look at my blog. I expect there to be a followup on that introductory post somewhere in the next 6 months.

Answer 2

Naming

Good names are very important for readability. For example, this:

testPermalinkServiceA_createPermalinkWithoutValidation_permalinkWithAllFilesAndWithoutValidation

is just way too long a name.

You can reduce it's size by removing all references to permalinks (it's in the PermalinkServiceTest class, so it's assumed that the tests test permalinks). It's also not very clear what the A stands for, but it seems to be only necessary because the tests need to be executed in a specific order, which doesn't seem like a good idea

The name also contains more duplication, as it contains WithoutValidation twice.

A shorter name might be testCreateLinkAllFilesNoValidation, or even just allFilesNoValidation (if that accurately described what is tested, which I'm not sure it does; It's hard to say without the actual code, but I'm eg unsure what allFiles are, or how they are used in the test).

Tests building on each other

I don't really like how the tests seem to build on each other; I would prefer if each test is self-contained.

Self-contained tests seem a lot more stable and maintainable, and would also result in easier to understand code. If you are worried about duplication, just extract that code to a method.

This would also get rid of many of the fields, which I think also results in nicer code.

Misc

Many of your tests don't actually contain any tests, why is that?

Answer 3

As already stated, sometimes there's no assertion in the test. And the only assertions present are Assert.notNull, which does not really verify much. Either you can test some properties of the permalinks, or you have a design problem, which can't be fixed in the tests alone.

Moreover, your @AfterClass indicates that you're talking to Mongo. This does not belong to a unit test. It's OK to write such tests, but they're usually way slower than unit tests and complement rather than replace them. Unit tests are the base.

---

PS: All permalink attributes are set within the service itself I'm testing (PermalinkService.java).

So verify that they're set correctly.

---

Each time the test is performed will insert data in the database. Will be unnecessary data, what I do with the method of mongoDB?

Ideally, the data should not be written anywhere. This can be done by wiring the service to use a mock(*) database connection. There are many libraries allowing to create such mocks and also verify that the expected calls (here: inserts) were really issued.

A simpler (yet somehow less performant) possibility is to use an in-memory database, which may be recreated before each test. Still, cleaning it may be faster. In any case, you should verify what has been written.

---

(*) The distinctions between fake, stub, mock are out of (my) scope.