Forem: MichaelFack1

Writing Good Tests (Part 2) - What Makes a Test Good

MichaelFack1 — Mon, 14 Mar 2022 12:05:38 +0000

In the previous post, I ranted about what not to do when writing tests and how to detect a bad test. In this post, I'm going to attempt to provide some pointers as to how you go about writing good tests.

What makes a test good?

Just like good code is a bit hard to pin down, so is a good test, but you will not be in any doubt when you read a good test, especially if it fails as it achieves its purpose - it will tell you exactly what went wrong and what needs fixing!

Let us demonstrate this by having a look at a failure. Consider our first example Test1. If it fails, you will be presented with text similar to "UnitTest1.Test1 failed: false != true". "This is true" said the tautalog. Instead consider the following test:

public readonly CmsService subject;
public readonly Mock<ICmsReadRepository> readRepoMock;
public readonly Mock<ICmsWriteRepository> writeRepoMock;

public CmsServiceUnitTest()
{
    writeRepoMock = new Mock<ICmsWriteRepository>();
    readRepoMock = new Mock<ICmsReadRepository>();
    subject = new CmsService(Mock.Of<ILogger<CmsService>>(), readRepoMock.Object, writeRepoMock.Object);
}
...
const string InvalidKeyDuoInvalidChar = "An invalid key e><ample";
...
[Fact]
public async Task ReadSectionAsync_InvalidCharKey_ThrowsInvalidCharException()
{
    // Arrange
    DefaultSetup();
    // Act
    var taskExpectedToThrow = subject.ReadSectionAsync(InvalidKeyDuoInvalidChar);
    // Assert
    await Assert.ThrowsAsync<InvalidCharException>(() =>  taskExpectedToThrow);
}
...

And the following message

This tells you that an InvalidCharException was expected from the CmsService when calling ReadSectionAsync, with a key containing an invalid char. Now, in this case, a few things could have gone wrong. Perhaps we no longer indicate failure to read with exceptions anymore but use the optional pattern instead, or perhaps we forgot to validate the key. I happen to know that we did not switch to the optional pattern which means the latter is true. Whoopsie.

All of this, I was able to infer from the test case, thus saving me tons of time I could have wasted Bughunting.

Furthermore, with a nice test coverage % and the most important and most common features under test, I can rest easy at night knowing my software is functional and not just a stinking heap of-. And, I also save time not manually testing which is time well spent in my opinion.

The purpose of a test

Without getting too metatheoretical, I would like to briefly discuss what "the purpose" of a test is, explicitly.

Say, I have a CMS service I want to test. I have some assumptions with respect to its functionality: It should be safe, practical and efficient. Moreover, say, that to be safe, it shouldn't allow all characters but only a specific subset of characters. Now I want to assert that my CmsService doesn't accept any of the illegal characters. Therefore, I write a unit test of which the purpose is to verify this. It initially fails, but after the introduction of a guard-clause, the test passes and I believe everything is good. The next day, I want to demonstrate this new behavior to my team, and I open the webpage utilizing the service to update the entries. You input an illegal character, press save and ... It saves!
You dive in to the database to discover a strange sequence of letters prefixed with an &. Somewhere there was a mismatch between the purpose of the test and the actual test.

This is obviously a very constructed example, but my point is that the test did not fulfill its purpose. A more appropriate test to write when verifying that the system does not accept invalid characters in its frontend is a frontend test, or perhaps a system test. Had I written a frontend test, I might have discovered that some middleware translates user input into safe strings that cannot be utilized in injection attacks. Had I written a system test, I might have discovered that the underlying CMS API has protection. It would have been more appropriate to write a unit test when asserting unit behavior, such as "I expect the CmsService to call the CmsApiClient when GetSectionAsync is called".

Conclusion

In the first post we discussed that if a test is not informative and exact, simple, timely and repeatable, then it's a bad test. In this post, we expanded on the concept of a test being good by discussing what a test needs to do; achieve its purpose of validating an assertion that we make about how some part of, or the whole, system works.

We also determined that this can only be done when we know what the assertion is and create a test based on this. In particular, we want the type of the test to match the layer of abstraction. If only there was some literature on the different kinds tests types paradigms that exist...

Remark

If you have any questions, disagree with anything, or if you would like something added to this blog post - leave a comment!

References

My personal experience,
https://docs.microsoft.com/en-us/dotnet/core/testing/unit-testing-best-practices
https://martinfowler.com/bliki/GivenWhenThen.html
https://gist.github.com/wojteklu/73c6914cc446146b8b533c0988cf8d29 (Clean Code by Robert C. Martin summary)
https://docs.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-6.0

Writing Good Tests (Part 1) - What Makes a Test Bad

MichaelFack1 — Thu, 03 Mar 2022 10:51:23 +0000

Unless you're some kind of demi-god, I believe you too have tried writing some new fresh code that didn't execute as you expected at first. Perhaps you forgot a guard-clause, some underlying dependency didn't behave as anticipated or, for some other reason, a bug was introduced to your precious code. What is more important (at least with regards to this blog post) is that you discovered this by compiling the code, running the software and seeing it fail.

In this blogpost, I hope to indoctrinate you into believing in the higher powers; automated testing!

Don't just write tests

Let me know if you've ever been in this situation before; you've just opened your favorite IDE to work on a large (and old) project, you see the infamous area of the project aptly named "Tests", and you shudder a little bit?

I have, and I can tell you why I shuddered - I knew I was the one delegated the tasks of fixing tests that did not succeed anymore. I was the designated Bughunter (trademark pending), and I knew a bug could take days to resolve, even for my more experienced coworkers who had been working on the project for years.

You know why? The tests were bad!

What makes a test bad?

Just like a bowl of cereal, a lot of things can make a test bad, and it is not always clear why a test has gone bad. However, once you've had a spoonful of it - you know!

In this short series, I am first going to supply you with a non-exhaustive list of things to check for when suspecting a test of being bad, and then later on in the series, I will discuss how to write good tests.

I just happened to have a CmsService lying around which implements a Read/Write access string mapping in a database and that I need to test, so, below I'll use it as an example of how NOT to write a test.

Try to spot a few things you find obscene in the following .NET code segment.



public class UnitTest1
{
    [Theory]
    [MemberData(nameof(TestData))]
    public async Task Test1(List<Tuple<int, CmsDbEntry>> entries, string expected, bool boolResult)
    {
        var repoMock = new Mock<ICmsWriteRepository>();
        var dbContextMock = new Mock<CmsDbContext>();
        var repo = new CmsRepository(null, dbContextMock.Object);
        var service = new CmsService(new LoggerFactory().CreateLogger<CmsService>(), repo, repoMock.Object);
        var entriesMock = new Mock<DbSet<CmsDbEntry>>();
        foreach (var (key, entry) in entries)
        {
            entriesMock.Setup(m => m.FindAsync(key)).ReturnsAsync(entry);
        }
        dbContextMock.Setup(m => m.Entries).Returns(entriesMock.Object);
        var result = await service.ReadSectionAsync("some arbitrary constant");
        Assert.Equal(result, expected, ignoreCase: true);
        if (boolResult) {
            repoMock.Setup(m => m.WriteAsync("some arbitrary constant", "Content")).ReturnsAsync(boolResult);
            var boolResult0 = await service.WriteSectionAsync("some arbitrary constant", "Content");
            Assert.Equal(boolResult, boolResult0);
        }
    }

    public object[][] TestData = new object[][]
    {
       ...
    };
}

Here's a few things I like to pay attention to:

Is the test readable?

At a quick glance, the test should convey what is being tested. If it fails to convey what is being tested, at best, it becomes harder to update as the target code changes, at worst, it can sabotage development as it misleads the developer.

If you cannot discern what a test tests, how can you know if it is correct? A test which you cannot tell whether is correct or not because you cannot discern its purpose or meaning adds no value. You are better off without it.

Is the test well named?

A well-named test might convey the entire purpose of a test, much like how a well-named method conveys its functionality.

Formatting test names in a consistent way helps conveying purpose. Additionally, in conjunction with other tests, well-named tests can actually help you pinpoint exactly where you should look for bugs.

I personally use the {method}_{condition}_{result}. ```c#

public class CmsServiceUnitTests {
[Fact]
public async Task ReadSectionAsync_ValidKeyOfExistingEntry_ReturnsEntry() {
...
}
}

- Other naming conventions also exist - I can recommend the [{Given}\_{When}\_{Then}](https://martinfowler.com/bliki/GivenWhenThen.html) format since user stories can be formulated in this format as well.
```c#


public class CmsServiceUnitTests {
  [Fact]
  public async Task GivenValidKeyEntryPairInDb_WhenFetchingEntryOfTheKey_ThenEntryIsReturned() {
    ...
  }
}

Does it test more than one thing?

A test that tests more than one thing at a time can be difficult to derive meaning from and becomes less useful with respect to pinpointing where a bug lives in the code.

One thing does not mean one field or just one value, but rather the resulting state. A rule of thumb: primitive fields and such can be checked together as a way of validating the state of an object, but do make sure to add more informative messages when you have more than one assertion in a test.

With the {method}_{condition}_{result} naming convention, this is given: ```c#

[Fact]
public async Task ReadSectionAsync_ValidKeyOfExistingEntry_ReturnsEntry() {
var expected = dbKeyEntryPair.Entry;
...
var actual = await service.ReadSectionAsync(validKey);
...
Assert.Equal(expected, actual);
}


### Is it well arranged?
A well-arranged test does not configure some state, execute some code, configure some more, execute some more, assert something and keep on mixing these things. In the abstract, a test that does one thing, and one thing only, validates that the code under testing from some configured state performs some execution, returns something expected and finds rest in some expected state.
- Microsoft advice adding the three A's as code comments:
```c#


[Fact]
public async Task {TestName}(){
  // Arrange
  {Configuration}
  // Act
  var actual = {subject}.{Method}({args});
  // Assert
  {Assertion(s)}
}

There is some disagreement out there in the Great Ether with regards to where one should put shared static configuration, such as the initialization of the test subject(s) and mocks as well as common configuration of the subject(s) and mocks. It is my opinion that initialization can occur in the constructor of the test class and that this will be the same for all tests in a test class. Configuring the subject(s) and mocks thus belongs to the configuration step of each test, as they likely vary. ```c#

public class CmsServiceUnitTests {
public Mock writeRepoMock;
public Mock readRepoMock;
public CmsService subject;

public CmsServiceUnitTests() {
readRepoMock = new Mock();
writeRepoMock = new Mock();
subject = new CmsService(Mock.Of>(), readRepoMock.Object, writeRepoMock.Object)
}

const string validKey = {key of valid format};
const string entry = {content of entry};

[Fact]
public async Task ReadSectionAsync_ValidKeyOfEntry_ReturnsEntry() {
// Arrange
readRepoMock.Setup(m => m.ReadAsync(validKey)).ReturnsAsync(entry);
var expected = entry;
// Act
var actual = subject.ReadSectionAsync(validKey);
// Assert
Assert.Equal(expected, actual);
}
}


### Does logic occur?
A great way to detect whether a test only does one thing or not is to check whether it contains logic, such as a `switch`-, an `if`- or a `for`-statement. These statements make the test a lot less readable seeing as the state of the subject and the input at the time of method calling becomes something computed, rather than something set (which can simply be read).
- I must admit that I sin with respect to this when compressing testcases - sometimes I find it easier to write a lot of testcases using one parameterized test, where some of the arguments inform the testcase to configure in a particular way, instead of splitting up the testcases into multiple separate parameterized tests. However, just because something is easy doesn't mean you should do it!

### Is it a typical test?
Here _typical_ refers to whether or not the test follows some sacred dogma allowing it to be categorized easily into some preconceived type, such as End-to-end integration tests, unit tests, or simply tests that fit in to the overall test strategy.

If the test does not neatly fit into a category, consider refactoring it in such a way that it does. A way to do this could potentially be to clone the test if it resides within a canyon of two or more categories, in order to ensure that each category gets its own version of the test.

### Does it follow Clean Code Principles?
In spite of popular belief, test code is actually also code and should in general receive the same care you provide to any other code. There's great benefits to this, such as readability, reusability and flexibility.
- To demonstrate this, consider a test class containing a large number of tests where the subject has some internal state that may vary. While one state can be considered both an initial and valid, all other states can abstractly be thought of as mutations of this first state. We can use this sort of thinking when coding our tests. In each test following `// arrange`, we use the method `Setup();`. Each test then deviates from this initial state by mutating it, perhaps with a `SetSubjectDependency(s => s.{dep}, null)` in a test validating how the subject handles internal nullreferences.

### Is the test independent?
Tests which share state and space with other tests are at risk of experiencing bugs themselves because the timing of execution of the tests may affect their correctness. This should be avoided by instantiating dependent objects for each test.
- If you want to increase readability of a test by reducing bloat by moving some variables to the class scope or similar, or perhaps refactoring away some magic constants, make sure they are immutable in the outer scope. If this is not an option, consider not moving it to the outer scope. Otherwise, you are vulnerable to undesirable behavior in your tests.

### Is the test timely (relatively)?
A test that is never run is a bad test as it adds no value. A tests that motivates the developer not to run it, perhaps because it takes a lot of time to set up or it just runs slowly, **is a bad test**.
  - A test is a slow test, seen relatively to how often you'd like to run it. If a test suite is run once a month, because the code it touches is very static, or the suite is overly thorough relatively to everyday development, it is acceptable that it takes longer to run. Test suites that are expected to run on a daily basis, perhaps even hourly basis, should be quick, though! Otherwise, you will end up getting more coffee than is healthy for you, with all those coffee breaks.

### Is the test repeatable?
A test which has stochastic behavior, such as being real-time dependent, using pseudo-randomness generators or threading timing are inherently bad tests because a pass does not guarantee anything (a false positive), and a failure can be hard to reproduce.

Believe me! In my more inexperienced days, I once spent two days trying to reproduce a test failure which turned out to only occur when the clock on the machine had gone past 19:00.

These qualities makes the test unreliable. To fix this, you fix the uncertainties. Mock the problematic dependencies, in relation to which you might possibly need to introduce wrappers, in order to gain control over what they return. In that way, the test result becomes reliable, although the result is less general and more specific - but do not fool yourself! This was already the case, now we just know the specifics.

- This also implies that you should not use a PRG or the clock by yourself to setup or run your tests!
- Sometimes, it cannot be avoided to have uncertainty in your tests, for instance if you are integration testing in conjunction with some third party code. In that case, there are some strategies concerning how to avoid false positives, such as repeating the code x times, but you should be aware that there are not absolutes in the realm of probabilistic.

## What makes a test good?
Having uncovered what makes a test bad, the next logical step is to explore how to identify a good test. Rather than letting this blogpost go on forever, I'll let you ponder upon the matter until the release of the second half of this short series on testing, wherein I'll elaborate further on testing and how to spot good tests - so watch out for part 2! 

# Conclusion
To sum up this blogpost, however, we've now compiled a list of things to check for when you suspect a test of being bad and need to identity what needs to be changed.

In case you already forgot, here's a shorter version:
1. Know what you are testing.
2. Name the test accordingly.
3. Test one thing at a time.
4. Arrange your tests.
5. Make your test readable.
6. Make your test simple.
7. Make your test timely.
8. Make your test independent.
9. Make your test repeatable.

... and if the test is without purpose, get rid of it.

## Remark
If there is anything you wonder about, disagree with or would like added to this blog post - leave a comment!
## References
My personal experience,
https://docs.microsoft.com/en-us/dotnet/core/testing/unit-testing-best-practices
https://martinfowler.com/bliki/GivenWhenThen.html
https://gist.github.com/wojteklu/73c6914cc446146b8b533c0988cf8d29 (Clean Code by Robert C. Martin summary)
https://docs.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-6.0