Unit Testing - should / how should I write tests to cover new code that doesn't affect the interface of a method?

Question

Having been trying to improve my unit tests, I'm trying to adhere to the principle of avoiding call verification. This is because it aligns with other principles I believe to be true:

• We Should test the behaviour, not the implementation details of code (i.e. don't try to prove the code is written like the code is written) (https://testing.googleblog.com/2013/08/testing-on-toilet-test-behavior-not.html).
• Call verification makes test code brittle. Code can be refactored in such a way that it is still correct, but break call verification assertions where internal method calls may have been reordered or removed.

However Robert C. Martin, a respected author on unit testing says:

You must write a failing test before you write any production code.

(https://blog.cleancoder.com/uncle-bob/2014/12/17/TheCyclesOfTDD.html)

These principles are not hard to follow when working through typical demo bank account scenarios seen in unit testing books, or problems that fit well with a functional style of programming, but consider the following requirement which seems to be a common scenario that appears to cause difficulty in adhering to the above principles:

Say we need to write a method that fetches and aggregates user data before making a call to an email service and we write the method as follows:

public Task SendUserEmail(long userId, string subject, string message)
{
    var user = _db.GetUser(userId);

    var emailMessage = new EmailMessage {
        Recipient = user.Email,
        Subject = subject,
        Message = message
    }

    _emailService.SendEmail(emailMessage);
}

If we're not testing implementation details via call verification, what is there to test here? There's no need to test that we have a method that accepts these three parameters, because "don't try to prove the code is written like the code is written", there's no need for a return value from this method, as an exception will be thrown if any of the internal calls fail.

However "You must write a failing test before you write any production code" according to Uncle Bob.

Further, if we're given a requirement to log the email send in a database using _db.LogEmailSend, we might change the method as follows:

public Task SendUserEmail(long userId, string subject, string message)
{
    var user = _db.GetUser(userId);

    var emailMessage = new EmailMessage {
        Recipient = user.Email,
        Subject = subject,
        Message = message
    }

    _emailService.SendEmail(emailMessage);

    _db.LogEmailSend(emailMessage);
}

Again, we'd prefer to avoid call verification, but as the interface is not affected by this additional feature we have no option if we're to follow "You must write a failing test before you write any production code".

Integration testing is likely to be the most reliable means of verifying that the new code really does what we expect, but we're not talking about integration testing here, we're talking about unit testing.

Is there something that could be improved in the structure of the code that that would make it easier to test while adhering to the afore mentioned principles? Should I return the internally composed EmailMessage composed before sending, even though that is not really the 'result' of the method and isn't required by calling code? Should we just not test this sort of code that simply glues components together and violate Robert C. Martin's assertion?

It seems that I'm unable to adhere to all these commonly quoted principles at the same time.

It's not unusual for development teams to gate pull requests with code coverage rules, so I'm pulled towards writing some sort of test, but it feels like that this forces me to use call verification, because it's the only thing I can write test code for here. But then I have brittle tests.

What can I improve here so that I have tests that cover the requirement without call verification?

Answer 1

Sometimes, detailed unit tests are not the right tool for the job.

Here is a copy of the method we are talking about:

public Task SendUserEmail(long userId, string subject, string message)
{
    var user = _db.GetUser(userId);

    var emailMessage = new EmailMessage {
        Recipient = user.Email,
        Subject = subject,
        Message = message
    }

    _emailService.SendEmail(emailMessage);

    _db.LogEmailSend(emailMessage);
}

This method has barely any behaviour of its own that could be tested. All it really does is to orchestrate various other systems to work together. All those other components can be assumed to have been tested independently, so they can be assumed to work.

Tests check for things that might have gone wrong. What could have gone wrong here is that we have combined these services incorrectly, or forgotten to trigger some external effect, or that we've failed to handle errors correctly.

It's also worth noting that all these other services – database, email provider – are external services. They are not part of your core domain.

To me, all of this together suggests that this function should likely be tested via an integration test instead.

That would still imply some degree of injection, though on the level of services rather than objects. For example, the integration test might set up your database service with a throwaway database in a Docker container that is destroyed afterwards, and might use a mock email provider that just appends the mail to an mbox file.

In Gherkin/BDD style, I'd expect a test for this to look like:

Scenario: sending and logging emails to users

Given a user 123 with email [email protected]
And that the time is 2022-12-30 21:02:54

When I send user 123 an email with subject “test subject” and body “click here to collect 1 million”

Then

• there is an email that looks like

  From: "This Company" <[email protected]>
  To: <[email protected]>
  Date: Thu, 30 Dec 2022 21:02:54 +0100
  Subject: test subject
  
  click here to collect 1 million
  
  (compliance footer)
  

• there is an email log entry on 2022-12-30 21:02:54 for sending a message to [email protected]

There are two important things going on here, one practical, one conceptual:

On a practical level, it's convenient to describe the expected result of a test with some representation that is as close to the real thing as possible, but stripping out irrelevant stuff. For example, the expected email here is described with MIME-style syntax, but ignores irrelevant headers. It shows that some footer will be appended to the message, without spelling it out (which would make the test overly fragile). This makes the meaning of the tests more accessible to humans.

On a more conceptual level, the important thing is we're not dealing with test spys or call assertion, we are describing what the system as a whole should be doing – not just that specific function. This also means that tests aren't excessively fragile, and are fairly cheap to write – once the necessary test infrastructure has been implemented.

To be clear, this approach isn't always appropriate. Preparing the test infrastructure so that your integration tests can work meaningfully can be difficult, especially if the system has unclear dependencies on external services. Sometimes, ensuring that your function calls specific other functions is a quicker way to give you confidence that your system is working as expected.

For example, consider a test that checks what happens if sending the email fails. Is that a synchronous or asynchronous operation? Should the email sending event be logged always, only if an asynchronous send was initiated, or only if it was completed successfully? Tests that rely on fault injection are often much easier to write via techniques such as injecting mocks, so that the faults are triggered precisely where we need them.

---

A note on following advice on Robert C Martin. That author often has good ideas, but expresses them in a misleadingly absolute manner. The claim that “you must write a failing test before you write any production code” is very strong and absolute, with words like “must” and “any”.

Such absolutely-phrased advice can be helpful for pointing you in the right direction, but taken literally it leads to lots of unnecessary busy-work. The value of tests isn't to make Uncle Bob happy, but to reduce your costs throughout the software development lifecycle. More immediately, the purpose of tests is to give you confidence that your code is working as you think it does. Spending the time to write a test for code that is “obviously correct” is wasted time.

I think his advice is best used as a compass, not as a map: it points you in a potentially sensible direction, but by following it you won't know when you've arrived at a sensible point to stop.

Phrased more reasonably, his advice could be interpreted as follows:

Tests have value. Writing tests is especially important for production code. A test-first approach such as TDD or some BDD practices can be useful.

Answer 2

Regarding call verification - test doubles/mocks that check if a method was called (at all, or with certain parameters, etc.) are called Spies. See this article on different kinds of test doubles written by Robert C. Martin.

He doesn't say not to use Spies, though, but advises us to minimize their use, as they potentially lead to more fragile tests.

Try to confine Spies to classes that are primarily "orchestrators". These will often (but not always) sit closer to the boundary between the outside world and your application.

These are mostly classes that have the narrow responsibility to "orchestrate" the inner-layer objects, delegating the details of the required work to them. For such "orchestrators", the relevant behavior is the high-level flow through the dependencies - you want to test that, without relying on any details of how that flow happens in any particular case.

In other cases, a class will provide an extension point, where you can plug in your own code that you expect to be called under certain conditions.

So, the key question is this: is your _emailService (etc.) an external dependency (injected through the constructor)? If it's entirely internal, then the test should not know about it. But of it isn't, then you have to ask yourself what is its contract with the dependent class?

It's likely that the contract of the two interacting classes involves that certain methods on the dependencies should be called under certain conditions. None of that (high level) flow is an internal detail. Again, internal details are how you make that high level requirement happen, and everything you may or may not do in between.

If you squint, this:

public Task SendUserEmail(long userId, string subject, string message)
{
    var user = _db.GetUser(userId);

    var emailMessage = new EmailMessage {
        Recipient = user.Email,
        Subject = subject,
        Message = message
    }

    _emailService.SendEmail(emailMessage);
}

is really

public Task SendUserEmail(...)
{
   // (1) Delegate to _db the job of getting the user.
   // (2) I KNOW BEST! I will create the email message myself!
   // (3) Delegate to the _emailService the job of sending the email.
}

One of these things is not like the others. So, one could argue that what you have here are mixed levels of abstraction. Suppose you need to send 5 different kinds of emails. If the design remains the same, there's some copy-pasting is bound to happen.

So, in the interest of DRY, you might consider doing something more akin to this:

// suppose the containing class is called MyMailer

public Task SendUserEmail(...)
{
   // (1) Delegate to _db the job of getting the user.
   var user = await _db.GetUser(userId);

   // (2) Delegate to _emailBulder the job of constructing the email.
   var email = _emailBuilder.Build(user);

   // (3) Delegate to the _emailService the job of sending the email.
   await _emailService.SendEmail(emailMessage);
}

You would then call it like so (I'm using 'new' here, but you might use a DI container):

var foo = new MyMailer(db, new NotificationEmailBuilder(), emailService);
foo.SendUserEmail(userId);

// or
var bar = new MyMailer(db, new PromoEmailBuilder(), emailService);
bar.SendUserEmail(userId);

// or
var baz = new MyMailer(db, new NewYearEmailBuilder(), emailService);
baz.SendUserEmail(userId);

// or
var fiz = new MyMailer(db, new CustomEmailBuilder(message), emailService);
fiz.SendUserEmail(userId);

// or...

// P.S. Look at all that code reuse happening! :D

Alternatively:

var foo = new MyMailer(db, emailService);
foo.SendUserEmail(userId, new NotificationEmailBuilder());

// or maybe a lambda-based variant:
var foo = new MyMailer(db, emailService);
foo.SendUserEmail(userId, (user) => GetNotificationEmail(user));

Now, suppose the contract is:
(1) pass userId to _db.GetUser,
(2) pass whatever was returned to the email builder,
(3) send whatever was returned via the email service.

^{(See the discussion below, though. What happens if there isn't a mathing userId in the database?)}

You want to test this high level flow, but not any of the details or specific values or email-building rules (in fact, with the email builder in place, your class doesn't even know anything about the email-building logic - so you shouldn't test that here). This other stuff belongs to the tests for the real implementations of the dependencies - a separate set of tests.

So, what do you do?

---

Setup a _db mock/spy that checks if it received the exact userId the test method provided, and does nothing with it - it should just return some canned user. It doesn't need to reflect any internal logic of the actual database. The canned user doesn't even have to have the provided userId in it. You don't care. You only care about the flow.

Setup an _emailBuilder mock/spy that checks if it got the canned user, does nothing with it, returns a canned email object.

Setup an _emailService mock that checks if it got the canned email.

Done.

---

Note that, except for the initial parameter, you aren't checking for any concrete values, or any other details. You are literally just checking the orchestration logic that this class is expected to provide for its callers. There's an expectation that the supplied dependencies will be called at certain points, in adherence with certain rules. It's like how part of the contract of the LINQ Select method is that it has to call your lambda at some point and pass it the next element, regardless of what it's doing behind the scenes. If LINQ's Select method wasn't guaranteed to do this, you wouldn't be using it for long.

In some scenarios, you might have flows that are a bit more complex - e.g. some paths might execute only under certain conditions. E.g. what happens if _db can't find the user with the given ID? Decide what should happen (silently fail and log, or maybe throw, or something else). Write a new test case that checks through the public interface or dependency contracts if your class adheres to the flow for this new scenario: setup a _db mock that throws (or returns null or whatever) - it doesn't even have to look at the supplied userId - then assert on the rest accordingly. You can write this test case before you write the code that implements it.

If tests like these pass, then the call should work in the same high level way with any real dependencies - assuming that their implementations don't throw or are otherwise misconfigured (which you can cover with a separate sets of tests).

As long as you don't break the high-level contract, you can, behind that constraint, add/remove/alter code, or change the implementations of the dependencies (or wrap them Decorator pattern–style, etc.) without ever having to change the test.

However, if you change the flow itself, the test will break, but that's good because so might your overall system, since it relies, somewhere, somehow, on the same things that the test relies on. That's your goal for this specific unit, that's what you want to capture - no more, no less. The test is a stand-in for the client code, but also, in some sense, for the system as a whole¹. It's an early warning system.

---

¹ E.g., suppose you had a class that produces a cryptographic hash of something, that you than use as a key to group things in a completely different project. Now suppose someone mistakenly modified the class so that it no longer always returns the same output when called on the same input (as expected from a hashing function), and it just so happens that nothing in that project breaks. It's the other project that breaks 7 days later. Now imagine you had a test that invoked this method twice on the same value, and failed immediately when the change was made. This test wouldn't be checking for the specific values returned, it would check if the two results are equal (the rule, the contract) - remember, it should pass for any correct implementation (you are free to change the hashing algorithm, and thus the value returned).

---

Tests that are narrowly focused and that check for the high-level behavior without relying on the details that could vary across different SUT and dependency implementations are generally less fragile and allow for easier refactoring and restructuring (the effects of the structure-affecting changes on the test suite should end up being more localized).

Now, the way I described this "flow" test above, it might seem fairly easy, even trivial. But writing these sorts of tests is hard - and in some cases might not be worth the effort (consider how critical the correctness of the particular piece of code is - see VoiceOfUnreason's answer).

The hard part is coming up with what these abstract, high-level behaviors actually should be, and figuring out what the rules are that are governing them - and how to express them in the tests. But on the bright side, this is how you end up with tests that are a "runnable specification", and this is also how tests inform your design. They force you to think about how you structured your code.

Answer 3

Tony Hoare, 1980

I conclude that there are two ways of constructing a software design: One way is to make it so simple that there are obviously no deficiencies....

    var user = _db.GetUser(userId);

    var emailMessage = new EmailMessage {
        Recipient = user.Email,
        Subject = subject,
        Message = message
    }

    _emailService.SendEmail(emailMessage);

That's... that's pretty close to "obviously no deficiencies".

If you felt compelled to have a test here, then you might instead consider a design where _db and _emailService are substitutable; and in your test you would use inert substitutes (possibly specialized for testing) rather than live production substitutes.

Another alternative (which your introduction of logging hints at) is to recognize that your need to see what's actually going on is the test driving you to consider observability/telemetry requirements. So you write your isolated automated checks using an in memory telemetry implementation, and some other technique to verify that the system really works when everything is wired together.

---

You must write a failing test before you write any production code

I think you'll find, if you do the experimenting, that this strict discipline isn't actually cost effective in the long run.

The code works for me, I don't work for the code -- GeePaw Hill

TDD, such that it "drives" the design at all, is really just encouraging you to factor your code such that

• Code that is complicated must be easy to test
• Code that is difficult to test must be so simple it can't possibly break

Test first works really well on complicated-but-easy-to-test. Test first sucks for simple-but-difficult-to-test.

Horses for courses; use techniques that are appropriate for the problem at hand.

---

If we're not testing implementation details via call verification

Sometimes, call verification is the right tool for the job.

For lots of side effects, there is a tension between what we want for testing (deterministic, reliable, timely) and what we get for reality. Thus, we end up introducing a proxy/substitute in place of the real component used in the production collaboration.

Because we're using a substitute, any verification is necessarily going to be checking some specification against a proxy measure.

And for a test subject whose responsibility ends at "send a message to this other collaborator", message verification is arguably the correct technique to use (what do we think could possibly be better than that?)

Of course, doing this well (meaning, doing it in such a way that you don't need to repeat the work each time a butterfly flaps its wings) requires being good/lucky in the skill of information hiding, which is to say reducing the blast radius of decisions that are likely to change.

Answer 4

The interface of your email sender is the parameters it receives, return value or exceptions thrown, side effects (like “emails sent today” counter increasing), and the emails being sent.

If none of that interface is supposed to change, then you don’t need new unit tests. Unless for example the email contents changed and you didn’t count that as an interface of the function. That needs tests, whether you count it as interface or not.

If your changes were performance related: Either you say “that’s not part of the interface” and do nothing. Or you say for example “capable of sending 250 emails per second” as a new part of the interface, then you write a unit test.

Answer 5

Surely its quite clear here that a successful test of this method requires that an email be sent to the users email address and logged.

When you construct your email sending class in the test you will be forced to inject the user, logging and email services. At that point what you chose to inject defines how "sending an email"/"logging" etc can be checked at the end of the test.

You can argue that this is "testing that a dependency is called" and hence bad, but I would argue that you can structure your mocks in such a way that you are checking..

• the email has been sent
• the log has been written
• the user email is as expected

etc

rather than

• emailService.Send().HasBeenCalled()

Sure essentially its the same thing, but if you change the internals of the class such that these dependencies are no longer used or work differently, your test will fail to compile, as it includes the class construction and dependency injection, and need re writing in any case.

Can your code be improved?

Well, yes you should consider returning the email to be sent, but you'll just have another class which orchestrates the sending anyway so the problem is still there as you point out.

Should you test logs are written? Yes, but here I think there is a strong argument that log writing is a 'cross cutting concern' injecting a log writer as per you example isn't the best way and as such testing a logger dependency isn't the best way to test.

Rather some sort of global exception handler could do the logging and testing would be done at an integration layer instead of unit tests.