1
votes

Someone on our team changed this code:

public class Rectangle implements Cloneable, Serializable {

    @Override
    public Rectangle clone() {
        return new Rectangle(x, y, width, height);
    }

}

to this code:

public class Rectangle implements Cloneable, Serializable {

    @Override
    public Rectangle clone() {
        try {
            // super.clone is safe to return since all of the Rectangle's fields are primitive.
            return (Rectangle) super.clone();
        } catch (CloneNotSupportedException e) {
            // should never happen since Cloneable is implemented
            return null;
        }
    }

}

They wrote a unit test that covers the try code path.

They did not write a test that covers the catch code path. The catch code path relies on things "baked" into Java, the only way to make it crash is to change the structure of the class, by dropping the Cloneable marker interface. But if that class structure changes, then the other unit test should fail.

Because the catch code path isn't covered by a unit test, the SonarQube quality gate "code coverage on new code" fails, and because the quality gate fails, the Jenkins job that builds that branch fails, and because the Jenkins job fails, Bitbucket won't allow the merge.

Already tried:

  • FAILED: Mark as false positive in SonarQube: not possible for code coverage quality gate, you can only do that for rule-based issues.
  • DIRTY HACK: Turn off (or lower) the quality gate in SonarQube before running the job, and turning it back on after the branch is merged: this works, but it feels so dirty that I think it should happen really exceptionally. I'm looking for a better solution that doesn't require a manual intervention.
  • FAILED: Mocking Rectangle: won't work, super.clone() would have to be mocked, which is on Object.clone(), which is a protected method.
  • FAILED: Fake it by having an additional class, e.g. Rectangle -> EvilRectangle -> TestRectangle, where EvilRectangle throws the CloneNotSupportedException and then TestRectangle is the actual class instantiated for the test. Won't work because that would change the signature of the clone() method, it doesn't throw CloneNotSupportedException in Rectangle.

Questions

  • Java people:
  • How does one write a unit test for the catch code path?
  • How can the code be rewritten, without changing it's public API, so that it becomes testable?
  • SonarQube people
  • How does one make the quality gate "code coverage on new code" pass?

EDIT HISTORY

  • added original code
1
"How can the code be rewritten, without changing it's public API, so that it becomes testable?" - don't use clone(). Refer to "Effective Java" for a discussion as to why. - daniu
Okay, googling "effective java don't use clone". I'll come back for additional questions if I can't figure it out. - Amedee Van Gasse
@daniu apparently "Effective Java" is an offline resource (a book). - Amedee Van Gasse
Luckily Effective Java isn't the only resource that knows why clone() is just a bad idea. You can find plenty of questions on SO about copying objects, where clone() may be suggested by someone (and debunked by many others). - Kayaman
I have added the original code, which did not contain super.clone() inside the clone() method. - Amedee Van Gasse

1 Answers

0
votes

Apache Commons Lang provides a clone method that does a deep copy and does not throw an exception. No exception means no try/catch needed, thus one less code path to test. I found this in an article discussing alternatives for Object.clone(): https://dzone.com/articles/java-cloning-copy-constructor-vs-cloning

Add this to the pom.xml:

<dependency>
  <groupId>commons-lang</groupId>
  <artifactId>commons-lang</artifactId>
  <version>2.6</version>
</dependency>

Add this to the import statements of your class:

import org.apache.commons.lang.SerializationUtils;

Your class must have implements Serializable. Add a private static final long serialVersionUID, that's apparently a best practice.

Then replace super.clone() with SerializationUtils.clone(this).

And finally, you can remove the try/catch around the clone() statement, or remove the trows from the method.

This is now my new code:

@Override
public Rectangle clone() {
    return (Rectangle) SerializationUtils.clone(this);
}

It was already covered by a unit test from before, and that unit test still passes.