Unit testing private methods

Question

preface: I know this topic has been asked about a lot on here in the past. Hopefully I will make it clear that I've read a fair amount of the questions/answers on the topic, and other literature, too. Alas, my questions persist. Some parts of my post written like a hypothetical dialogue with someone who supports the principles, followed by my counter-arguments. If I've done a bad job of "representing the other side", please let me know.

In her book Practical Object-Oriented Design, An Agile Primer Using Ruby, Sandi Metz has three approaches to unit testing private methods, each with a corresponding sub-heading (all of them are on page 216):

1. 9.3.1. Ignoring Private Methods during Tests
2. 9.3.2. Removing Private Methods from the Class under Test
3. 9.3.3. Choosing to Test a Private Method

Ignoring Private Methods during Tests

In this section, she talks about how "Testing private methods is never necessary," because their behaviour should have been comprehensively tested indirectly by the tests of the public methods of the object under test. This is technically true, but I could use the same argument to support the idea that you shouldn't write unit tests at all.

Well-written integration tests could cover all cover paths of all units, thus testing units is never strictly necessary. Yet unit tests still have value: They make it easier to find the root-cause of a failure (as compared to integration tests, which typically require more hunting to find errors), and offer a more isolated environment in which to debug them (which is simpler due to having less moving parts, and potentially confounding issues).

Unit tests also have a cost: they bind more strongly to more of the public interfaces of objects in your prod code than integration tests do. While "public interface" is usually assumed to be synonymous for "rarely-changing," that's just not true. There's a different between:

• the public interface of a library that's depended on by many third parties (which should rarely make breaking changes), and
• the public interface of a standalone application, with no external dependants.

Such a standalone application only has its public interfaces used by other objects from the same application. These interfaces are constantly being refactored and improved, so they're expected to change, and the unit tests have to be edited to keep up (increasing costs, the same way over-coupling to implementation details does).

Removing Private Methods from the Class under Test

In this section, she talks about how a class with too many private methods is indicative that perhaps it's doing too much. She suggests that if the interface of these private methods is somewhat fleshed out and stable, they should be extracted into public methods of new objects.

I find that following this advice leads me far into the scary IAbstractMetaFactoryStrategyProvider land. It's a scary land, where all the verbs are nouned in senseless ways. Naming things is hard, the names tend to (almost necessarily) become more long/descriptive (because there's so many classes in such a system, and they all need to be easily differentiated from each other), and evermore abstract (i.e. less related to the business entities).

Consider this real world example I ran into:

MacOS has a system called "UserDefaults", which is a mechanism for storing user preferences in a domain, which is just a namespace (e.g. there's one domain per app). It's essentially just a simple key/value store, which is backed by .plist files (an XML-based storage format). I wanted to write DefaultsPlistLocator, an object that can find the location of the backing .plist file on disk for a given domain . E.g. if the domain is com.example.domain, the output is a URL modelling the path /Users/Bob/Library/Preferences/com.example.domain.plist. Here's a look:

class DefaultsPlistLocator {
    func locatePlistFile(forDomain domain: String) -> URL {
        let homeDirectory = FileManager.default.homeDirectoryForCurrentUser
        let preferencesDirectory = homeDirectory.appendingPathComponent("Library/Preferences")
        
        return preferencesFolder.appendingPathComponent(domain + ".plist")
    }
}

Now here's a catch, there's a special domain called NSGlobalDomain, which is shared among all apps. Its backing .plist file breaks the pattern, and has a special location: .../.GlobalPreferences.plist. To cope with this fact, I introduced the following change:

class DefaultsPlistLocator {
    func locatePlistFile(forDomain domain: String) -> URL {
        let homeDirectory = FileManager.default.homeDirectoryForCurrentUser
        let preferencesDirectory = homeDirectory.appendingPathComponent("Library/Preferences")
        
        return preferencesFolder.appendingPathComponent(plistName(forDomain: domain))
    }
    
    private func plistName(forDomain domain: String) -> String {
        if domain == "NSGlobalDomain" {
            return ".GlobalPreferences.plist"
        } else {
            return domain + ".plist"
        }
    }
}

It's not a particularly large helper function, but it leads to a style of code I've come to quite enjoy. As Grady Booch put it:

Clean code is simple and direct. Clean code reads like well-written prose. Clean code never obscures the designer’s intent but rather is full of crisp abstractions and straightforward lines of control.

The plistName(forDomain:) function is pretty niche, so I made it private. It's not terribly complex, but imagine it was complex enough to warrant being tested on its own (rather indirectly through the public API locatePlistFile(forDomain:), but not reusable enough to warrant being extracted out. I would just go ahead and unit test this private method, and call it a day.

The suggestion here is to extract this into a public method of a new class, and inject and object of that class as a dependancy of DefaultsPlistLocator. From what I can tell, this is recommended in the pursuit of the single responsibility principle, and some abstract notion of reusability. I refute that: both of these functions serve to one responsibility, locating plist files, and it's really rather unlikely I'll need to reuse plistName(forDomain:). One might respond:

But you can't know that, you'll be surprised how often you'll run into situations where you want to reuse things you didn't consider would be reused at the time.

And I mean, fair enough, but I'm not scared of refactoring. If this function/class was well written, i.e. it doesn't have many dependencies (it doesn't "pull in the world"), then extracting this method is absolutely trivial. I feel no need to take pro-active measures against this refactoring in the future, because really, it's trivial. On the flip side, extracting it entails:

1. Creating a new class to contain this new public method. What would I call that? DefaultsPlistNameDeriver? That name:
   1. Sucks.
   2. Wouldn't tell much about what that actually means
   3. Is duplicating what the method name already suggests
   4. Is "verbing a noun" in a forced but otherwise unnecessary way
2. Extracting the interface of DefaultsPlistNameDeriver into, say, IDefaultsPlistNameDeriver
3. Writing a DefaultsPlistNameDeriverTestDouble which conforms to IDefaultsPlistNameDeriver for test purposes
4. Adding a new defaultsPlistNameDeriver: IDefaultsPlistNameDeriver field in the DefaultsPlistLocator class
5. Adding a new parameter to the DefaultsPlistLocator constructor to inject the dependency
6. Adding a new assignment of the constructor param from #5 to the object field from #4.
7. Managing the lifetime of a new object, figuring out who should inject the dependency, etc.
8. Adding a new test file and class for DefaultsPlistNameDeriver

That's a LOT of boilerplate/overhead, that increases complexity/noise of the system, with no perceivable value if it's done "too early". It's 10s of extra lines, for a helper function that's only 5 lines long (and could conceivably be pretty well-written as a perfectly understandable, single-line conditional expression domain == "NSGlobalDomain" ? ".GlobalPreferences.plist" : domain + ".plist") If you do this everywhere, you end up with FizzBuzzEnterpriseEdition, where you're injecting a NumberIsMultipleOfAnotherNumberVerifier.

Side note: I think this is the Strategy Pattern, is that correct?

I would find myself extracting this code for the wrong reason: not because I have any intent to reuse this helper function, but because I'm compelled to follow the "don't unit test private methods" dogma, for its own sake.

Worst of all, I fear that if I held myself to a strict standard of extracting methods like this will be a perverse incentive. I'll just say "meh, fuck it", and not extract the function, in-lining a bunch of code that would otherwise be better off extracting into little helper functions.

Choosing to Test a Private Method

In this section, she discussing actually unit testing private methods. She cautions against making your tests too fragile by coupling them to implementation details of your application. Doing so makes refactoring harder (more tests to go back and adjust), and increases cost. I totally agree, there's certainly a point at which over-coupling costs more than it helps.

But extracting this private method would make no difference to its stability. Public or private, if I need to change it, I will (and adjust the tests accordingly). This naturally changes if it was a library with external dependancies, but it's not.

My actual questions

1. Have I misunderstood about these principles, their motivations, and costs vs benefits?
2. How should I handle this specific plistName(forDomain:) example?
3. Does the guidance here change depending on whether or not this is a hobby personal project, or a professional team project? I.e. would being "strict" offer benefits to a single dev working on a single project, or are they more to do with large software systems that evolve more.

Answer 1

This is a long question with many arguments, and I've done my best to provide answers to all arguments. But I want to take a moment to summarize here.

You've reasoned about your stance deeply, and many of your observations are reasonably correct. However, the conclusions you then draw from those observations seemingly come out of left field and do not logically follow from the observations you made.

Most feedback points I wrote generally boil down to that same feedback.

---

Ignoring Private Methods during Tests

Well-written integration tests could cover all cover paths of all units, thus testing units is never strictly necessary. Yet unit tests still have value: They make it easier to find the root-cause of a failure (as compared to integration tests, which typically require more hunting to find errors), and offer a more isolated environment in which to debug them (which is simpler due to having less moving parts, and potentially confounding issues).

I'm not quite following here. You say unit testing isn't necessary, but then give an excellent benefit it provides when you do it. If getting the benefit isn't enough of a reason to consider doing it, then how are you defining "necessary"?
Because by that logic, nothing is necessary when you don't want it. You didn't need to become a software developer. You didn't need to post this question. I didn't need to read your question. I didn't need to get out of bed today. Etc...

Using that same logic, testing isn't necessary. Just run the application and see that it works, right? I'll spare you the lecture on why that's not a good idea. You touched on the benefits of testing yourself so there's no point repeating it. I asssume we agree that testing is not strictly necessary to deliver software but really really really advisable if you want to do your job well.

The main point that is often glossed over is that a test's purpose is to fail, not to pass. Don't get me wrong, a failed test implies a bug in your code, and bugs are not good to have. But when you already have a bug, having a failed test to raise that red flag for you is a good thing.

Think of it this way: when comparing two doctors who investigate the same 1000 patients, the better doctor will catch and diagnose more diseases than the other doctor. It's not good for patients to have more diseases but the doctor's diagnosis did not cause the disease, and while the disease may be bad, the diagnosis of that disease is a good thing (it provokes action to treat the disease).

Translating back: It's not good for codebases to have bugs, but the unit test's failure did not cause the bug, and while the bug may be bad, the unit test failure because of that bug is a good thing (it provokes action to fix the bug).

Well-written integration tests could cover all cover paths of all units

This is the mistake you're making here. You're only thinking of tests that pass. Yes, if a full-blown integration test passes, that implies that all of the integrated components themselves work (assuming you wrote the assertion logic for everything you care about)

But when it fails, which I want to remind you is the real purpose of a unit test, then your single combined integration test is much vaguer than it needs to be. It just tells you that something failed. That's about as useful as an exception without a stacktrace, exception type and message, i.e. not useful at all. What failed? Why did it fail? Is that the only thing that failed?

Not to mention that you're also blind to the "two wrongs coincidentally happen to make a right" type of bugs. E.g. if I write an integration test that reverses a string twice and compares the initial and resulting values, then that test would also pass if I never reversed the string at all.
This is of course a dramatically oversimplified example, but similar issues can exist where one component just happens to counteract the buggy behavior of another component.

You already addressed the benefits of using failed tests to debug so I won't repeat that part of it. Just be aware that you should think of the failures as much as the successes, if not more.

---

As an aside...

Whenever I point out potential bugs or how certain tests don't catch everything, I often get a response along the lines of "well they just needed to properly test for that and they would've caught the issue".

The answer here is simple: if you make assumptions based on a developer who writes perfect tests, then testing is irrelevant as such a perfect developer would never create a bug in the first place.

There's no way for any developer to know that a test suite is 100% complete. Developers simply write tests until they feel confident about the test results. If any bug pops up, the test suite gets expanded to cover for that bug in the future.

This means that the more complex a test becomes, the more likely you are to forget to test one of its many possible outcomes. This is why integration tests should not be your only line of defense: too complex to reasonably guarantee that every interaction or chain of events is tested.

---

Unit tests also have a cost: they bind more strongly to more of the public interfaces of objects in your prod code than integration tests do.

Since unit tests are tests of a unit's public contract, your observation is tautological. Of course your tests need to change when your tested object changes. This comes in two flavors:

• Changes to the logic will require you to add/update/remove unit tests to account for that new/updated/removed behavior.
• Changes to the contract without logic changes (= refactoring) will require the unit test to be refactored without changing the purpose of the tests themselves.

That's just unavoidable. And I'm not quite sure how ro why you'r asserting that this is a relevant excuse to skip writing unit tests. That's like saying you don't need to brush your teeth because combing your hair takes less time/effort.
The observation ("I comb my hair faster than I brush my teeth") might be correct but the conclusion ("Brushing my teeth can be skipped since it's less efficient") you're drawing from it is wrong.

While "public interface" is usually assumed to be synonymous for "rarely-changing," that's just not true.

Same reply here: your observation isn't wrong but the conclusion you're drawing from it ("we should skip unit tests") is wrong.

---

Removing Private Methods from the Class under Test

how a class with too many private methods is indicative that perhaps it's doing too much

That's a straightforward application of SRP and it is correct.

Note, though, that this is just a suggestion, i.e. one (or many) possible indications of there being an issue in your class. There is no set amount of private methods which guarantee that SRP is being violated and that would force you to abstract.

There is a strong tendency for SRP-violating classes to have a lot of varied distinct logic, which could be written in a monolithic function or could be subdivided into private logic. In either case, SRP is violated. Don't read too much into the "private method" part of the suggestion itself.

I find that following this advice leads me far into the scary IAbstractMetaFactoryStrategyProvider land. It's a scary land, where all the verbs are nouned in senseless ways. Naming things is hard, the names tend to (almost necessarily) become more long/descriptive (because there's so many classes in such a system, and they all need to be easily differentiated from each other), and evermore abstract (i.e. less related to the business entities).

How were you able to name these supposed private methods, and then struggle naming the classes which replace the private methods? Both of them needed a name that describes their function, so I don't get why you supposedly struggle with the latter and not the former.

Also, nomenclature requires some skill/experience on the one hand (because it helps you separate the correct distinct responsibilities), but also requires you to understand the problem you're trying to solve.

If you're struggling to come up with a name for some piece of logic that you clearly just decided needs to live on its own in its own class, then I think you're either abstracting the wrong thing or you haven't understood your abstraction well enough to understand its purpose (and thus the name it should receive based on that purpose).

With your MacOS example, your conclusions go off the rails. I want to address each point separately:

The plistName(forDomain:) function is pretty niche, so I made it private.

...but not reusable enough to warrant being extracted out

That is not a reason for making it private. Read up on SRP and notice how a class' responsibility is not defined by how often it is used.

It it's a separate reponsibility, even if only containing a few lines of code, that can still justify making a separate class. For example, you could have different classes for generating the plist file path, accessing the plist file on a given path, and parsing the data from that file on the given path.

Regardles of whether these three responsibilities require many or few lines, the three responsibilities are distinct and will generally warrant abstraction.

It's not terribly complex, but imagine it was complex enough to warrant being tested on its own

Following on from the previous point, the size of a class does not define whether it should be tested or not. Anything that's either custom business logic or in any way non-trivial or prone to breaking should be tested.

You clearly have specific business rules for this plist path generation (NSGlobalDomain vs default), and this already proves the necessity of some tests to see if the right path (NSGlobalDomain vs default) is being generated under the right conditions (the input string).

I would just go ahead and unit test this private method, and call it a day.

Unit testing private logic is not a thing. Unit tests focus on the public contract. Private logic is an implementation detail, and tests explicitly avoid implementation details (except the mocking of dependencies).

Your test is an outside consumer. It only has access to the public methods/properties of your class, not the private ones. The consumers of this class (the test, production code, ...) do not care, nor do they need to know if your class has private logic or not.

If that private logic is triggered when a certain public method is called, then the unit test for that public method will pick up on any bugs generated by the private logic.

If there is no public method/property which triggers this internal logic, then this logic isn't being used, and therefore can be removed.

In the end, all of your class' content should be part of its public behavior. If something isn't part of the public behavior, then it can safely be deleted since it's clearly not being used.

And I mean, fair enough, but I'm not scared of refactoring.

First of all, "I"m not scared of X" is not the same as "I should do X". I'm not scared of breaking glass but that doesn't mean I should break my windows.

Secondly, if you're not scared of refactoring, why were you arguing that unit tests should be skipped because of the bother of having to refactor them when your public contract changes? You can't logically argue both points at the same time.

If this function/class was well written, i.e. it doesn't have many dependencies (it doesn't "pull in the world"), then extracting this method is absolutely trivial.

We're back at the aside comment I made. If you already assume that things were being done properly at all times, then the code shouldn't need refactoring to begin with since it would've been perfect when it was first written.

Secondly, the amount of dependencies of a class is not a guiding compass for when a chunk of private logic should be abstracted or not. The correctness of an abstraction is not defined by how much effort it takes to refactor your old code.

I feel no need to take pro-active measures against this refactoring in the future, because really, it's trivial.

It's a very common biased mistake for developers to make decisions about something they currently know a lot about (becayse they're handling it).

What you're forgetting is that it's not going to be trivial several months down the line, or when another developer has to deal with your code which he has never seen before, or when many other classes were added and the codebase itself is complex (even if the classes themselves are relatively simple), or when there's many consumers of your logic who depend on the public contract that you now need to refactor (thus leading to breaking changes).

All of these factors further compound the difficulty of having to refactor your classes. It's not hard to go around a tree, but it is hard to go through a forest. You may think you're only planting one tree today but if everyone plants trees thinking "it's just one tree", soon you'll have a forest that no one can easily cross anymore.

That's a LOT of boilerplate/overhead, that increases complexity/noise of the system, with no perceivable value if it's done "too early".

YAGNI certainly has its purpose. But YAGNI focuses on premature optimizations and overengineering based on current features.

That's not what you're talking about. What you're talking about is refusing to implement an abstraction that has already proven to be relevant, on the basis that it's a small change, you can live with the less clean implementation, and thus you are going to put this off for as long as possible.

The same response applies that I give to junior developers: if it's such a small change, why are you spending such a long time arguing just to avoid doing it? If it's a small change, just do it now and be done with it, less to keep track of. If it's a big change, then we really should look at it now because we shouldn't waste time in the future on analyzing the thing we already know and understand today.

It's 10s of extra lines, for a helper function that's only 5 lines long (and could conceivably be pretty well-written as a perfectly understandable, single-line conditional expression domain == "NSGlobalDomain" ? ".GlobalPreferences.plist" : domain + ".plist")

I really want to stress this point:

LINE COUNT IS NOT A VALID MEASURE OF CODE QUALITY

What matters is the logic, its branching complexities and its distinct responsibilities.

Secondly, condensing your logic into a single line is shorter, but is not always the most readable choice. Take it from me, someone who has written some dirty ternary operator chains (3 or 4 levels deep) in my days as a junior developer.

Do you know when I stopped doing things like that? The first time I had to revisit a project 6 months after having dropped it. Suddenly, I understood how code that makes sense when you write it (fully understanding the goal) is not necessarily readable in the future (when you're trying to refresh your memory on what its goal was).

Code is read more often than it is written. Therefore, we should prioritize the ease of reading the code over the ease of writing it. If a few extra lines means that it's much easier to understand by a developer who doesn't yet closely know the domain, then that's most definitely worth doing.

I would find myself extracting this code for the wrong reason: not because I have any intent to reuse this helper function,

Abstraction certainly promotes code reuse, but reuse is not the only reason to abstract logic.

"I want to reuse this, so I should abstract it" is correct. "This should not be abstracted since I don't want to reuse it" is not correct. The latter is not logically consistent with the former.

---

Choosing to Test a Private Method

But extracting this private method would make no difference to its stability. Public or private, if I need to change it, I will (and adjust the tests accordingly).

You're correct about the change needing to happen in either case. However, you're only thinking of the optimistic outcome of your refactoring working without additional side effects.

What happens if you happen to introduce a bug? Since you argued against unit tests and against additional abstraction, depending on the consequence of that bug you're going to either not see it, or your integration test is going to give you a vague failure that doesn't pinpoint the issue.

And that's assuming the issue is immediately visible when implementing the refactoring. If this issue remains hidden for some time, then you're going to spend a lot of time figuring out what went wrong.

If you had separated your responsibilities into separate classes, and unit tested each of them, you would've caught the issue earlier, and with more information (i.e. you'd know exactly which public behavior of which component failed).

---

Your actual questions

1. Have I misunderstood about these principles, their motivations, and costs vs benefits?

Like I said before, you're making some correct observations but they aren't actually justifying the conclusions you seem to draw from them.

There's an underlying tone in your question where you're already aware of the benefits in doing something, but your argument often boils down to "that's extra work" or "I can do that at a later time". No offense, we all encounter it in our personal careers, but that's laziness talking.

2. How should I handle this specific plistName(forDomain:) example?

Whether the path generation and file content retrieval should be separated right now or not is not conclusively answerable. There are many factors that can change this:

• Agile: generally favors keeping things simple until that implementation is no longer adequate
• Waterfall: generally favors doing things the right way from the start
• If these files are generated by an external resource, then your path format is a business requirement, and the generation of the correct path should be separated logic that can be tested directly
• If these files are generated and consumed internally in your application and amount to nothing but an implementation detail, then testing it becomes more a matter of due diligence as opposed to business requirement. I still advise testing it separately but the need to do so is lower than in the previous bullet point.
• Are you reasonably expecting other possible path formats to be included? ("reasonably" is an important modifier here!)
• Is this path format liable to change (e.g. macOS update)?
• ...

You need to weigh your options carefully here. But how you implement it doesn't change whether and how to unit test it. Unit tests don't care about implementation, only public contract. The public contract (string input, path output) seems pretty rigid in your case. But even if it wasn't, that's not an argument against unit testing.

3. Does the guidance here change depending on whether or not this is a hobby personal project, or a professional team project?

That depends what you're asking. Are you asking...

• ... what makes you more employable?
• ... what minimizes risk of project failure or budget overruns?
• ... what maximizes code quality?
• ... what maximizes the ability for different developers to work in this codebase?
• ... what minimizes regressions or uncaught mistakes?
• ... what minimizes the time to deliver the project?
• ... what the cleanest theoretical approach is barring practical considerations?
• ... what would be expected of a junior developer?
• ... what would be expected of a senior developer?

Different focus, different answer.

For example, pure CompSci theory means the context (professional/hobby) is wholly irrelevant since it focuses on algorithmic elegance/performance.

Another example, your company may focus on a strict budget more than the quality of the final product, or vice versa. I can't answer that, that's completely contextual.

I.e. would being "strict" offer benefits to a single dev working on a single project, or are they more to do with large software systems that evolve more.

Again, context is key. Even if something doesn't net you a direct benefit in a personal atmosphere, it might still be good practice/training experience to then apply that in a professional context.

I personally cut particular corners in my personal projects that I don't cute professionally. For example, I assume that my codebase will never move away from EF/MS SQL Server. Why? Because I haven't done so in 10 years, have no intention to, and my personal time is quite limited to I want to maximize my output instead of keeping my projects in limbo.
But I would never do so for a client (unless explicitly instructed), since the cost of making a wrong assumption is massively larger than for my personal pet projects.

Answer 2

A unit test has no right to know if I use private methods or not.

A unit tests job is to ensure that when I refactor I haven't changed behavior. If I can to add and remove private methods without changing behavior the unit test shouldn't break.

If you want to use unit testing to be sure a private method works then don't sneak past the access control. Just ensure your testing provides code coverage. You need to test when domain is "NSGlobalDomain" and when it's not. That covers the code. This is true if you use:

• a private method
• a ternary operator
• a map with a default value

Since there are many good ways to solve this it's silly for a unit test to lock you into a particular solution. Test the behavior you want. Not the implementation you happen to have.

A public interface might have much code written against it. A private method should not. The lack of code written against a private method is what makes it flexible. It's why we limit access. If we subvert that with testing we lose that value.

As for reusing this code elsewhere, I subscribe to the once, twice, never again rule. Being overly aggressive about removing duplicating forces you to design against an uncertain future. Only once you see the pattern should you design against it. One is an event, two is a coincidence, three is a pattern.

Keep in mind, the DRY principle isn't about identical looking code. It's about making a decision in one place. So don't worry if you're typing something familiar. Worry if you're enshrining the same idea in many places that will all need to change when that idea does. If you're doing that, well then it's worth spending the time to come up with a good name.

I mean, plistName? Come on. You can do better than that.