Tests improve code quality, but only for the parts of the code base that are actually being tested.
You need a tool that tells you which parts of a program are not being tested so you can write tests for those parts and find more bugs.
If it isn't tested it's buggy.
If you aren't measuring it, it isn't as good as you think.
Roughly 1 in 5 to 2 in 3 new tests find unexpected bugs.
Cenqua Clover $250-$2500 payware http://www.cenqua.com/clover/
Cobertura (GPL): http://cobertura.sourceforge.net/
Coverlipse Eclipse plug-in: http://coverlipse.sourceforge.net/index.php
Jester: http://jester.sourceforge.net/
Tool instruments byte code with extra code to measure which statements are and are not reached.
Test suite is run. Data is collected.
Data is analyzed to generate reports (often in HTML) showing line by line what is and is not reached.
Any line that isn't reached isn't being tested.
I'm going to use JUnit 3.8 in the examples, but this approach is normally test framework agnostic.
Does covered == tested? A necessary but not sufficient condition.
http://www.cafeconleche.org/cobertura-jaxen/
Look in FractionFormat
. They're at least two (minor) bugs in untested code.
Measure your coverage to identify untested code
Write tests
Run tests
Fix bugs
Repeat
Write one test for any untested package.
When all packages are tested, write one test for any untested class.
When all classes are tested, write one test for any untested method.
When all methods are tested, write one test for any untested line.
taskdef element
cobertura-instrument task
test target that runs test task with instrumented classes instead of regular classes
<taskdef classpath="cobertura.jar" resource="tasks.properties" />
<target name="instrument">
<cobertura-instrument todir="target/instrumented-classes">
<fileset dir="target/classes">
<include name="**/*.class"/>
</fileset>
</cobertura-instrument>
</target>
<target name="cover-test" depends="instrument">
<mkdir dir="${testreportdir}" />
<junit dir="./" failureproperty="test.failure" printSummary="yes"
fork="true" haltonerror="true">
<!-- Normally you can create this task by copying your existing JUnit
target, changing its name, and adding these next two lines.
You may need to change the locations to point to wherever
you've put the cobertura.jar file and the instrumented classes. -->
<classpath location="cobertura.jar"/>
<classpath location="target/instrumented-classes"/>
<classpath>
<fileset dir="${libdir}">
<include name="*.jar" />
</fileset>
<pathelement path="${testclassesdir}" />
<pathelement path="${classesdir}" />
</classpath>
<batchtest todir="${testreportdir}">
<fileset dir="src/java/test">
<include name="**/*Test.java" />
<include name="org/jaxen/javabean/*Test.java" />
</fileset>
</batchtest>
</junit>
</target>
% ant instrument
% ant cover-test
% ant coverage-report
<reports>
<report>maven-jdepend-plugin</report>
<report>maven-checkstyle-plugin</report>
<report>maven-cobertura-plugin</report>
<report>maven-changes-plugin</report>
<report>maven-changelog-plugin</report>
<report>maven-developer-activity-plugin</report>
<report>maven-file-activity-plugin</report>
<report>maven-license-plugin</report>
<report>maven-javadoc-plugin</report>
<report>maven-jxr-plugin</report>
<report>maven-junit-report-plugin</report>
<report>maven-linkcheck-plugin</report>
<report>maven-tasklist-plugin</report>
<report>maven-pmd-plugin</report>
</reports>
...
<dependencies>
<dependency>
<groupId>xerces</groupId>
<artifactId>xmlParserAPIs</artifactId>
<version>2.6.2</version>
</dependency>
...
<dependency>
<groupId>maven-plugins</groupId>
<artifactId>maven-cobertura-plugin</artifactId>
<version>1.1.1</version>
<type>plugin</type>
</dependency>
</dependencies>
Line coverage
Branch coverage
Case coverage
Does covered == tested?
The less that's public the better
Tests should only test the public (including protected) interfaces
Code that can't be reached through the public interfaces can be deleted
Test should not need to change when private parts change; enables refactoring and optimization
Want to test code as it's actually used; not as you think it might be
Working through the public API to find the untested parts teaches you about code flow
Code that can't be reached can be deleted
Naming convention mistakes (e.g. tesSomeReallyComplexCondition()
) can prevent tests from running
Whole test cases can be accidentally omitted from a suite.
Complicated conditions inside a test method can keep part of the tests from being reached.
100% coverage is not always achievable
But 90% coverage is grunt work
And 95%+ is not hard
Testing protected methods by writing subclasses as inner classes
Tests that require particular libraries or other environmental conditions
Test that use mock objects to force things correctly behaving software shouldn't do
private constructors that prevent instantiation of utility classes that only contain static methods; e.g. java.lang.Math
Example
throw new UnsupportedOperationException()
for unimplemented, never-called methods in non-public classes required only to implement an interface; Example: ChildIterator
throw new RuntimeException()
in catch blocks for exceptions that "can't happen" in a correctly functioning VM; e.g. catch UnsupportedEncodingException for the UTF-8 encoding; Example:
nu.xom.Text.build()
fail()
statements when testing for exceptions to be thrown
platform or VM-specific code; especially code for platform specific bugs or workarounds
Calls to System.exit()
:
I checked with JUnit 4 and noticed that it doesn't call System.exit(), so that's something we need to fix.--Kent Beck on the junit mailing list, October 12, 2005
Private implementations of public interfaces
Also private subclasses of public abstract classes
throw new UnsupportedOperationException()
for unimplemented, never-called methods in non-public classes required only to implement an interface
package nu.xom.xslt;
/**
*
* <p>
* This is just for XSLTransform, and implements only the functionality
* that class requires. Other classes should not use this.
* It is far from a conformant implementation of XMLReader.
* </p>
*
*/
class XOMReader implements XMLReader {
private SAXConverter converter;
public boolean getFeature(String uri)
throws SAXNotRecognizedException, SAXNotSupportedException {
if ("http://xml.org/sax/features/namespace-prefixes".equals(uri)
|| "http://xml.org/sax/features/namespaces".equals(uri)) {
return true;
}
throw new SAXNotRecognizedException("XOMReader doesn't support features");
}
public void setFeature(String uri, boolean value)
throws SAXNotRecognizedException, SAXNotSupportedException {
}
public Object getProperty(String uri) throws SAXNotRecognizedException,
SAXNotSupportedException {
if ("http://xml.org/sax/properties/lexical-handler".equals(uri)) {
return converter.getLexicalHandler();
}
else {
throw new SAXNotRecognizedException("XOMReader doesn't support features");
}
}
public void setProperty(String uri, Object value)
throws SAXNotRecognizedException, SAXNotSupportedException {
if ("http://xml.org/sax/properties/lexical-handler".equals(uri)) {
LexicalHandler handler = (LexicalHandler) value;
converter.setLexicalHandler(handler);
}
else {
throw new SAXNotRecognizedException(
"XOMReader doesn't support " + uri);
}
}
public void setEntityResolver(EntityResolver resolver) {
throw new UnsupportedOperationException();
}
public EntityResolver getEntityResolver() {
return null;
}
public void setDTDHandler(DTDHandler handler) {
// throw new UnsupportedOperationException();
}
public DTDHandler getDTDHandler() {
return null;
}
public void setContentHandler(ContentHandler handler) {
converter = new SAXConverter(handler);
// warn the SAXConverter that we're using this for XSLT
converter.setContentHandler(new XSLTHandler(null));
}
public ContentHandler getContentHandler() {
return null;
}
public void setErrorHandler(ErrorHandler handler) {
}
public ErrorHandler getErrorHandler() {
return null;
}
public void parse(InputSource source)
throws IOException, SAXException {
XOMInputSource xis = (XOMInputSource) source;
converter.convert(xis.getNodes());
}
public void parse(String url) throws IOException, SAXException {
throw new UnsupportedOperationException();
}
}
fail()
public void testGetChild() {
try {
a1.getChild(0);
fail("Didn't throw IndexOutofBoundsException");
}
catch (IndexOutOfBoundsException success) {
// success
}
}
In Clover use statement contexts
Cobertura doesn't support this (yet)
Cobertura/Clover/etc. all operate by instrumenting the .class files
Must be sure to run the instrumented code rather than the actual code
Can overflow maximum method sizes; example: UnicodeUtil
Could such a tool use JVMPI instead?
Or a custom ClassLoader that instruments on the fly?
Does not instrument code
Error seeding: changes source code; then recompiles
If tests break, that line is tested; otherwise it's not.
Upside: really finds out if code is tested, not merely exercised
Downside: Takes a long time to run.
Tests must pass
Passing tests must not print anything on System.err
Make copy of source tree
$ java -classpath ../jester/jester.jar:lib/junit.jar:build/classes:lib/xalan.jar:lib/xercesImpl.jar:build/xom-1.1b5.jar junit.textui.TestRunner nu.xom.tests.AttributeTest ................................... Time: 0.196 OK (35 tests)
$ java -classpath ../jester/bin:lib/junit.jar:lib/xalan.jar:lib/xercesImpl.jar:src:build/classes:build/xom-1.1b5.jar jester.TestTester nu.xom.tests.AttributeTest src/nu/xom/Attribute.java
Be sure instrumented clases in src show up in classpath before the regular classes
~/projects/xom$ java -classpath ../jester/bin:lib/junit.jar:lib/xalan.jar:lib/xercesImpl.jar:src:build/classes:build/xom-1.1b5.jar jester.TestTester nu.xom.tests.AttributeTest src/nu/xom/Attribute.java Use classpath: ../jester/bin:lib/junit.jar:lib/xalan.jar:lib/xercesImpl.jar:src:build/classes:build/xom-1.1b5.jar Warning - could not find jester.cfg so using default configuration values. Warning - could not find ignorelist.cfg so using default ignore list. Warning - could not find mutations.cfg so using default mutations. For File src/nu/xom/Attribute.java: 54 mutations survived out of 96 changes. Score = 44 src/nu/xom/Attribute.java - changed source on line 149 (char index=5618) from 0 to 1 if (name.indexOf(':') > >>>0) { prefix = name.substring(0, n src/nu/xom/Attribute.java - changed source on line 162 (char index=5995) from if ( to if (false && _setValue(value); >>>if ("xml".equals(this.prefix) && "id".equals( src/nu/xom/Attribute.java - changed source on line 198 (char index=6904) from if ( to if (true || String prefix = ""; >>>if (qualifiedName.indexOf(':') >= 0) { src/nu/xom/Attribute.java - changed source on line 198 (char index=6904) from if ( to if (false && String prefix = ""; >>>if (qualifiedName.indexOf(':') >= 0) { src/nu/xom/Attribute.java - changed source on line 198 (char index=6938) from 0 to 1 qualifiedName.indexOf(':') >= >>>0) { prefix = qualifiedName.subst src/nu/xom/Attribute.java - changed source on line 199 (char index=6988) from 0 to 1 fix = qualifiedName.substring(>>>0, qualifiedName.indexOf(':')); i src/nu/xom/Attribute.java - changed source on line 200 (char index=7032) from if ( to if (true || me.indexOf(':')); >>>if ("xml:id".equals(qualifiedName)) { src/nu/xom/Attribute.java - changed source on line 200 (char index=7032) from if ( to if (false && me.indexOf(':')); >>>if ("xml:id".equals(qualifiedName)) { src/nu/xom/Attribute.java - changed source on line 219 (char index=7607) from 0 to 1 s.length(); int pos = >>>0; while (pos < length && s.charAt(po src/nu/xom/Attribute.java - changed source on line 220 (char index=7655) from == to != pos < length && s.charAt(pos) >>>== ' ') pos++; s = s.substring(pos); src/nu/xom/Attribute.java - changed source on line 222 (char index=7729) from 1 to 2 int end = s.length()->>>1; while (end > 0 && s.charAt(end) == src/nu/xom/Attribute.java - changed source on line 223 (char index=7753) from 0 to 1 gth()-1; while (end > >>>0 && s.charAt(end) == ' ') end--; s = src/nu/xom/Attribute.java - changed source on line 223 (char index=7772) from == to != ile (end > 0 && s.charAt(end) >>>== ' ') end--; s = s.substring(0, end src/nu/xom/Attribute.java - changed source on line 224 (char index=7811) from 0 to 1 nd--; s = s.substring(>>>0, end+1); length = s.length src/nu/xom/Attribute.java - changed source on line 224 (char index=7818) from 1 to 2 s = s.substring(0, end+>>>1); length = s.length(); src/nu/xom/Attribute.java - changed source on line 228 (char index=7939) from false to true ); boolean wasSpace = >>>false; for (int i = 0; i < length; i+ src/nu/xom/Attribute.java - changed source on line 229 (char index=7967) from 0 to 1 = false; for (int i = >>>0; i < length; i++) { char c = s. src/nu/xom/Attribute.java - changed source on line 231 (char index=8035) from if ( to if (true || c = s.charAt(i); >>>if (c == ' ') { if (wasSpace) src/nu/xom/Attribute.java - changed source on line 231 (char index=8035) from if ( to if (false && c = s.charAt(i); >>>if (c == ' ') { if (wasSpace) src/nu/xom/Attribute.java - changed source on line 231 (char index=8041) from == to != .charAt(i); if (c >>>== ' ') { if (wasSpace) conti src/nu/xom/Attribute.java - changed source on line 232 (char index=8067) from if ( to if (true || (c == ' ') { >>>if (wasSpace) continue; sb.ap src/nu/xom/Attribute.java - changed source on line 232 (char index=8067) from if ( to if (false && (c == ' ') { >>>if (wasSpace) continue; sb.ap src/nu/xom/Attribute.java - changed source on line 234 (char index=8150) from true to false ); wasSpace = >>>true; } else { src/nu/xom/Attribute.java - changed source on line 238 (char index=8246) from false to true ); wasSpace = >>>false; } } return src/nu/xom/Attribute.java - changed source on line 267 (char index=8892) from if ( to if (true || Type type) { >>>if (isXMLID() && ! Type.ID.equals(type)) { src/nu/xom/Attribute.java - changed source on line 267 (char index=8892) from if ( to if (false && Type type) { >>>if (isXMLID() && ! Type.ID.equals(type)) { src/nu/xom/Attribute.java - changed source on line 320 (char index=10485) from if ( to if (false && ing value) { >>>if ("xml".equals(this.prefix) && "id".equals( src/nu/xom/Attribute.java - changed source on line 361 (char index=11601) from if ( to if (true || LocalName(localName); >>>if (isXMLID()) { this.setType(Att src/nu/xom/Attribute.java - changed source on line 361 (char index=11601) from if ( to if (false && LocalName(localName); >>>if (isXMLID()) { this.setType(Att src/nu/xom/Attribute.java - changed source on line 444 (char index=14067) from if ( to if (false && tring URI) { >>>if ("xml".equals(prefix) && "id".equals(this. src/nu/xom/Attribute.java - changed source on line 492 (char index=15759) from if ( to if (false && t = this.getParent(); >>>if (parent != null) { // test for src/nu/xom/Attribute.java - changed source on line 496 (char index=15953) from if ( to if (false && espaceURI(prefix); >>>if (currentURI != null && !currentURI.equals( src/nu/xom/Attribute.java - changed source on line 496 (char index=15968) from != to == x); if (currentURI >>>!= null && !currentURI.equals(URI)) { src/nu/xom/Attribute.java - changed source on line 597 (char index=18965) from 1 to 2 ult = new StringBuffer(length+>>>12); for (int i = 0; i < length; i++) src/nu/xom/Attribute.java - changed source on line 719 (char index=22649) from 0 to 1 result.append('>>>0'); break; src/nu/xom/Attribute.java - changed source on line 722 (char index=22742) from 1 to 2 result.append('>>>1'); break; src/nu/xom/Attribute.java - changed source on line 725 (char index=22835) from 2 to 3 result.append('>>>2'); break; src/nu/xom/Attribute.java - changed source on line 728 (char index=22928) from 3 to 4 result.append('>>>3'); break; src/nu/xom/Attribute.java - changed source on line 731 (char index=23021) from 4 to 5 result.append('>>>4'); break; src/nu/xom/Attribute.java - changed source on line 734 (char index=23114) from 5 to 6 result.append('>>>5'); break; src/nu/xom/Attribute.java - changed source on line 737 (char index=23207) from 6 to 7 result.append('>>>6'); break; src/nu/xom/Attribute.java - changed source on line 740 (char index=23300) from 7 to 8 result.append('>>>7'); break; src/nu/xom/Attribute.java - changed source on line 743 (char index=23393) from 8 to 9 result.append('>>>8'); break; src/nu/xom/Attribute.java - changed source on line 746 (char index=23486) from 9 to 0 result.append('>>>9'); break; src/nu/xom/Attribute.java - changed source on line 772 (char index=24175) from true to false sAttribute() { return >>>true; } /** * <p> src/nu/xom/Attribute.java - changed source on line 805 (char index=25185) from 1 to 2 c final Type CDATA = new Type(>>>1); /** * <p> * src/nu/xom/Attribute.java - changed source on line 817 (char index=25707) from 2 to 3 atic final Type ID = new Type(>>>2); /** * <p> src/nu/xom/Attribute.java - changed source on line 829 (char index=26173) from 3 to 4 c final Type IDREF = new Type(>>>3); /** * <p> * src/nu/xom/Attribute.java - changed source on line 841 (char index=26686) from 4 to 5 final Type IDREFS = new Type(>>>4); /** * <p> * src/nu/xom/Attribute.java - changed source on line 851 (char index=27070) from 5 to 6 final Type NMTOKEN = new Type(>>>5); /** * <p> * src/nu/xom/Attribute.java - changed source on line 862 (char index=27495) from 6 to 7 inal Type NMTOKENS = new Type(>>>6); /** * <p> * src/nu/xom/Attribute.java - changed source on line 873 (char index=27918) from 7 to 8 inal Type NOTATION = new Type(>>>7); /** * <p> * src/nu/xom/Attribute.java - changed source on line 884 (char index=28341) from 8 to 9 final Type ENTITY = new Type(>>>8); /** * <p> * src/nu/xom/Attribute.java - changed source on line 895 (char index=28799) from 9 to 0 inal Type ENTITIES = new Type(>>>9); /** * <p> * 54 mutations survived out of 96 changes. Score = 44 took 3 minutes ~/projects/xom$
Use a faster compiler; e.g. Jikes
Use standard optimization techniques such as inspecting the code with a profiler and eliminating redundant code in the unit tests.
Make sure you aren't mutating comments.
Make sure you aren't mutating untested code such as samples and the tests themselves.
Make sure the most expensive tests (e.g. ones that do I/O) show up later in the test suite.
Null is never passed to this method. This should be tested.
Or could we just eliminate the check; what happens if null is passed? Write a couple of tests and find out:
public void testStringValueOfNull() {
assertEquals("", StringFunction.evaluate(null, null));
}
public void testStringValueOfNullWithNonNullNavigator() {
assertEquals("", StringFunction.evaluate(null, new DocumentNavigator()));
}
Which objects are frequently passed to this method, and could those be tested for first?
Code contributed without any tests:
Goal of package was to allow XPath to be used on regular HTML.
No matter what I did I could not make a test case hit this code.
Purpose was to convert "the element, but not attribute, names of an HTML document to upper or lower case, depending on your specification, so that lower case [element] xpath expressions will work just as well on HTML as they do on XHTML (which is case sensitive and enforces lower case on elements and attributes)."
In other words, the goal is that we can now write //p and have that same expression select all the paragraph elements in both HTML and XHTML documents.
BUT THIS DOESN'T WORK!
//p never selects paragraph elements in an XHTML document. In XHTML documents, unlike HTML documents, the p element is in a namespace. Thus, when querying an XHTML document you have to write //foo:p, where the foo prefix is bound to the XHTML namespace URI. (Remember, in XPath expressions we never use a default namespace not bound to a prefix, even if the instance documents do.)
Allowing the lower case name "p" in an XPath expression to match both p and P is insufficient to allow the same XPath expression to query both HTML and XHTML documents. You still need two different XPath expressions.
If you want to query an XHTML DOM, the regular, normal Jaxen DOM navigator works just fine, because XHTML is XML. And if you want to query an HTML DOM, then the regular, normal Jaxen DOM navigator also works just fine, because DOM HTML is still a well-defined tree that follows all the DOM rules. In this tree the element names happen to be upper case. But that's OK. It's in no way a problem for Jaxen or XPath.
Test first development would have prevented this package from ever hitting the CVS tree. Test-second development at least succeeded in eliminating it.
org.jaxen.patterns
Goal of was to support XSLT match patterns
This code could be tested (and now is) but wasn't being tested. Why hadn't I noticed?
Because nothing I was doing needed it. This entire package could be eliminated from my distro to make a smaller, simpler JAR file.
From Jaxen's BaseXPath
class:
Are these methods too simple to fail?
/** Return the original expression text.
*
* @return the normalized XPath expression string
*/
public String toString()
{
return this.exprText;
}
/** Returns the string version of this xpath.
*
* @return the normalized XPath expression string
*
* @see #toString
*/
public String debug()
{
return this.xpath.toString();
}
The second one wasn't tested. Testing revealed that it didn't return what the JavaDoc said it did. In this case, the JavaDoc was incorrect and needed to be repaired.
/** Returns a string representation of the parse tree.
*
* @return a string representation of the parse tree.
*/
public String debug()
{
return this.xpath.toString();
}
Further testing revealed a problem in
the toString()
method in DefaultNameStep
that was implicitly invoked by this method.
How many times a line is executed can be surprisingly useful information
Cases that are infrequent can be moved after frequent cases
Unexecuted cases can be eliminated (or tested)
From Jaxen's SimpleNamespaceContext
class:
Does this suggest an optimization?
getParentNode
in org.jaxen.DefaultNavigator
:
It's inefficient.
Fix by overriding getParentNode
in the DOMNavigator with a much simpler version.
Plus there's a JavaDoc bug!
Program testing can be quite effective for showing the presence of bugs, but is hopelessly inadequate for showing their absence.
--Edsger W. Dijstra, 1976
This code from XOM's Canonicalizer
class
was 100% covered. However code coverage did not show that this was an infinite loop if
the loop executed more than twice:
ParentNode root = (ParentNode) node;
while (root.getParent() != null) root = node.getParent();
Also from XOM's Canonicalizer
class,
Code coverage can notice an exception is or is not thrown into a catch block. However, it can't determine if both cases reach into a finally block:
try {
write(node.query(".//. | .//@* | .//namespace::*"));
}
finally {
if (pseudoRoot != null) pseudoRoot.removeChild(0);
}
If you haven't tested a method, do you really know what it's doing?
This is not the same question as, "Is the method doing what it's supposed to be doing?"
Especially important for legacy code
This presentation: http://www.cafeconleche.org/slides/stpcon2005/codecoverage/
Cobertura: http://cobertura.sourceforge.net/
Measure test coverage with Cobertura: http://www.ibm.com/developerworks/java/library/j-cobertura/
Jester: http://jester.sourceforge.net
Test your tests with Jester: http://www.ibm.com/developerworks/library/j-jester/?ca=drs-j1105
Clover: http://www.cenqua.com/clover/