139

This is the code:

package com.XXX;
public final class Foo {
  private Foo() {
    // intentionally empty
  }
  public static int bar() {
    return 1;
  }
}

This is the test:

package com.XXX;
public FooTest {
  @Test 
  void testValidatesThatBarWorks() {
    int result = Foo.bar();
    assertEquals(1, result);
  }
  @Test(expected = java.lang.IllegalAccessException.class)
  void testValidatesThatClassFooIsNotInstantiable() {
    Class cls = Class.forName("com.XXX.Foo");
    cls.newInstance(); // exception here
  }
}

Works fine, the class is tested. But Cobertura says that there is zero code coverage of the private constructor of the class. How can we add test coverage to such a private constructor?

yegor256
  • 102,010
  • 123
  • 446
  • 597
  • It seems to me as if you are trying to enforce the Singleton pattern. If so, you might like dp4j.com (which does exactly that) – simpatico Feb 22 '11 at 20:59
  • shouldn't "intentionally empty" be replaced with throwing exception? In that case you could write test that expect that specific exception with specific message, no? not sure if this is overkill – Ewoks Sep 20 '17 at 10:04

18 Answers18

173

I don't entirely agree with Jon Skeet. I think that if you can get an easy win to give you coverage and eliminate the noise in your coverage report, then you should do it. Either tell your coverage tool to ignore the constructor, or put the idealism aside and write the following test and be done with it:

@Test
public void testConstructorIsPrivate() throws NoSuchMethodException, IllegalAccessException, InvocationTargetException, InstantiationException {
  Constructor<Foo> constructor = Foo.class.getDeclaredConstructor();
  assertTrue(Modifier.isPrivate(constructor.getModifiers()));
  constructor.setAccessible(true);
  constructor.newInstance();
}
drvdijk
  • 5,556
  • 2
  • 29
  • 48
Javid Jamae
  • 8,741
  • 4
  • 47
  • 62
  • 28
    But this is eliminating noise in the coverage report by *adding noise* to the test suite. I would have just ended the sentence at "put the idealism aside". :) – Christopher Orr Oct 17 '11 at 15:27
  • 12
    To give this test any sort of meaning, you should probably also assert that the constructor's access level is what you expect it to be. – Jeremy Nov 23 '11 at 19:21
  • Adding the evil reflection plus Jeremy's ideas plus a meanful name like "testIfConstructorIsPrivateWithoutRaisingExceptions", I guess this is "THE" answer. – Eduardo Costa Dec 06 '11 at 14:52
  • 1
    This is syntactically incorrect is it not? What is `constructor`? Shouldn't `Constructor` be parameterized and not a raw type? – Adam Parkin Mar 22 '13 at 18:41
  • As TDD developer call the test "testConstructorIsPrivate()" and use this test to motivate the createion of the private constructor. – timomeinen Mar 26 '13 at 09:00
  • 7
    This is wrong: `constructor.isAccessible()` always returns false, even on a public constructor. One should use `assertTrue(Modifier.isPrivate(constructor.getModifiers()));`. – timomeinen Mar 26 '13 at 09:08
  • @timomeinen - I'm no longer actively developing in Java, so I'm too lazy to test out your suggestion, but I'll take your word for it. I'm sure I'll get more comments if you're wrong. :-) – Javid Jamae Mar 26 '13 at 16:08
  • @AdamParkin yes this was syntactically incorrect, but not because of the lack of parameterization, but because of the missing throws clauses. – drvdijk Mar 30 '14 at 18:24
  • 3
    If you use JaCoCo, >= 0.7.10 [will ignore empty private constructors](https://github.com/jacoco/jacoco/pull/529) – lightswitch05 Oct 25 '17 at 15:50
  • @lightswitch05 unfortunately if you go for the belt-and-braces approach of `throw new UnsupportedOperationException();` in the constructor body to make sure no-one is trying to instantiate by reflection then JaCoCo doesn't ignore it – SteveR Nov 22 '22 at 13:26
97

Well, there are ways you could potentially use reflection etc - but is it really worth it? This is a constructor which should never be called, right?

If there's an annotation or anything similar that you can add to the class to make Cobertura understand that it won't be called, do that: I don't think it's worth going through hoops to add coverage artificially.

EDIT: If there's no way of doing it, just live with the slightly reduced coverage. Remember that coverage is meant to be something which is useful to you - you should be in charge of the tool, not the other way round.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • @Vincenzo: Well, that excludes the whole class - you only want to exclude the constructor. – Jon Skeet Dec 23 '10 at 16:10
  • 18
    I don't want to "slightly reduce coverage" in the entire project just because of this particular constructor.. – yegor256 Dec 23 '10 at 20:34
  • 40
    @Vincenzo: Then IMO you're placing too high a value on a simple number. Coverage is an *indicator* of testing. Don't be a slave to a tool. The point of coverage is to give you a level of confidence, and to suggest areas for extra testing. Artificially calling an otherwise unused constructor doesn't help with either of those points. – Jon Skeet Dec 23 '10 at 20:54
  • 21
    @JonSkeet: I totally agree with "Don't be a slave to a tool", but it does not smell good to remember every "flaw count" in every project. How to make sure the 7/9 result is Cobertura limitation, and not programmer's? A new programmer must enter every failure (that can be a lot in large projects) to check class-by-class. – Eduardo Costa Dec 06 '11 at 14:50
  • 1
    Archimedes has it right IMO. He _is_ testing the code, and at the same time providing full coverage. He is ensuring that coding standards are followed and thereby making it harder for a coder to use the class improperly. Also, given that he's captured the coding standards in a test, the effort to test becomes negligible and there is a real benefit realized. I swiped the code and I'm using it now. Thanks Archimedes :) – Matt Friedman Feb 14 '13 at 00:47
  • 5
    This does not answer the question. and by the way, some managers look at the coverage numbers. They don't care why. They know that 85% is better than 75%. – ACV Mar 09 '16 at 12:47
  • 2
    @ACV: Then what they "know" is incorrect. If you have two sets of tests covering the same code, there's no guarantee that the tests with higher coverage will actually be better tests. I would have no hesitation in arguing with a manager who started applying dubious metrics of quality without understanding them. – Jon Skeet Mar 09 '16 at 12:54
  • 2
    A practical use case to test otherwise inaccessible code is to achive 100% test coverage so that no one has to look at that class again. If the coverage is stuck at 95% many developers may attempt to figure out the reason for that just to bump into this issue over and over again. – thisismydesign Feb 22 '17 at 15:56
  • @JonSkeet "there's no guarantee that the tests with higher coverage will actually be better tests." -- and here might come handy mutation testing - which will tell how good your test cases are. Check it out: http://pitest.org/ – user1697575 Nov 19 '19 at 13:56
  • I think/agree-with-those .. that it is mostly a meaningless unit-test. However...I have a "big brother" sonar system telling me my build fails if I don't hit 80% coverage...so while I beat the drum hard for "I need meaningful unit-tests, not 'get the % meaningless unit-tests", big brother sonar doesn't always agree with me. Especially for small changes (like adding a null safety-check) where my total lines are small, but a new sonar rule makes me put in a comment about the empty-body constructor. So I tend to not care about the (IMHO) mostly meaningless empty-constructor unit test. my2cents – granadaCoder Aug 23 '23 at 10:57
83

Although it's not necessarily for coverage, I created this method to verify that the utility class is well defined and do a bit of coverage as well.

/**
 * Verifies that a utility class is well defined.
 * 
 * @param clazz
 *            utility class to verify.
 */
public static void assertUtilityClassWellDefined(final Class<?> clazz)
        throws NoSuchMethodException, InvocationTargetException,
        InstantiationException, IllegalAccessException {
    Assert.assertTrue("class must be final",
            Modifier.isFinal(clazz.getModifiers()));
    Assert.assertEquals("There must be only one constructor", 1,
            clazz.getDeclaredConstructors().length);
    final Constructor<?> constructor = clazz.getDeclaredConstructor();
    if (constructor.isAccessible() || 
                !Modifier.isPrivate(constructor.getModifiers())) {
        Assert.fail("constructor is not private");
    }
    constructor.setAccessible(true);
    constructor.newInstance();
    constructor.setAccessible(false);
    for (final Method method : clazz.getMethods()) {
        if (!Modifier.isStatic(method.getModifiers())
                && method.getDeclaringClass().equals(clazz)) {
            Assert.fail("there exists a non-static method:" + method);
        }
    }
}

I have placed the full code and examples in https://github.com/trajano/maven-jee6/tree/master/maven-jee6-test

krico
  • 5,723
  • 2
  • 25
  • 28
Archimedes Trajano
  • 35,625
  • 19
  • 175
  • 265
  • 12
    +1 Not only does this solve the problem without tricking the tool, but it fully tests the coding standards of setting up a utility class. I had to change the accessibility test to use `Modifier.isPrivate` as `isAccessible` was returning `true` for private constructors in some cases (mocking library interference?). – David Harkness Nov 11 '12 at 00:40
  • 4
    I really want to add this to JUnit's Assert class, but don't want to take credit for your work. I think it's very good. It would be great to have `Assert.utilityClassWellDefined()` in JUnit 4.12+. Have you considered a pull request? – Visionary Software Solutions Feb 25 '13 at 11:13
  • Note that using `setAccessible()` to make the constructor accessible causes problems for Sonar's code coverage tool (when I do this the class disappears from the code coverage reports of Sonar). – Adam Parkin Mar 22 '13 at 20:14
  • Thanks, I do reset the accessible flag though. Perhaps it's a bug on Sonar itself? – Archimedes Trajano Mar 23 '13 at 03:31
  • I looked at my Sonar report for coverage on my batik maven plugin, it seems to cover correctly. http://site.trajano.net/batik-maven-plugin/cobertura/index.html – Archimedes Trajano Apr 04 '14 at 13:21
  • That should be a core assert. Thx for sharing. (I forgot the final modifier for the class ! Worth it!) – Snicolas Aug 28 '14 at 15:19
  • Great idea! It is not "solving the coverage problem" but rather enforcing proper coding standards. – krico Oct 31 '14 at 08:57
  • @ArchimedesTrajano Great idea. I think calling `getDeclaredMethods` will be a more thorough check since it will catch private instance methods. – Sam Berry Dec 01 '14 at 13:34
  • I thought about it @SamB. but I didn't go for it because there can be Utility methods with some internal class that they want class level methods to perform but does not expose any of them to the public which I had allowed for. – Archimedes Trajano Feb 26 '15 at 16:24
  • Why are you reverting accessibility with `setAccessible(false)`? What you're modifying, is your local "reflective handle" to the class constructor. Resetting _could_ have some sense except you're not using that "handle" anymore. – Piotr Findeisen Mar 13 '16 at 15:28
  • Because the constructor is private so I disabled the fact that it is private. However it should be private to begin with as such I disable access to it after the test. – Archimedes Trajano Mar 13 '16 at 15:54
  • @ArchimedesTrajano I think Piotr's point is that `setAccessible` only operates on that instance of the `Constructor` object, not on the constructor "itself". You can verify this by calling `getDeclaredConstructor()` again after `setAccessible(true)` -- `isAccessible()` will still be `false` on the second return value. (I wasn't sure till I verified it myself.) – David Moles Mar 20 '16 at 21:56
  • Thanks! exactly what i needed to test the private constructor – lucasmonteiro001 Apr 26 '18 at 12:39
21

I had made private the constructor of my class of static utility functions, to satisfy CheckStyle. But like the original poster, I had Cobertura complaining about the test. At first I tried this approach, but this doesn't affect the coverage report because the constructor is never actually executed. So really all this tests is if the constructor is remaining private - and this is made redundant by the accessibility check in the subsequent test.

@Test(expected=IllegalAccessException.class)
public void testConstructorPrivate() throws Exception {
    MyUtilityClass.class.newInstance();
    fail("Utility class constructor should be private");
}

I went with Javid Jamae's suggestion and used reflection, but added assertions to catch anybody messing with the class being tested (and named the test to indicate High Levels Of Evil).

@Test
public void evilConstructorInaccessibilityTest() throws Exception {
    Constructor[] ctors = MyUtilityClass.class.getDeclaredConstructors();
    assertEquals("Utility class should only have one constructor",
            1, ctors.length);
    Constructor ctor = ctors[0];
    assertFalse("Utility class constructor should be inaccessible", 
            ctor.isAccessible());
    ctor.setAccessible(true); // obviously we'd never do this in production
    assertEquals("You'd expect the construct to return the expected type",
            MyUtilityClass.class, ctor.newInstance().getClass());
}

This is so overkill, but I gotta admit I like the warm fuzzy feeling of 100% method coverage.

Ben Hardy
  • 1,739
  • 14
  • 16
14

With Java 8, it is possible to find other solution.

I assume that you simply want to create utility class with few public static methods. If you can use Java 8, then you can use interface instead.

package com.XXX;

public interface Foo {

  public static int bar() {
    return 1;
  }
}

There is no constructor and no complain from Cobertura. Now you need to test only the lines you really care about.

Arnost Valicek
  • 2,418
  • 1
  • 18
  • 14
  • 2
    Unfortunately however, you can't declare the interface as "final", preventing anyone from subclassing it - otherwise this would be the best approach. – Michael Berry Jul 15 '19 at 17:28
  • Because subclassing would... give you access to already public methods? Let's not try to idiot proof everything when it is actually a great solution. – john16384 Feb 27 '21 at 20:29
6

The following worked to me on a class created with Lombok annotation @UtilityClass, that automatically add a private constructor.

@Test
public void testConstructorIsPrivate() throws IllegalAccessException, InvocationTargetException, InstantiationException, NoSuchMethodException {
    Constructor<YOUR_CLASS_NAME> constructor = YOUR_CLASS_NAME.class.getDeclaredConstructor();
    assertTrue(Modifier.isPrivate(constructor.getModifiers())); //this tests that the constructor is private
    constructor.setAccessible(true);
    assertThrows(InvocationTargetException.class, () -> {
        constructor.newInstance();
    }); //this add the full coverage on private constructor
}

Although constructor.setAccessible(true) should works when private constructor has been written manually, with Lombok annotation doesn't work, since it force it. Constructor.newInstance() actually tests that the constructor is invoked and this complete the coverage of the costructor itself. With the assertThrows you prevent that the test fails and you managed the exception since is exactly the error that you expect. Although this is a workaround and I don't appreciate the concept of "line coverage" vs "functionality/behavior coverage" we can find a sense on this test. In fact you are sure that the Utility Class has actually a private Constructor that correctly throws an exception when invoked also via reflaction. Hope this helps.

5

Newer versions of Cobertura have built-in support for ignoring trivial getters/setters/constructors:

https://github.com/cobertura/cobertura/wiki/Ant-Task-Reference#ignore-trivial

Ignore Trivial

Ignore trivial allows the ability to exclude constructors/methods that contain one line of code. Some examples include a call to a super constrctor only, getter/setter methods, etc. To include the ignore trivial argument add the following:

<cobertura-instrument ignoreTrivial="true" />

or in a Gradle build:

cobertura {
    coverageIgnoreTrivial = true
}
Mike Buhot
  • 4,790
  • 20
  • 31
5

The reasoning behind testing code that doesn't do anything is to achieve 100% code coverage and to notice when the code coverage drops. Otherwise one could always think, hey I don't have 100% code coverage anymore but it's PROBABLY because of my private constructors. This makes it easy to spot untested methods without having to check that it just was a private constructor. As your codebase grows you'll actually feel a nice warm feeling looking at 100% instead of 99%.

IMO it's best to use reflection here since otherwise you would have to either get a better code coverage tool that ignores these constructors or somehow tell the code coverage tool to ignore the method (perhaps an Annotation or a configuration file) because then you would be stuck with a specific code coverage tool.

In a perfect world all code coverage tools would ignore private constructors that belong to a final class because the constructor is there as a "security" measure nothing else:)
I would use this code:

    @Test
    public void callPrivateConstructorsForCodeCoverage() throws SecurityException, NoSuchMethodException, IllegalArgumentException, InstantiationException, IllegalAccessException, InvocationTargetException
    {
        Class<?>[] classesToConstruct = {Foo.class};
        for(Class<?> clazz : classesToConstruct)
        {
            Constructor<?> constructor = clazz.getDeclaredConstructor();
            constructor.setAccessible(true);
            assertNotNull(constructor.newInstance());
        }
    }
And then just add classes to the array as you go.
jontejj
  • 2,800
  • 1
  • 25
  • 27
4

Don't. What's the point in testing an empty constructor? Since cobertura 2.0 there is an option to ignore such trivial cases (together with setters/getters), you can enable it in maven by adding configuration section to cobertura maven plugin:

<configuration>
  <instrumentation>
    <ignoreTrivial>true</ignoreTrivial>                 
  </instrumentation>
</configuration>

Alternatively you can use Coverage Annotations: @CoverageIgnore.

Krzysztof Krasoń
  • 26,515
  • 16
  • 89
  • 115
4

My preferred option in 2019+: Use lombok.

Specifically, the @UtilityClass annotation. (Sadly only "experimental" at the time of writing, but it functions just fine and has a positive outlook, so likely to soon be upgraded to stable.)

This annotation will add the private constructor to prevent instantiation and will make the class final. When combined with lombok.addLombokGeneratedAnnotation = true in lombok.config, pretty much all testing frameworks will ignore the auto-generated code when calculating test coverage, allowing you to bypass coverage of that auto-generated code with no hacks or reflection.

Michael Berry
  • 70,193
  • 21
  • 157
  • 216
3

Finally, there is solution!

public enum Foo {;
  public static int bar() {
    return 1;
  }
}
kan
  • 28,279
  • 7
  • 71
  • 101
  • How is that testing the class which is posted in the question though? You shouldn't assume that you can turn every class with a private constructor into an enum, or that you'd want to. – Jon Skeet Jan 27 '12 at 16:04
  • @JonSkeet I can for the class in question. And most of utility classes which have only bunch of static methods. Otherwise a class with the only private constructor doesn't have any sense. – kan Jan 27 '12 at 16:09
  • 1
    A class with a private constructor *may* be instantiated from public static methods, although of course then it's easy to get the coverage. But fundamentally I would prefer any class which extends `Enum` to really be an enum... I believe that reveals intent better. – Jon Skeet Jan 27 '12 at 16:11
  • @JonSkeet The question assumes a class which should not be instantiated. Otherwise it should be tested. A class which could not be ever instantiated is an `enum`. And well... yes, it's question of preferences, I prefer 100% coverage more than some vague principles. – kan Jan 27 '12 at 16:17
  • 4
    Wow, I'd *absolutely* prefer code which makes sense over a pretty arbitrary number. (Coverage is no guarantee of quality, nor is 100% coverage feasible in all cases. Your tests should *guide* your code at best - not steer it over a cliff of bizarre intentions.) – Jon Skeet Jan 27 '12 at 16:20
  • 1
    @Kan: Adding a dummy call to the constructor to bluff the tool should not be the intent. Anyone who relies on a single metric to determine project's wellbeing is already on the path to destruction. – Apoorv Aug 06 '12 at 11:45
  • This is relying on language semantics due to a design decision of the underlying platform as a guide of when to make Enums. Enums are enumerations, specifically designed to show a ranked collection of items that are statically known. A static "utils" type class may be a code smell, but that would not make it necessarily an enum. This choice would fundamentally be poor. – Visionary Software Solutions Dec 19 '12 at 10:18
1

I don't know about Cobertura but I use Clover and it has a means of adding pattern-matching exclusions. For example, I have patterns that exclude apache-commons-logging lines so they are not counted in the coverage.

John Engelman
  • 4,119
  • 1
  • 16
  • 12
1

ClassUnderTest testClass=Whitebox.invokeConstructor(ClassUnderTest.class);

acpuma
  • 479
  • 5
  • 15
1

Another option is to create a static initializer similar to the following code

class YourClass {
  private YourClass() {
  }
  static {
     new YourClass();
  }

  // real ops
}

This way the private constructor is considered tested, and the runtime overhead is basically not measurable. I do this to get 100% coverage using EclEmma, but likely it works out for every coverage tool. The drawback with this solution, of course, is that you write production code (the static initializer) just for testing purposes.

0

If I were to guess the intent of your question I'd say:

  1. You want reasonable checks for private constructors that do actual work, and
  2. You want clover to exclude empty constructors for util classes.

For 1, it is obvious that you want all initialisation to be done via factory methods. In such cases, your tests should be able to test the side effects of the constructor. This should fall under the category of normal private method testing. Make the methods smaller so that they only do a limited number of determinate things (ideally, just one thing and one thing well) and then test the methods that rely on them.

For example, if my [private] constructor sets up my class's instance fields a to 5. Then I can (or rather must) test it:

@Test
public void testInit() {
    MyClass myObj = MyClass.newInstance(); //Or whatever factory method you put
    Assert.assertEquals(5, myObj.getA()); //Or if getA() is private then test some other property/method that relies on a being 5
}

For 2, you can configure clover to exclude Util constructors if you have a set naming pattern for Util classes. E.g., in my own project I use something like this (because we follow the convention that names for all Util classes should end with Util):

<clover-setup initString="${build.dir}/clovercoverage.db" enabled="${with.clover}">
    <methodContext name="prvtCtor" regexp="^private *[a-zA-Z0-9_$]+Util *( *) *"/>
</clover-setup>

I have deliberately left out a .* following ) because such constructors are not meant to throw exceptions (they are not meant to do anything).

There of course can be a third case where you may want to have an empty constructor for a non-utility class. In such cases, I would recommend that you put a methodContext with the exact signature of the constructor.

<clover-setup initString="${build.dir}/clovercoverage.db" enabled="${with.clover}">
    <methodContext name="prvtCtor" regexp="^private *[a-zA-Z0-9_$]+Util *( *) *"/>
    <methodContext name="myExceptionalClassCtor" regexp="^private MyExceptionalClass()$"/>
</clover-setup>

If you have many such exceptional classes then you can choose to modify the generalized private constructor reg-ex I suggested and remove Util from it. In this case, you will have to manually make sure that your constructor's side effects are still tested and covered by other methods in your class/project.

<clover-setup initString="${build.dir}/clovercoverage.db" enabled="${with.clover}">
    <methodContext name="prvtCtor" regexp="^private *[a-zA-Z0-9_$]+ *( *) .*"/>
</clover-setup>
Apoorv
  • 1,091
  • 6
  • 19
0
@Test
public void testTestPrivateConstructor() {
    Constructor<Test> cnt;
    try {
        cnt = Test.class.getDeclaredConstructor();
        cnt.setAccessible(true);

        cnt.newInstance();
    } catch (Exception e) {
        e.getMessage();
    }
}

Test.java is your source file,which is having your private constructor

Walery Strauch
  • 6,792
  • 8
  • 50
  • 57
  • It would be nice to explain, why this construct helps with coverage. – Markus Mar 31 '13 at 21:36
  • True, and secondly: why catching an exception in your test? The exception being thrown should actually make the test fail. – Jordi May 06 '13 at 15:23
0

Sometimes Cobertura marks code not intended to be executed as 'not covered', there's nothing wrong with that. Why are you concerned with having 99% coverage instead of 100%?

Technically, though, you can still invoke that constructor with reflection, but it sounds very wrong to me (in this case).

Nikita Rybak
  • 67,365
  • 22
  • 157
  • 181
-2

You can't.

You're apparently creating the private constructor to prevent instantiation of a class that is intended to contain only static methods. Rather than trying to get coverage of this constructor (which would require that the class be instantiated), you should get rid of it and trust your developers not to add instance methods to the class.

Anon
  • 2,654
  • 16
  • 10