0

I've been trying to refactor some existing code which is in essence a giant nested procedural call inside what should otherwise be an object oriented architecture. The entry point to the relevant code is a static function, which iteslf calls other static functions calling other static functions etc. totaling to a few thousand lines of code. At a high level, the calls perform two actions of

  1. Performing metadata updates prior to real work
  2. Executing work related to the call

For the metadata updates, it can be further subdivided into 3 sections which are relatively granular

  1. Query for some data related to the overall call
  2. Use information from step 1 to update physical file level metadata
  3. Use information from step 1 to perform a metadata update at a higher abstraction level than step 2

Originally I had created a class to handle all of the metata updates, with 3 private methods for each of the work. Roughly the following code

class Metadata(...) {
  def updateOnEvent(...): Unit = {
    val data = queryFromStep1()

    if (data.requiresUpdating) {
      updatePhsicalFileMetadata(data)
    }
    updateHigherLevelMetadata(data)
  }

  private def queryFromStep1(): Data = ???

  private def updatePhsicalFileMetadata(data: Data): Unit = ???

  private def updateHigherLevelMetadata(data: Data): Unit = ???
}

The issue I'm having is that I feel the urge to unit test the 3 private methods here, and my understanding is that this is a sign of a poor class abstraction.

My next thought was to extract each method into a separate class, and test each class individually, in addition to tests for the Metadata class to make sure everything is glued together properly. However this doesn't seem to be a great solution either since now we have many tiny classes each exposing only a single public method, with no private methods either.

I'm wondering what a better solution could possibly be. I'm hesitant to expose all 4 methods as public from the Metadata class, because the caller really only cares about updateOnEvent for the purpose of handling an event. Is there a simple solution I'm missing? I can provide more details if needed.

4
  • Does this answer your question? Testing private methods as protected – gnat Mar 13 at 23:12
  • @gnat is the gist of the idea that I should gain coverage for these 3 private methods by calling the one public method in various scenarios that end up testing the private methods indirectly? – User Mar 14 at 0:42
  • 1
    "this is a sign of a poor class abstraction" - maybe, but there needs to be a balance; maybe it will be helpful if you think about it in this way: the idea is that there should be an inner "core" of the application (or of a component) hidden behind "surface level" classes/interfaces that are called by code in an outer layer (or in a different component). For the same reason that other code should not depend on the internals of the layer/component, the test should not depend on the system/component under test (they are just another client to it); this lets you restructure those internals. – Filip Milovanović Mar 14 at 4:19
  • "I feel the urge to unit test the 3 private methods here" - it could be that these methods are still too big or do too much in some sense (in terms of responsibilities) – Filip Milovanović Mar 14 at 4:21
2

Why did you make these methods private in the first place? Hopefully you wanted to hide implementation details. Doing so helps keep the relationship between this class and code that uses it simple. That is to say, decoupled.

If you let test code sneak past that public method then the test code defeats that attempt at decoupling. Let that happen and refactoring becomes a huge pain.

My next thought was to extract each method into a separate class, and test each class individually, in addition to tests for the Metadata class to make sure everything is glued together properly. However this doesn't seem to be a great solution either since now we have many tiny classes each exposing only a single public method, with no private methods either.

Tiny classes are a good thing. There is no rule that a class must have private methods.

But private methods are not the only way to hide an implementation. You can use one object to hide many other objects. The facade pattern does exactly this. That still hides the implementation so long as using code respects the pattern and uses the object that offers that simplified interface. That abstraction needs to be tested. Testing it documents it.

Now the objects behind that facade object can have their own public methods. And you can write code that tests those public methods. Why is this OK when they are the same methods that used to be private?

Because now the code that tests the facade objects interface (an abstraction) and code that pokes around in it's implementation are clearly separated. This makes it easier to delete the tests when you delete the methods/classes that go with them. Something you can do with confidence only because you have good tests on the facade.

The danger here is that you start to care about code that you never use. Code that is tested when the facade is tested you know can get used. Code that only gets called by tests that reach behind the facade might never be needed and will clutter your code base.

The lesson here is that the structure of your code doesn't tell you what to test. After all, you can just restructure your code. You should test an abstraction. When you reach behind that abstraction you are not supporting it. You may be supporting another abstraction that has a good reason to be tested on it's own. But that doesn't help the one you reached past at all.

So it comes down to whether you have a real need to support more than the one abstraction. Sometimes you do. Abstractions can nest. But remember, I'm just a mere mortal. Don't make it complicated if you don't need to. Or you'll just end up confusing me.

0

Have a look at your development software first. Some have an option to mark a function as “testable” which means it may be private and usually no accessible from the outside, but the compiler makes an exception when compiling unit tests.

If you can only test public methods, and you have methods that you want to be private and that you want to test, then something must give. You can make the method public even though it shouldn’t be called from the outside. You can not test it. You can rename the function from “someFunction” to “someFunctionPublicForTestingOnly“. The last one is ugly but works.

Now some people will tell you to test only the public method. That’s misguided. If the actions of your public method are complex enough that you need to split it up, and the parts are difficult enough to contain possible bugs, then you should unit test these methods. Next they tell you to split up your class - so your private methods are pointlessly exposed to the world.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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