If we don't verify that private methods are called with unit tests, how do we verify that they are called?

Question

I hate to be bringing this up again, but I'm really trying to understand how to safeguard something with my tests.

I have a public method (below) that calls a private method before calling another method that actually takes some action. I want to make sure that the call to the private method doesn't get removed because that could be catastrophic. I've done some research, here, here, and here, and they all say not to test private methods. I can understand that, I guess, but then how do I safeguard against the removal of this line of code?

As you can see, the public method returns void, so I can't test the results of the public method call. And I have unit tests that test ApplicationShouldBeInstalled() directly.

public void InstallApplications()
{
    foreach (App app in this._apps)
    {
        // This is the line of code that can't be removed. How can I make
        // sure it doesn't get removed?
        if (!ApplicationShouldBeInstalled(app)) { continue; }

        // This simply can't run unless it passes the above call.
        CommonUtility.Container.Resolve<IAppInstaller>().InstallApplication(this, app);
    }                       
}

EDIT - I went with this, based on JerKimball's answer.

Basically, I just use a Mock object (from Moq), and then verify that its method was called the expected number of times.

[TestMethod()]
public void ApplicationShouldBeInstalledTest_UseCase13()
{
    var mockAppInstaller = new Mock<IAppInstaller>();
    mockAppInstaller.Setup(m => m.InstallApplication(It.IsAny<ApplicationServer>(),
        It.IsAny<Application>()));
    CommonUtility.Container.RegisterInstance<IAppInstaller>(mockAppInstaller.Object);

    // Actual test code here

    appServer.InstallApplications();
    mockAppInstaller.Verify(x => x.InstallApplication(It.IsAny<ApplicationServer>(),
        It.IsAny<Application>()), Times.Never());
}

I can't let this go; that edit is just ugly. Even though I have to create an actual mock class, this approach is much cleaner:

Mock implementation:

public class MockAppInstaller : IAppInstaller
{
    public bool Invoked { get; set; }

    public void InstallApplication(ApplicationServer server, Application app)
    {
        this.Invoked = true;
    }
}

Test method:

[TestMethod()]
public void ApplicationShouldBeInstalledTest_UseCase14()
{
    MockAppInstaller mockAppInstaller = new MockAppInstaller();
    CommonUtility.Container.RegisterInstance<IAppInstaller>(mockAppInstaller);

    // Actual test code here

    appServer.InstallApplications();
    Assert.AreEqual(true, mockAppInstaller.Invoked);
}

Measuring code coverage will tell you whether it was called or not. — millimoose, Feb 14 '13 at 21:40
I suggest you tag this language-agnostic, I think it is very relevant for all platforms — Miserable Variable, Feb 14 '13 at 21:40
This also might be a case for not making that method private, but putting it into a mockable object. — millimoose, Feb 14 '13 at 21:41
If you have a good set of unit tests that fully test the public interface (fully means different things to different people), then if that line got removed, the system would be in a incorrect state, causing other tests to fail. Now I use a continuous integration server, so we test on every commit. That works for us as we can simulate most of our external interfaces well enough. By doing this, even though the "InstallApplications" test does not fail, another one will, and you would be able to see what commit caused it. Showing the line was removed. " — Rob Goodwin, Feb 14 '13 at 21:45
If I understood correctly, eventually you had to change your private method to be public in order to mock it, right? — BornToCode, Sep 07 '15 at 16:02

score 5 · Answer 1 · answered Feb 14 '13 at 21:43

5

I want to make sure that the call to the private method doesn't get removed because that could be catastrophic.

That sounds like it should be easy. Catastrophes are pretty easy to spot. So run a test which calls the public method, and check whether anything catastrophic happened. The code within the private method which guarded against the catastrophe is effectively a visible side-effect of running your public method... whereas the fact that it was due to a private method call is an implementation detail.

So in this case, you should basically create an application which shouldn't be installed (for whatever reason), and validate that when you call InstallApplications it isn't installed.

answered Feb 14 '13 at 21:43

Jon Skeet

1,421,763
867
9,128
9,194

That may be easier said than done. When that public method eventually calls `InstallApplication()`, the mock object is used (during testing) and simply does nothing. The catastrophe isn't an error, it's the fact that an app would be installed to one of our deployment environments, including production. How do I check for that? – Bob Horn Feb 14 '13 at 21:51
1

If you say that a mock object is called, can you not just verify that the mock was called? – Bobbles Feb 14 '13 at 21:54
How? By having the mock object set some variable somewhere, then having the test code check for that? – Bob Horn Feb 14 '13 at 21:56
@Bobbles I updated my question to show a possible implementation of your suggestion. Is this what you were thinking? – Bob Horn Feb 14 '13 at 22:25
@BobHorn: Most mocking frameworks allow you to explicitly verify that a particular number of calls have been made. – Jon Skeet Feb 14 '13 at 22:59
@JonSkeet Good point. I just updated my question to show the solution using Moq. And I no longer need an actual class for the mock object. – Bob Horn Feb 14 '13 at 23:33

score 3 · Accepted Answer · answered Feb 14 '13 at 21:51

3

Here's one possible option...granted, this is very simplistic, but it might be applicable to your situation with a bit of alteration;

Let's say that App looks like:

public class App 
{
    public virtual bool CanBeInstalled {get; set;}
}

And ApplicationShouldBeInstalled looks like:

private bool ApplicationShouldBeInstalled(App app) { return app.CanBeInstalled; }

You could write a unit test that "confirms via expected actions", like so:

void Main()
{
    var sut = new ThingToTest();    

    var mockApp = new Mock<App>();
    var wasCanBeInstalledChecked = false;
    mockApp
       .SetupGet(app => app.CanBeInstalled)
       .Callback(() => wasCanBeInstalledChecked = true);

    // of course, whatever you'd do here to get an app into this class
    sut.AddApp(mockApp.Object);

    sut.InstallApplications();
    Debug.Assert(wasCanBeInstalledChecked == true);
}

answered Feb 14 '13 at 21:51

JerKimball

16,584
3
43
55

I actually like this idea, but I think there could be an even better implementation. Using your example, I'm changing production code to signify what's happening during tests. Maybe another approach would be for the test/mock object to signify that it was called. +1 for the tip. Thanks. – Bob Horn Feb 14 '13 at 21:58
@BobHorn Yeah, I had to make a number of assumptions...hopefully there's some other "this better happen" event you can similarly key off of. – JerKimball Feb 14 '13 at 21:59
Even though I didn't use your exact implementation, it is the same basic idea for the simple solution I ended up using (posted in my question). Thanks! – Bob Horn Feb 14 '13 at 22:46
My pleasure! Although I do admit, I rather like the refactored alternatives posed by Tony and Oliver... – JerKimball Feb 14 '13 at 22:52
I like them as well, but the solution I used is foolproof. If, for any reason, that call to the private method is removed, the test will now catch it. The consequences are just too much to leave it to chance. – Bob Horn Feb 14 '13 at 22:55

Olivier Jacot-Descombes · Answer 3 · 2013-02-15T17:38:57.343

Even making this method public won't enable you to see whether it was called or not. By making it public it would allow you to test it, but nothing more.

Your code would be much clearer, if you would use the if statement in a regular way. Its purpose would even be clear to those programmers who are now removing this line

public void InstallApplications()
{
    foreach (App app in this._apps)
    {
        if (ApplicationShouldBeInstalled(app)) {
            CommonUtility.Container
                .Resolve<IAppInstaller>()
                .InstallApplication(this, app);
        }
    }                       
}

You could also have this method increase a counter when called. Unit tests can then test this counter.

Note: You can make this counter internal and make it visible to the unit test project with the InternalsVisibleToAttribute.

UPDATE

Here's how this counter could be implemented:

public int ApplicationShouldBeInstalled_Count { get; private set; }

public bool ApplicationShouldBeInstalled(App app)
{
    ApplicationShouldBeInstalled_Count++;
    ...
}

Then test

var sut = new MyInstallerClass();
int oldCount = sut.ApplicationShouldBeInstalled_Count;
sut.InstallApplications();
int newCount = sut.ApplicationShouldBeInstalled_Count;

Assert.AreEqual(oldCount + sut.Apps.Count, newCount);

I would contend that using the `if` statement, as a guard clause like I have done, is a good, regular use of the `if` statement. If I change it to what you proposed, that still doesn't prevent the call to the private method from being removed. — Bob Horn, Feb 14 '13 at 21:55
Yes, but in your code the if-line and the installation-line are just two independent lines of code, where as my proposal embeds the installation-line into the if-statement, making their dependency explicit (and visible to every idiot). — Olivier Jacot-Descombes, Feb 14 '13 at 21:59
Understood, and thank you, but it just doesn't solve the problem of making it foolproof. Idiots exist, and even good programmers make mistakes sometimes. — Bob Horn, Feb 14 '13 at 22:04

score 1 · Answer 4 · answered Feb 14 '13 at 22:05

1

public void InstallApplications()
{
    foreach (App app in this._apps.ThatShouldBeInstalled)
    {
        CommonUtility.Container.Resolve<IAppInstaller>().InstallApplication(this, app);
    }                       
}

Just thought I'd throw a bit of lateral thinking in the mix...

Stop it being inadvertantly being taken out, by advertantly taking it out. :)

answered Feb 14 '13 at 22:05

Tony Hopkinson

20,172
3
31
39

Ooooh... that's interesting. And this is why I asked the question; to get some different thoughts. So, basically put that code elsewhere and reconstruct the code so taking out one line wouldn't damage things. Thank you. – Bob Horn Feb 14 '13 at 22:07
As you said some fool could take it out.Having been said person on more than 5,678 occasions.I've learnt a bit of self defence. People have been trying to get me out of my scope rut recently (I'm one of those hide everything by default types). Delegating private tasks to public classes is a habit I'm trying to gain. – Tony Hopkinson Feb 14 '13 at 23:10

If we don't verify that private methods are called with unit tests, how do we verify that they are called?

4 Answers4