4

This question already has an answer here:

I want to test a method which is not as much as a unit, because it is more of a 'orchestrator' / 'process' / 'controller' / 'coordination' class.

This is the case:

I have four unit tested classes:

  • One is a data service which can read/write data from the database
  • Second is a textservice which can create content for emails / messages etc.
  • Third is a mailservice that can send email
  • Fourth is a class dat can create tasks for users in our system (tasks are things they should do)

Now I created a new class, which sends an email all people which are late with paying an invoice. It reads data with the data service, creates the appropriate text with the textservice, sends an email with the email service, writes the new invoice status with the data service and creates a task when the emailing for a specific invoice fails.

Now this new class is my 'orchestrator' or 'process' or 'controller' or 'coordination' class.

I have these kind of classes a lot in our application because we try to make our classes (like the data/email/textservice) as small as possible so when 'work has to be done', like in this case the 'mail all people which are late with paying', we create a new 'orchestrator' or 'process' or 'controller' or 'coordination' class.

I think I have these kind of classes for the most of my actions in my webcontrollers because most input sent from the browser involves coordination between multiple (smaller) classes

Now how do I test these classes / methods?

I used to mock all 4 classes in my test and verify at the end that the classes are called, and in the right order.

But more and more I read that you shoud not do this, because then you test for the internal working of a method, and when you refactor that method, the test fails. So I should test for results, not for inner working. But this method is void, so there is nut much of a result to verify on. The only thing I can think for instance to check for is: is email sent? But the only way to check that is to verify the email service is called, but then I'm back at testing the internals.

I don't see these kind of examples in the unit testing / tdd books, because they most of the time only work with the small classes like the calculator class, but rarely do I see examples for 'orchestrator' classes like I'm describing, but which occur a lot in my code.

For those who think it's a duplicate: I think the answers here provide much more background than the one answer at the other question. That other question was answered with a integration test in mind, and my question is about unit testing, not integration testing. So I can't agree with the duplicate answer mark.

marked as duplicate by Doc Brown, gnat, GlenH7, user40980, Kilian Foth Jan 30 '15 at 11:44

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • For those who think it's a duplicate: I think the answers here provide much more background than the one answer at the other question. That other question was answered with a integration test in mind, and my question is about unit testing, not integration testing. So I can't agree with the duplicate answer mark. – Michel Jan 30 '15 at 13:09
15

From what you describe I would say that the way you have been doing it - mocking the collaborators - is the best approach. It may be that you are over-specifying with your mock - for instance, requiring an order which is not really required by the business needs; if that is the case you could lighten up on the order requirements of your tests. But basically for the coordinator you want to know that:

  • given a data service who says "these people are late"
  • given a text service which generates email of the form "hey, you're late"
  • given an email service which sends to a list of recipients
  • it will submit a request to the email service to send "hey, you're late" to the aforementioned set of people.

If it's doing that (and a few other things you mentioned which I'm not bothering to re-specify here), then as far as you're concerned, your coordinator class is working. That's an easy test to write with mocks, clean and communicative. That's the way I'd recommend.

  • 1
    If you have mocked the email service, you should be testing your coordinator's collaboration with that service. So it is enough to know that your coordinator has requested that the email service send the desired messages. The mock email service will not actually send an email; that's not something you want to test as part of the coordinator's unit test. – Carl Manaster Jan 29 '15 at 9:56
  • 5
    At present you have a requirement that the email service is called, and your test verifies this requirement is satisfied. When you change the requirement so that a communication service, rather than an email service, must be called, then you need to update the test to reflect that. – Carl Manaster Jan 29 '15 at 10:21
  • 1
    @Michel: If you change the requirements for some class, it is not expected that all existing tests remain green. Changing requirements means that you also need to inspect the existing tests for required changes. – Bart van Ingen Schenau Jan 29 '15 at 10:35
  • 1
    Alternatively, you could design the communicationservice interface now such that the existing emailservice implements it and the coordinator's injected dependency is a communicationservice, not an emailservice. Mock the communicationservice for test. You risk YAGNI, and you also risk that the second communications medium required, somewhere down the line, isn't a good fit for the interface you designed at the start. But if your existing requirements are of form, "communicate with users, for example by email" then it does express current requirements and might save changing tests later. – Steve Jessop Jan 29 '15 at 14:00
  • 1
    @Michel if your language supports it, your coordinator class should be relying on interfaces for each of the four components, not classes. The first time the mail service is replaced by a communication service, it should be possible to add the appropriate interface to that, so that nothing at all is broken. (It's also quite difficult to build mocks without using interfaces rather than concrete classes...). That might be an interim step, until a broader redesign is done. Or not. Whatever works. – itsbruce Jan 29 '15 at 15:56
5

It is absolutely not the responsibility of your coordinator to send mail, only to ask that it be sent. There are many reasons why a mail might not be sent, or delayed. It is the responsibility of your mail class to

  1. send valid mail
  2. reject invalid mail
  3. Log what it did (including unexpected failures to send valid mail)

All your coordinator has to do, with regard to mail, is

  1. Ask for mail to be sent
  2. React appropriately if the call to the mail object returns a failure.
  3. Log what it did

If your tests for your mail class are comprehensive, you should be confident that valid mail requests will be sent and problems logged. If, for some reason, your coordinator can create problem emails, this will become apparent very quickly in use and you will have all the data you need from the logs to fix the problem. Please be relaxed about that; there is no way to create tests which guarantee no failure ever. Content yourself with tests that test a components contract and that show the class always behaves responsibly both when a request honours the contract and when it breaks it.

It is important that components be testable in isolation. If testing one component requires that others be fully present, then they really are not separate components and you risk creating one big ball of mud. It is actually more dangerous than mocking, because you risk the creation of hidden dependencies on implementation details of those other components. It also means that any internal changes to those components force an update to the build and test process for this one. That scales terribly and wastes everybody's time.

Don't import any logic from the mail class into the mock. Please do test for both success and failure of the mail class, but make those arbitrary events, not something using logic copied from the real class into your mock. It is a waste of time and reintroduces the risk of hidden dependencies on implementation details.

Build your system cleanly based on these principles. Trust the system. Don't complicate it into a mess because you want it to prove in each part that the whole system works. That will fail and make failure more likely.

tl;dr

The law of Demeter applies to testing as well.

  • I think the question wasn't clear enough, because I did not mean to suggest that i was testing the internals of the email class – Michel Jan 30 '15 at 9:58
  • I know that is not what you mean to do, Michel. But the fact is that by including a genuine mail class in your coordinator test code, you still risk becoming dependent on its implementation in ways you had not expected. – itsbruce Jan 30 '15 at 10:01
  • That's why I talked about hidden dependencies. You don't mean to creat them but they happen. It's similar to the Fragile Base Class problem. – itsbruce Jan 30 '15 at 10:07
3

I don't think there is anything else that you can do except testing that the functions inside that method are called.

Since the "orchestration" method doesn't contain much logic unit-testing is not so important. It's a method that only wires up other methods. It integrates other components. Therefore there should be integration tests that cover those lines of code. Unit tests may be omitted. So I wouldn't worry about it too much as long as you have those integration tests. Though, how this can be integration tested is another topic because you should not mock all of the other components and therefore get some dependencies that you are going to have to deal with.

2

You generally don't need to test controller classes, since they shouldn't hold complex logic. The actual work should be done in other classes if it is written correctly, so in the end you're essentially ensuring that the work gets delegated correctly.

The complexity of this depends entirely on the language that you're using, since to do it correctly, you would need to create stubs. Languages like Java might have a hard time with this, but there are libraries that help you with this like Mockito.

If you do need to test your controller class, then you should ideally remove the surrounding classes and put stub classes in their places, then you simply verify that the proper method gets called in response to each input to your controller class. I remind you that the whole point of unit testing is to verify that each method is doing its job properly. For a controller class, that generally means delegating tasks properly.

The alternative to using stubs would be to perform the actual work, though this can easily get overwhelming to test given the complexity and other complications such as writing to the database, so I would advise against it.

Also, I would strongly advise you to extract only the information required in web controller classes and pass it to the methods that do the logic. The reason for this is that you don't have to mock http request and response objects, but only what you pass, and you shouldn't have to test a method that dumbly retrieves the data required and passes it to another method (if it passes a null value, fine, don't test any more than you have to).

I hope that helps!

  • Good answer. Agree with this line ' I remind you that the whole point of unit testing is to verify that each method is doing its job properly.' But is the goal of this method to (amongst others) send an email, or to call EmailService.SendEmail(); ? The first one I can't test, the second will break when I refactor the EmailService to a generic 'communicationservice'. My unit test then fails because the emailservice doesn't get called, but the end result which I can not test (the email gets sent) is still achieved. – Michel Jan 29 '15 at 9:31
  • 1
    To resolve the question of whether the goal is to send an email or call the right method of its dependency, consider whether you would be happy if the co-ordinator class implemented RFC 5321 for itself from scratch. If that would be fine, then what you have isn't "a unit test of the co-ordinator", it's "an integration test of a mail client". In some contexts you want such a test, but since you're calling this a unit test of the co-ordinator I suspect that in fact what you really want is just that: a test that it uses its specified dependencies correctly. – Steve Jessop Jan 29 '15 at 14:06
  • @SteveJessop You can do an integration test, though to be thorough, you'd need to perform a test of the individual component itself so that if the component tests succeed and the integrated controller tests fail, then you know it is a problem with the controller. Though, I tend to stray away from created tests more complicated than I have to. – Neil Jan 29 '15 at 14:11
  • @Neil: agreed. And another way to look at this is that the design for the whole system might say, "all email shall be sent through the EmailService component, because we anticipate various email gotchas and only want to fix them once". Then you can go ahead and unit-test the co-ordinator knowing that it really is a hard requirement that it calls SendEmail() correctly, not that an email is actually sent. So, making requirements like that helps keep the tests simple. Or in short: thinking in terms of dependency injection changes how you design your system :-) – Steve Jessop Jan 29 '15 at 14:16

Not the answer you're looking for? Browse other questions tagged or ask your own question.