Very answered question.
IHMO, the excellent answer from @BlueRaja - Danny Pflughoeft is one of the best.
Lots of answers suggest only testing the public interface, but IMHO
this is unrealistic - if a method does something that takes 5 steps,
you'll want to test those five steps separately, not all together.
This requires testing all five methods, which (other than for testing)
might otherwise be private.
Above all, I want to stress that the question "should we make a private method public to unit test it" is a question which an objectively correct answer depends on multiple parameters.
So I think that in some cases we have not to and in others we should.
Making public a private method or extracting the private method as a public method in another class (new or existing)?
It is rarely the best way.
A unit test has to test the behavior of one API method/function.
If you test a public method that invokes another public method belonging to the same component, you don't unit test the method. You test multiple public methods at the same time.
As a consequence, you may duplicate tests, test fixtures, test assertions, the test maintenance and more generally the application design.
As the tests value decreases, they often lose interest for developers that write or maintain them.
To avoid all this duplication, instead of making the private method public method, in many cases a better solution is extracting the private method as a public method in a new or an existing class.
It will not create a design defect.
It will make the code more meaningful and the class less bloat.
Besides, sometimes the private method is a routine/subset of the class while the behavior suits better in a specific structure.
At last, it also makes the code more testable and avoid tests duplication.
We can indeed prevent tests duplication by unit testing the public method in its own test class and in the test class of the client classes, we have just to mock the dependency.
Mocking private methods?
While it is possible by using reflection or with tools as PowerMock, I think that it is often a way to bypass a design issue.
A private member is not designed to be exposed to other classes.
A test class is a class as another. So we should apply the same rule for it.
Mocking public methods of the object under test?
You may want to change the modifier private to public to test the method.
Then to test the method that uses this refactored public method, you may be tempted to mock the refactored public method by using tools as Mockito (spy concept) but similarly to mocking private methods, we should avoid to mock the object under test.
The Mockito.spy() documentation says it itself :
Creates a spy of the real object. The spy calls real methods unless they are > > stubbed.
Real spies should be used carefully and occasionally, for example when
dealing with legacy code.
By experience, using spy() generally decreases the test quality and its readability.
Besides, it is much more error-prone as the object under test is both a mock and a real object.
It is often the best way to write an invalid acceptance test.
Here is a guideline I use to decide whether a private method should stay private or be refactored.
Case 1) Never make a private method public if this method is invoked once.
It is a private method for a single method. So you could never duplicate test logic as it is invoke once.
Case 2) You should wonder whether a private method should be refactored as a public method if the private method is invoked more than once.
How to decide ?
The private method doesn't produce duplication in the tests.
-> Keep the method private as it is.
The private method produces duplication in the tests. That is, you need to repeat some tests, to assert the same logic for each test that unit-tests public methods using the private method.
-> If the repeated processing may make part of the API provided to clients (no security issue, no internal processing, etc...), extract the private method as a public method in a new class.
-> Otherwise, if the repeated processing has not to make part of the API provided to clients (security issue, internal processing, etc...), don't widen the visibility of the private method to public.
You may leave it unchanged or move the method in a private package class that will never make part of the API and would be never accessible by clients.
Code examples
The examples rely on Java and the following libraries : JUnit, AssertJ (assertion matcher) and Mockito.
But I think that the overall approach is also valid for C#.
1) Example where the private method doesn't create duplication in the test code
Here is a Computation class that provides methods to perform some computations.
All public methods use the mapToInts() method.
public class Computation {
public int add(String a, String b) {
int[] ints = mapToInts(a, b);
return ints[0] + ints[1];
}
public int minus(String a, String b) {
int[] ints = mapToInts(a, b);
return ints[0] - ints[1];
}
public int multiply(String a, String b) {
int[] ints = mapToInts(a, b);
return ints[0] * ints[1];
}
private int[] mapToInts(String a, String b) {
return new int[] { Integer.parseInt(a), Integer.parseInt(b) };
}
}
Here is the test code :
public class ComputationTest {
private Computation computation = new Computation();
@Test
public void add() throws Exception {
Assert.assertEquals(7, computation.add("3", "4"));
}
@Test
public void minus() throws Exception {
Assert.assertEquals(2, computation.minus("5", "3"));
}
@Test
public void multiply() throws Exception {
Assert.assertEquals(100, computation.multiply("20", "5"));
}
}
We could see that the invocation of the private method mapToInts() doesn't duplicate the test logic.
It is an intermediary operation and it doesn't produce a specific result that we need to assert in the tests.
2) Example where the private method creates undesirable duplication in the test code
Here is a MessageService class that provides methods to create messages.
All public methods use the createHeader() method :
public class MessageService {
public Message createMessage(String message, Credentials credentials) {
Header header = createHeader(credentials, message, false);
return new Message(header, message);
}
public Message createEncryptedMessage(String message, Credentials credentials) {
Header header = createHeader(credentials, message, true);
// specific processing to encrypt
// ......
return new Message(header, message);
}
public Message createAnonymousMessage(String message) {
Header header = createHeader(Credentials.anonymous(), message, false);
return new Message(header, message);
}
private Header createHeader(Credentials credentials, String message, boolean isEncrypted) {
return new Header(credentials, message.length(), LocalDate.now(), isEncrypted);
}
}
Here is the test code :
import java.time.LocalDate;
import org.assertj.core.api.Assertions;
import org.junit.Test;
import junit.framework.Assert;
public class MessageServiceTest {
private MessageService messageService = new MessageService();
@Test
public void createMessage() throws Exception {
final String inputMessage = "simple message";
final Credentials inputCredentials = new Credentials("user", "pass");
Message actualMessage = messageService.createMessage(inputMessage, inputCredentials);
// assertion
Assert.assertEquals(inputMessage, actualMessage.getMessage());
Assertions.assertThat(actualMessage.getHeader())
.extracting(Header::getCredentials, Header::getLength, Header::getDate, Header::isEncryptedMessage)
.containsExactly(inputCredentials, 9, LocalDate.now(), false);
}
@Test
public void createEncryptedMessage() throws Exception {
final String inputMessage = "encryted message";
final Credentials inputCredentials = new Credentials("user", "pass");
Message actualMessage = messageService.createEncryptedMessage(inputMessage, inputCredentials);
// assertion
Assert.assertEquals("Aç4B36ddflm1Dkok49d1d9gaz", actualMessage.getMessage());
Assertions.assertThat(actualMessage.getHeader())
.extracting(Header::getCredentials, Header::getLength, Header::getDate, Header::isEncryptedMessage)
.containsExactly(inputCredentials, 9, LocalDate.now(), true);
}
@Test
public void createAnonymousMessage() throws Exception {
final String inputMessage = "anonymous message";
Message actualMessage = messageService.createAnonymousMessage(inputMessage);
// assertion
Assert.assertEquals(inputMessage, actualMessage.getMessage());
Assertions.assertThat(actualMessage.getHeader())
.extracting(Header::getCredentials, Header::getLength, Header::getDate, Header::isEncryptedMessage)
.containsExactly(Credentials.anonymous(), 9, LocalDate.now(), false);
}
}
We could see that the invocation of the private method createHeader() creates some duplication in the test logic.
createHeader() creates indeed a specific result that we need to assert in the tests.
We assert 3 times the header content while a single assertion should be required.
We could also note that the asserting duplication is close between the methods but not necessary the same as the private method has a specific logic :
Of course, we could have more differences according to the logic complexity of the private method.
Besides, at each time we add a new public method in MessageService that calls createHeader(), we will have to add this assertion.
Note also that if createHeader() modifies its behavior, all these tests may also need to be modified.
Definitively, it is not a very good design.
Refactoring step
Suppose we are in a case where createHeader() is acceptable to make part of the API.
We will start by refactoring the MessageService class by changing the access modifier of createHeader() to public :
public Header createHeader(Credentials credentials, String message, boolean isEncrypted) {
return new Header(credentials, message.length(), LocalDate.now(), isEncrypted);
}
We could now test unitary this method :
@Test
public void createHeader_with_encrypted_message() throws Exception {
...
boolean isEncrypted = true;
// action
Header actualHeader = messageService.createHeader(credentials, message, isEncrypted);
// assertion
Assertions.assertThat(actualHeader)
.extracting(Header::getCredentials, Header::getLength, Header::getDate, Header::isEncryptedMessage)
.containsExactly(Credentials.anonymous(), 9, LocalDate.now(), true);
}
@Test
public void createHeader_with_not_encrypted_message() throws Exception {
...
boolean isEncrypted = false;
// action
messageService.createHeader(credentials, message, isEncrypted);
// assertion
Assertions.assertThat(actualHeader)
.extracting(Header::getCredentials, Header::getLength, Header::getDate, Header::isEncryptedMessage)
.containsExactly(Credentials.anonymous(), 9, LocalDate.now(), false);
}
But what about the tests we write previously for public methods of the class that use createHeader() ?
Not many differences.
In fact, we are still annoyed as these public methods still need to be tested concerning the returned header value.
If we remove these assertions, we may not detect regressions about it.
We should be able to naturally isolate this processing but we cannot as the createHeader() method belongs to the tested component.
That's why I explained at the beginning of my answer that in most of cases, we should favor the extraction of the private method in another class to the change of the access modifier to public.
So we introduce HeaderService :
public class HeaderService {
public Header createHeader(Credentials credentials, String message, boolean isEncrypted) {
return new Header(credentials, message.length(), LocalDate.now(), isEncrypted);
}
}
And we migrate the createHeader() tests in HeaderServiceTest.
Now MessageService is defined with a HeaderService dependency:
public class MessageService {
private HeaderService headerService;
public MessageService(HeaderService headerService) {
this.headerService = headerService;
}
public Message createMessage(String message, Credentials credentials) {
Header header = headerService.createHeader(credentials, message, false);
return new Message(header, message);
}
public Message createEncryptedMessage(String message, Credentials credentials) {
Header header = headerService.createHeader(credentials, message, true);
// specific processing to encrypt
// ......
return new Message(header, message);
}
public Message createAnonymousMessage(String message) {
Header header = headerService.createHeader(Credentials.anonymous(), message, false);
return new Message(header, message);
}
}
And in MessageService tests, we don't need any longer to assert each header value as this is already tested.
We want to just ensure that Message.getHeader() returns what HeaderService.createHeader() has returned.
For example, here is the new version of createMessage() test :
@Test
public void createMessage() throws Exception {
final String inputMessage = "simple message";
final Credentials inputCredentials = new Credentials("user", "pass");
final Header fakeHeaderForMock = createFakeHeader();
Mockito.when(headerService.createHeader(inputCredentials, inputMessage, false))
.thenReturn(fakeHeaderForMock);
// action
Message actualMessage = messageService.createMessage(inputMessage, inputCredentials);
// assertion
Assert.assertEquals(inputMessage, actualMessage.getMessage());
Assert.assertSame(fakeHeaderForMock, actualMessage.getHeader());
}
Note the assertSame() use to compare the object references for the headers and not the contents.
Now, HeaderService.createHeader() may change its behavior and return different values, it doesn't matter from the MessageService tests point of view.
@VisibileForTestingannotation to make such methods - I recommend you do this so that the reason for the method not beingprivateis properly documented. – Boris the Spider