Does Cobertura have a problem with the switch statement?

[ghost]

In a recent project of mine I use maven/cobertura and I wonder why a given switch statement reduces overall branch coverage to below 100% while its case branches, including the default branch have been accounted for by 2+ visitations/traversals.

Still, the switch statement will be reported as if there was some branch that was not visited/traversed yet.

Or is this a problem with the maven cobertura plugin?

[christ66]

I'm wondering if there can be a test scenario where elements is null and getTagName() can get a NPE or is null. I'm not quite sure. I will take this example and try verifying it.

[ghost]

Can and does cobertura need to make any assumptions on that? Of course element may be null, but then again, it does not have anything to do with the branch coverage.

I think that cobertura assumes that switch is an if like statement where the expression may or may not be true, and, since the second branch will never be taken, it will not record it as having been taken.

Considering also the fact that it reports 8 branches with only 7 being covered by the test cases:

And also considering the number of branches in the source:

It seems that cobertura assumes the switch to have 2 branches whereas it is rather just one or none as it will always enter the switch statement regardless of the value of element.getTagName().

Also, cobertura assumes 2 for the default case which is IMO also wrong in that the default case will always be entered.

[avivey]

The default case is only entered if no other case holds (Barring interesting games with omitting the break statement).

[christ66]

@avivey is correct in the default case. however I do believe the real issue behind it is with that default break statement. If you look at the case CoreConstants.TAG_NAME_HANDLER; you will see that the break statement actually has coverage (3 executions), however the default break does not have any (should be 13). It might be a bug in that it's showing less coverage because cobertura sees this and reads "no coverage for break statement therefore no coverage". Cobertura might think that you entered the default case however you threw an exception in the calling of super.materializeNode(parentNode, element); This case it would be a bug.

Actually if you don't mind would you mind sending me the .class file for this that way I can be 100% sure on the situation? A quick view at the byte code can explain a lot. If you can't for private reasons you can always download a program called JD (Java Decompiler) which allows you to decompile the instrumented code. It will show exactly how cobertura works and we can get to the bottom of this.

[ghost]

Here is the decompiled class file after instrumentation:

package de.axnsoftware.pengine.core.internal;

import de.axnsoftware.pengine.core.specification.nodes.IHandledNode;
import de.axnsoftware.pengine.core.specification.nodes.IHandlerNode;
import de.axnsoftware.pengine.core.specification.nodes.IProcessSpecification;
import de.axnsoftware.pengine.core.specification.nodes.ISpecificationNode;
import de.axnsoftware.pengine.core.util.ETriState;
import de.axnsoftware.pengine.core.util.PropertyHolder;
import net.sourceforge.cobertura.coveragedata.LightClassmapListener;
import net.sourceforge.cobertura.coveragedata.TouchCollector;
import org.w3c.dom.Element;

abstract class AbstractHandledNodeImpl extends AbstractTransitionalNodeImpl
  implements IHandledNode
{
  public static final transient int[] __cobertura_counters;
  private final PropertyHolder<IHandlerNode> handlerNodeHolder;

  protected AbstractHandledNodeImpl(IProcessSpecification arg1, Element arg2, String arg3)
  {
    super(processSpecification, element, tagName);

    __cobertura_counters[2] += 1; this.handlerNodeHolder = new PropertyHolder();
    __cobertura_counters[3] += 1;
  }

  public final IHandlerNode getHandler()
  {
    int i = 0; __cobertura_counters[4] += 1; i = 6; if (ETriState.UNKNOWN == this.handlerNodeHolder.getState()) { __cobertura_counters[5] += 1; i = 0;

      __cobertura_counters[7] += 1; synchronized (this.handlerNodeHolder)
      {
        __cobertura_counters[8] += 1; i = 10; if (ETriState.UNKNOWN == this.handlerNodeHolder.getState()) { __cobertura_counters[9] += 1; i = 0;

          __cobertura_counters[11] += 1; prepareNode(this.handlerNodeHolder, "handler");
        }

        __cobertura_counters[i] += 1; i = 0; __cobertura_counters[12] += 1;
      }
    }
    __cobertura_counters[i] += 1; i = 0; __cobertura_counters[13] += 1; i = 15; if (ETriState.UNDEFINED == this.handlerNodeHolder.getState()) { __cobertura_counters[14] += 1; i = 0;

      __cobertura_counters[16] += 1; throw new IllegalStateException("TBD:expected handler node, found none");
    }

    __cobertura_counters[i] += 1; i = 0; __cobertura_counters[17] += 1; return (IHandlerNode)this.handlerNodeHolder.getValue();
  }

  <T extends ISpecificationNode> T materializeNode(ISpecificationNode arg1, Element arg2)
  {
    int i = 0; __cobertura_counters[18] += 1; ISpecificationNode result = null;

    __cobertura_counters[19] += 1; String str = element.getTagName(); int j = -1; i = 20; switch (str.hashCode()) { case 692803402:
      if (20 == i) { __cobertura_counters[22] += 1; i = 0; } i = 24; if (str.equals("handler")) { __cobertura_counters[23] += 1; i = 0; j = 0; } break; } __cobertura_counters[i] += 1; if (20 == i) { __cobertura_counters[21] += 1; i = 0; } i = 0; i = 25; switch (j)
    {
    case 0:
      if (25 == i) { __cobertura_counters[27] += 1; i = 0; } __cobertura_counters[28] += 1; result = new HandlerNodeImpl((IHandledNode)parentNode, element);

      __cobertura_counters[29] += 1; break;
    default:
      if (25 == i) { __cobertura_counters[26] += 1; i = 0; } __cobertura_counters[30] += 1; result = super.materializeNode(parentNode, element);
    }

    __cobertura_counters[31] += 1; return result;
  }

  static
  {
    __cobertura_init();
  }

  public static void __cobertura_init()
  {
    if (__cobertura_counters == null)
    {
      __cobertura_counters = new int[32];
      TouchCollector.registerClass("de/axnsoftware/pengine/core/internal/AbstractHandledNodeImpl");
    }
  }

  public static void __cobertura_classmap_0(LightClassmapListener paramLightClassmapListener)
  {
    paramLightClassmapListener.putLineTouchPoint(33, 1, "<init>", "(Lde/axnsoftware/pengine/core/specification/nodes/IProcessSpecification;Lorg/w3c/dom/Element;Ljava/lang/String;)V");
    paramLightClassmapListener.putLineTouchPoint(35, 2, "<init>", "(Lde/axnsoftware/pengine/core/specification/nodes/IProcessSpecification;Lorg/w3c/dom/Element;Ljava/lang/String;)V");
    paramLightClassmapListener.putLineTouchPoint(36, 3, "<init>", "(Lde/axnsoftware/pengine/core/specification/nodes/IProcessSpecification;Lorg/w3c/dom/Element;Ljava/lang/String;)V");
    paramLightClassmapListener.putLineTouchPoint(41, 4, "getHandler", "()Lde/axnsoftware/pengine/core/specification/nodes/IHandlerNode;");
    paramLightClassmapListener.putJumpTouchPoint(41, 6, 5);
    paramLightClassmapListener.putLineTouchPoint(43, 7, "getHandler", "()Lde/axnsoftware/pengine/core/specification/nodes/IHandlerNode;");
    paramLightClassmapListener.putLineTouchPoint(45, 8, "getHandler", "()Lde/axnsoftware/pengine/core/specification/nodes/IHandlerNode;");
    paramLightClassmapListener.putJumpTouchPoint(45, 10, 9);
    paramLightClassmapListener.putLineTouchPoint(47, 11, "getHandler", "()Lde/axnsoftware/pengine/core/specification/nodes/IHandlerNode;");
    paramLightClassmapListener.putLineTouchPoint(50, 12, "getHandler", "()Lde/axnsoftware/pengine/core/specification/nodes/IHandlerNode;");
    paramLightClassmapListener.putLineTouchPoint(53, 13, "getHandler", "()Lde/axnsoftware/pengine/core/specification/nodes/IHandlerNode;");
    paramLightClassmapListener.putJumpTouchPoint(53, 15, 14);
    paramLightClassmapListener.putLineTouchPoint(55, 16, "getHandler", "()Lde/axnsoftware/pengine/core/specification/nodes/IHandlerNode;");
    paramLightClassmapListener.putLineTouchPoint(59, 17, "getHandler", "()Lde/axnsoftware/pengine/core/specification/nodes/IHandlerNode;");
    paramLightClassmapListener.putLineTouchPoint(67, 18, "materializeNode", "(Lde/axnsoftware/pengine/core/specification/nodes/ISpecificationNode;Lorg/w3c/dom/Element;)Lde/axnsoftware/pengine/core/specification/nodes/ISpecificationNode;");
    paramLightClassmapListener.putLineTouchPoint(69, 19, "materializeNode", "(Lde/axnsoftware/pengine/core/specification/nodes/ISpecificationNode;Lorg/w3c/dom/Element;)Lde/axnsoftware/pengine/core/specification/nodes/ISpecificationNode;");
    paramLightClassmapListener.putSwitchTouchPoint(69, 2147483647, new int[] { 22, 21 });
    paramLightClassmapListener.putJumpTouchPoint(69, 24, 23);
    paramLightClassmapListener.putSwitchTouchPoint(69, 2147483647, new int[] { 26, 27 });
    paramLightClassmapListener.putLineTouchPoint(73, 28, "materializeNode", "(Lde/axnsoftware/pengine/core/specification/nodes/ISpecificationNode;Lorg/w3c/dom/Element;)Lde/axnsoftware/pengine/core/specification/nodes/ISpecificationNode;");
    paramLightClassmapListener.putLineTouchPoint(75, 29, "materializeNode", "(Lde/axnsoftware/pengine/core/specification/nodes/ISpecificationNode;Lorg/w3c/dom/Element;)Lde/axnsoftware/pengine/core/specification/nodes/ISpecificationNode;");
    paramLightClassmapListener.putLineTouchPoint(79, 30, "materializeNode", "(Lde/axnsoftware/pengine/core/specification/nodes/ISpecificationNode;Lorg/w3c/dom/Element;)Lde/axnsoftware/pengine/core/specification/nodes/ISpecificationNode;");
    paramLightClassmapListener.putLineTouchPoint(84, 31, "materializeNode", "(Lde/axnsoftware/pengine/core/specification/nodes/ISpecificationNode;Lorg/w3c/dom/Element;)Lde/axnsoftware/pengine/core/specification/nodes/ISpecificationNode;");
    paramLightClassmapListener;
  }

  public static void __cobertura_classmap(LightClassmapListener paramLightClassmapListener)
  {
    paramLightClassmapListener.setClazz("de/axnsoftware/pengine/core/internal/AbstractHandledNodeImpl");
    paramLightClassmapListener.setSource("AbstractHandledNodeImpl.java");
    __cobertura_classmap_0(paramLightClassmapListener);
    paramLightClassmapListener;
  }

  public static int[] __cobertura_get_and_reset_counters()
  {
    int[] arrayOfInt = __cobertura_counters;
    __cobertura_counters = new int[__cobertura_counters.length];
    return arrayOfInt;
  }
}

[ghost]

Looking at the compiler generated switch statement, I think it is the use of string rather than integer constants that leads to the falsely interpreted coverage data, especially when looking at the compiler generated code for that switch statement...

I think I will introduce an enum for the node type and use that instead :D

[ghost]

Having analyzed the generated-decompiled code a little bit, it shows that the compiler optimized out the break statement in the default branch. Thus, cobertura never generates any line data for the source line.

In addition, the compiler makes two switch statements out of a single switch-on-string statement, thus leading cobertura to assume that there are more branches available than the source code would let you think there were.
One thing perhaps, that cobertura could account for when analyzing the first switch on the str.hashCode()?

[ghost]

Should I close this or is there an issue that needs to be fixed?

I for my part am no longer using strings in switch statements...

[christ66]

I believe this is actually a bug in cobertura in that all the branches do get covered however there's something weird going on with the default test case (Like you mentioned jdk7 optimization issue). I will mark it for next release (2.1.0) and investigate the reason for this. We do have switch statement tests, however I will try to examine those tests a little closer.

[ThrawnCA]

The issue is, Java checks the String hashCode first, for performance reasons, and only does a full equality check if the hash code matches. Thus, you have extra branches. In practice, it will actually run faster (since string hash codes are cached), and is functionally identical (because equal strings have equal hash codes), but Cobertura complains.

You can work around it by adding extra test cases with engineered String hash collisions. Subtract 1 from the second-last character, then add 19 to the hex value of the last character. Eg the word "both" becomes "bos\u0087". This will test the normally-unused branch.

[will8ug]

So, you guys are still working on this, right? As I have been running into a similar problem like this. While I'm actually using switch/case on integer.

    @Test
    void testCustomImplementationOfGetAtPutAt() {
        def user = new User(id: 1, name: 'Alex')
        assert user[0] == 1
        assert user[1] == 'Alex'
        shouldFail IllegalArgumentException, {
            assert user[-1] == 'Alex'
        }

        user[0] = 2
        user[1] = 'Bob'
        shouldFail IllegalArgumentException, {
            user[-1] = 'Bob'
        }
        assert user.id == 2
        assert user.name == 'Bob'
    }

Coverage report is like this:

My dev environment:

• java version "1.8.0_45"
• Groovy Version: 2.4.4
• Gradle 2.8

[ThrawnCA]

That's not Java code; are you using Scala?

I don't know what bytecode Scala (or Groovy, or other JVM language) generates behind the scenes, but I wouldn't be surprised if it has optimisations that cause unexpected code coverage results. It's not really a failing of Cobertura.