2

Say I have the following class:

public class ProblematicObject {
    public static void methodToTest() {
        SomeDependency dep = DependencyGetter.getDependency(SomeDependency.class);
        dep.doStuff();
    }
}

Is it an acceptable practice to modify and add methods to this class for the sake of making unit testing cleaner (aka avoiding PowerMock)? Here's what the class above would look like:

public class ProblematicObject {

    // new
    private static SomeDependency dep;

    // updated
    public static void methodToTest() {
        getSomeDependency().doStuff();
    }

    // new
    private SomeDependency getSomeDependency() {
        if (this.dep == null) {
            return DependencyGetter.getDependency(SomeDependency.class);
        }
        return dep;
    }

    // new, only used for testing, not in any impl code
    @Deprecated
    protected void setDependencyGetter(SomeDependency dep) {
        this.dep = dep;
    }

}

I seem to recall reading somewhere that adding methods for the sake of testing (instead of refactoring the problematic class) is looked down upon.

Paulo Mattos
  • 18,845
  • 10
  • 77
  • 85
Ampp3
  • 601
  • 1
  • 7
  • 12
  • Check this question, there's lots of opinions there. [How do I test a class that has private methods, fields or inner classes?](https://stackoverflow.com/questions/34571/how-do-i-test-a-class-that-has-private-methods-fields-or-inner-classes). In my view private methods should be tested via the already available public methods. Adding methods just for testing smells bad, and changing visibility for testing is a potential mistake waiting to happen. – d.j.brown Oct 13 '17 at 17:57
  • I personally think that it would defeat the purpose of unit tests to change your code specifically for unit tests. The idea of unit tests in general are to test your code in the context its expected to be used for. I don't believe that every class & every method is required to have a Unit Test case. I would recommend taking a step back and looking at the context/workflow that these private methods/variables are being used in and perhaps create a Functional Unit Test for that workflow. – Steve Oct 13 '17 at 18:08
  • The `@Deprecated` annotation is a nice touch ;) – Paulo Mattos Oct 13 '17 at 18:13
  • 2
    [*Tests trump Encapsulation*](https://8thlight.com/blog/uncle-bob/2015/06/30/the-little-singleton.html) wrote Uncle Bob Martin – M. le Rutte Oct 13 '17 at 18:14
  • 1
    Putting in methods to call in tests is like putting a manhole in a sewer: you don't need it for normal operation, but it lets you have a peek inside lets you know things aren't going wrong. – Andy Turner Oct 13 '17 at 18:15
  • Not the main point here, but your `static` usage seems a bit off (e.g. `this.dep` but dep is static; static methods calling instance methods, etc). – Paulo Mattos Oct 13 '17 at 18:25

1 Answers1

1

In this particular example, you are in fact improving your class design by making it testable.

This new unit test made you think from the point of view of the client of ProblematicObject and made you provide a way to inject that dependency.

alayor
  • 4,537
  • 6
  • 27
  • 47
  • But of cause the `ProblematicObject` would be even better changed to get the `SomeDependenc` via *constructor injection*. And `getDependency()` should not be a `static` method in `DependencyGetter`... – Timothy Truckle Oct 14 '17 at 17:14