Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trim stack trace. #1028

Merged
merged 9 commits into from
Feb 6, 2017
48 changes: 6 additions & 42 deletions src/main/java/junit/runner/BaseTestRunner.java
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
package junit.runner;

import java.io.BufferedReader;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.PrintWriter;
import java.io.StringReader;
import java.io.StringWriter;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
Expand All @@ -19,6 +17,7 @@
import junit.framework.Test;
import junit.framework.TestListener;
import junit.framework.TestSuite;
import org.junit.internal.StackTraces;

/**
* Base class for all test runners.
Expand Down Expand Up @@ -264,6 +263,10 @@ public static int getPreference(String key, int dflt) {
* Returns a filtered stack trace
*/
public static String getFilteredTrace(Throwable e) {
if (!showStackRaw()) {
return StackTraces.getTrimmedStackTrace(e);
}

StringWriter stringWriter = new StringWriter();
PrintWriter writer = new PrintWriter(stringWriter);
e.printStackTrace(writer);
Expand All @@ -275,53 +278,14 @@ public static String getFilteredTrace(Throwable e) {
* Filters stack frames from internal JUnit classes
*/
public static String getFilteredTrace(String stack) {
if (showStackRaw()) {
return stack;
}

StringWriter sw = new StringWriter();
PrintWriter pw = new PrintWriter(sw);
StringReader sr = new StringReader(stack);
BufferedReader br = new BufferedReader(sr);

String line;
try {
while ((line = br.readLine()) != null) {
if (!filterLine(line)) {
pw.println(line);
}
}
} catch (Exception IOException) {
return stack; // return the stack unfiltered
}
return sw.toString();
return showStackRaw() ? stack : StackTraces.trimStackTrace(stack);
}

protected static boolean showStackRaw() {
return !getPreference("filterstack").equals("true") || fgFilterStack == false;
}

static boolean filterLine(String line) {
String[] patterns = new String[]{
"junit.framework.TestCase",
"junit.framework.TestResult",
"junit.framework.TestSuite",
"junit.framework.Assert.", // don't filter AssertionFailure
"junit.swingui.TestRunner",
"junit.awtui.TestRunner",
"junit.textui.TestRunner",
"java.lang.reflect.Method.invoke("
};
for (int i = 0; i < patterns.length; i++) {
if (line.indexOf(patterns[i]) > 0) {
return true;
}
}
return false;
}

static {
fgMaxMessageLength = getPreference("maxmessage", fgMaxMessageLength);
}

}
203 changes: 203 additions & 0 deletions src/main/java/org/junit/internal/StackTraces.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
package org.junit.internal;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.PrintWriter;
import java.io.StringReader;
import java.io.StringWriter;
import java.util.AbstractList;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

/**
* Utility class for working with stack traces.
*/
public class StackTraces {
private StackTraces() {
}

/**
* Gets a trimmed version of the stack trace of the given exception. Stack trace
* elements that are below the test method are filtered out.
*
* @return a trimmed stack trace, or the original trace if trimming wasn't possible
*/
public static String getTrimmedStackTrace(Throwable exception) {
StringWriter stringWriter = new StringWriter();
PrintWriter writer = new PrintWriter(stringWriter);
exception.printStackTrace(writer);
return trimStackTrace(exception.toString(), stringWriter.toString());
}

/**
* Trims the given stack trace. Stack trace elements that are below the test method are
* filtered. out.
*
* @param fullTrace the full stack trace
* @return a trimmed stack trace, or the original trace if trimming wasn't possible
*/
public static String trimStackTrace(String fullTrace) {
return trimStackTrace("", fullTrace);
}

static String trimStackTrace(String extracedExceptionMessage, String fullTrace) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is quite long. I think it is separated into different activities which could be "extractMethod" refactored. Example, collecting the original set of stack trace lines could be a method of its own, as could be the loop on line 71 below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashleyfrieze I tried to extract a method here, but I couldn't find a clean way to to it. Extracting a method at line 71 won't shorten the method by much, and might even hurt readability

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kcooney if you tell me how to do it, I could fork your code and show you a refactoring via pull request. In general, my rule is this - if you've put a comment on top of a block of code, then there's probably a method waiting to be extracted. So lines 71-77 look like a method to me, as do lines 50-60.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashleyfrieze I'm a fan of the Hub tool for tasks like this (see http://blog.spreedly.com/2014/06/24/merge-pull-request-considered-harmful/#). That being said, for breaking up private or back-scoped methods, I think it would be simpler for me to push this and for you to send a new pull.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kcooney just let me know where it is and I'll have a look and send you a pull request.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashleyfrieze let you know where what is? The blog post explains how to take a github pull request and get it into your local repo

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kcooney - aha! (GitHub rookie). I'll look!

StringBuilder trimmedTrace = new StringBuilder(extracedExceptionMessage);
BufferedReader reader = new BufferedReader(
new StringReader(fullTrace.substring(extracedExceptionMessage.length())));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extraced -> extracted - typo


try {
// Collect the stack trace lines for "exception" (but not the cause).
List<String> stackTraceLines = new ArrayList<String>();
String line;
boolean hasCause = false;
while ((line = reader.readLine()) != null) {
if (line.startsWith("Caused by: ")) {
hasCause= true;
break;
}
stackTraceLines.add(line);
}
if (stackTraceLines.isEmpty()) {
// No stack trace?
return fullTrace;
}
stackTraceLines = trimStackTraceLines(stackTraceLines, hasCause);
if (stackTraceLines.isEmpty()) {
// Could not trim stack trace lines.
return fullTrace;
}
appendStackTraceLines(stackTraceLines, trimmedTrace);
if (line != null) {
// Print remaining stack trace lines.
do {
trimmedTrace.append(line).append("\n");
line = reader.readLine();
} while (line != null);
}
return trimmedTrace.toString();
} catch (IOException e) {
}
return fullTrace;
}

private static void appendStackTraceLines(
List<String> stackTraceLines, StringBuilder destBuilder) {
for (String stackTraceLine : stackTraceLines) {
destBuilder.append(stackTraceLine).append("\n");
}
}

private static List<String> trimStackTraceLines(
List<String> stackTraceLines, boolean hasCause) {
State state = State.PROCESSING_OTHER_CODE;
int linesToInclude = stackTraceLines.size();
for (String stackTraceLine : asReversedList(stackTraceLines)) {
state = state.processLine(stackTraceLine);
if (state == State.DONE) {
List<String> trimmedLines = stackTraceLines.subList(0, linesToInclude);
if (!hasCause) {
return trimmedLines;
}
List<String> copy = new ArrayList<String>(trimmedLines);
copy.add("\t..." + (stackTraceLines.size() - copy.size()) + "more");
return copy;
}
linesToInclude--;
}
return Collections.emptyList();
}

private static <T> List<T> asReversedList(final List<T> list) {
return new AbstractList<T>() {

@Override
public T get(int index) {
return list.get(list.size() - index - 1);
}

@Override
public int size() {
return list.size();
}
};
}

private enum State {
PROCESSING_OTHER_CODE {
@Override public State processLine(String line) {
if (isTestFrameworkStackTraceLine(line)) {
return PROCESSING_TEST_FRAMEWORK_CODE;
}
return this;
}
},
PROCESSING_TEST_FRAMEWORK_CODE {
@Override public State processLine(String line) {
if (isReflectionStackTraceLine(line)) {
return PROCESSING_REFLECTION_CODE;
} else if (isTestFrameworkStackTraceLine(line)) {
return this;
}
return PROCESSING_OTHER_CODE;
}
},
PROCESSING_REFLECTION_CODE {
@Override public State processLine(String line) {
if (isReflectionStackTraceLine(line)) {
return this;
} else if (isTestFrameworkStackTraceLine(line)) {
// This is here to handle TestCase.runBare() calling TestCase.runTest().
return PROCESSING_TEST_FRAMEWORK_CODE;
}
return DONE;
}
},
DONE {
@Override public State processLine(String line) {
return this;
}
};

/** Processes a stack trace line, possibly moving to a new state. */
public abstract State processLine(String line);
}

private static final String[] TEST_FRAMEWORK_METHOD_NAME_PREFIXES = {
"org.junit.runner.",
"org.junit.runners.",
"org.junit.experimental.runners.",
"org.junit.internal.",
"junit.",
"org.apache.maven.surefire."

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If including Maven, should it also include Gradle? http://gradle.org/docs/current/dsl/org.gradle.api.tasks.testing.Test.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashleyfrieze Good idea. Would you please send me an example stack trace from a failing JUnit test running on Gradle? I can add test cases for Gradle and SureFire.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the output from a Gradle test failure. I don't use Gradle myself, so I asked a colleague.

<?xml version="1.0" encoding="UTF-8"?>
<testsuite name="com.example.ImmutableClassTest" tests="2" skipped="0" failures="1" errors="0" timestamp="2015-05-07T16:27:39" hostname="retinahandle.local" time="0.164">
  <properties/>
  <testcase name="how_I_think_it_should_work" classname="com.example.ImmutableClassTest" time="0.163">
    <failure message="org.mutabilitydetector.unittesting.MutabilityAssertionError: &#10;Expected: com.example.ImmutableClass to be IMMUTABLE&#10;     but: com.example.ImmutableClass is actually NOT_IMMUTABLE&#10;    Reasons:&#10;        Field can have a mutable type (java.util.Optional) assigned to it. [Field: foo, Class: com.example.ImmutableClass]&#10;        Field can have a mutable type (java.util.Optional) assigned to it. [Field: bar, Class: com.example.ImmutableClass]&#10;    Allowed reasons:&#10;        Field can have collection with mutable element type (java.util.Optional&lt;com.example.ImmutableClass$Bar&gt;) assigned to it. [Field: bar, Class: com.example.ImmutableClass]&#10;" type="org.mutabilitydetector.unittesting.MutabilityAssertionError">org.mutabilitydetector.unittesting.MutabilityAssertionError: 
Expected: com.example.ImmutableClass to be IMMUTABLE
     but: com.example.ImmutableClass is actually NOT_IMMUTABLE
    Reasons:
        Field can have a mutable type (java.util.Optional) assigned to it. [Field: foo, Class: com.example.ImmutableClass]
        Field can have a mutable type (java.util.Optional) assigned to it. [Field: bar, Class: com.example.ImmutableClass]
    Allowed reasons:
        Field can have collection with mutable element type (java.util.Optional&lt;com.example.ImmutableClass$Bar&gt;) assigned to it. [Field: bar, Class: com.example.ImmutableClass]

    at org.mutabilitydetector.unittesting.internal.AssertionReporter.assertThat(AssertionReporter.java:48)
    at org.mutabilitydetector.unittesting.MutabilityAsserter.assertInstancesOf(MutabilityAsserter.java:125)
    at org.mutabilitydetector.unittesting.MutabilityAssert.assertInstancesOf(MutabilityAssert.java:741)
    at com.example.ImmutableClassTest.how_I_think_it_should_work(ImmutableClassTest.java:24)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
    at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecuter.runTestClass(JUnitTestClassExecuter.java:86)
    at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecuter.execute(JUnitTestClassExecuter.java:49)
    at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassProcessor.processTestClass(JUnitTestClassProcessor.java:64)
    at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:50)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.gradle.messaging.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:35)
    at org.gradle.messaging.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
    at org.gradle.messaging.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:32)
    at org.gradle.messaging.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:93)
    at com.sun.proxy.$Proxy2.processTestClass(Unknown Source)
    at org.gradle.api.internal.tasks.testing.worker.TestWorker.processTestClass(TestWorker.java:106)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.gradle.messaging.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:35)
    at org.gradle.messaging.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
    at org.gradle.messaging.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:360)
    at org.gradle.internal.concurrent.DefaultExecutorFactory$StoppableExecutorImpl$1.run(DefaultExecutorFactory.java:64)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)
</failure>
  </testcase>
  <testcase name="this_works_now" classname="com.example.ImmutableClassTest" time="0.001"/>
  <system-out><![CDATA[]]></system-out>
  <system-err><![CDATA[]]></system-err>
</testsuite>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; I'll try to find time to add this to the test cases this weekend. It's possible that Gradle doesn't use the code I'm changing when it prints out test failures or produces test XML.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out, I don't need to special case Maven Surefire or Gradle. I added testcases to verify that

};

private static boolean isTestFrameworkStackTraceLine(String line) {
return isMatchingStackTraceLine(line, TEST_FRAMEWORK_METHOD_NAME_PREFIXES);
}

private static final String[] REFLECTION_METHOD_NAME_PREFIXES = {
"sun.reflect.",
"java.lang.reflect.",
"org.junit.rules.RunRules.evaluate(", // get better stack traces for failures in method rules
"junit.framework.TestCase.runBare(", // runBare() directly calls setUp() and tearDown()
};

private static boolean isReflectionStackTraceLine(String line) {
return isMatchingStackTraceLine(line, REFLECTION_METHOD_NAME_PREFIXES);
}

private static boolean isMatchingStackTraceLine(String line, String[] packagePrefixes) {
if (!line.startsWith("\tat ")) {
return false;
}
line = line.substring(4);
for (String packagePrefix : packagePrefixes) {
if (line.startsWith(packagePrefix)) {
return true;
}
}

return false;
}
}
2 changes: 1 addition & 1 deletion src/main/java/org/junit/internal/TextListener.java
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ protected void printFailures(Result result) {

protected void printFailure(Failure each, String prefix) {
getWriter().println(prefix + ") " + each.getTestHeader());
getWriter().print(each.getTrace());
getWriter().print(each.getTrimmedTrace());
}

protected void printFooter(Result result) {
Expand Down
14 changes: 11 additions & 3 deletions src/main/java/org/junit/runner/notification/Failure.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import java.io.Serializable;
import java.io.StringWriter;

import org.junit.internal.StackTraces;
import org.junit.runner.Description;

/**
Expand Down Expand Up @@ -65,9 +66,7 @@ public String toString() {
}

/**
* Convenience method
*
* @return the printed form of the exception
* Gets the printed form of the exception and its stack trace.
*/
public String getTrace() {
StringWriter stringWriter = new StringWriter();
Expand All @@ -76,6 +75,15 @@ public String getTrace() {
return stringWriter.toString();
}

/**
* Gets a the printed form of the exception, with a trimmed version of the stack trace.
* This method will attempt to filter out frames of the stack trace that are below
* the test method call.
*/
public String getTrimmedTrace() {
return StackTraces.getTrimmedStackTrace(getException());
}

/**
* Convenience method
*
Expand Down
37 changes: 20 additions & 17 deletions src/test/java/junit/tests/runner/StackFilterTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,28 +15,31 @@ protected void setUp() {
StringWriter swin = new StringWriter();
PrintWriter pwin = new PrintWriter(swin);
pwin.println("junit.framework.AssertionFailedError");
pwin.println(" at junit.framework.Assert.fail(Assert.java:144)");
pwin.println(" at junit.framework.Assert.assert(Assert.java:19)");
pwin.println(" at junit.framework.Assert.assert(Assert.java:26)");
pwin.println(" at MyTest.f(MyTest.java:13)");
pwin.println(" at MyTest.testStackTrace(MyTest.java:8)");
pwin.println(" at java.lang.reflect.Method.invoke(Native Method)");
pwin.println(" at junit.framework.TestCase.runTest(TestCase.java:156)");
pwin.println(" at junit.framework.TestCase.runBare(TestCase.java:130)");
pwin.println(" at junit.framework.TestResult$1.protect(TestResult.java:100)");
pwin.println(" at junit.framework.TestResult.runProtected(TestResult.java:118)");
pwin.println(" at junit.framework.TestResult.run(TestResult.java:103)");
pwin.println(" at junit.framework.TestCase.run(TestCase.java:121)");
pwin.println(" at junit.framework.TestSuite.runTest(TestSuite.java:157)");
pwin.println(" at junit.framework.TestSuite.run(TestSuite.java, Compiled Code)");
pwin.println(" at junit.swingui.TestRunner$17.run(TestRunner.java:669)");
pwin.println("\tat junit.framework.Assert.fail(Assert.java:144)");
pwin.println("\tat junit.framework.Assert.assert(Assert.java:19)");
pwin.println("\tat junit.framework.Assert.assert(Assert.java:26)");
pwin.println("\tat MyTest.f(MyTest.java:13)");
pwin.println("\tat MyTest.testStackTrace(MyTest.java:8)");
pwin.println("\tat java.lang.reflect.Method.invoke(Native Method)");
pwin.println("\tat junit.framework.TestCase.runTest(TestCase.java:156)");
pwin.println("\tat junit.framework.TestCase.runBare(TestCase.java:130)");
pwin.println("\tat junit.framework.TestResult$1.protect(TestResult.java:100)");
pwin.println("\tat junit.framework.TestResult.runProtected(TestResult.java:118)");
pwin.println("\tat junit.framework.TestResult.run(TestResult.java:103)");
pwin.println("\tat junit.framework.TestCase.run(TestCase.java:121)");
pwin.println("\tat junit.framework.TestSuite.runTest(TestSuite.java:157)");
pwin.println("\tat junit.framework.TestSuite.run(TestSuite.java, Compiled Code)");
pwin.println("\tat junit.swingui.TestRunner$17.run(TestRunner.java:669)");
fUnfiltered = swin.toString();

StringWriter swout = new StringWriter();
PrintWriter pwout = new PrintWriter(swout);
pwout.println("junit.framework.AssertionFailedError");
pwout.println(" at MyTest.f(MyTest.java:13)");
pwout.println(" at MyTest.testStackTrace(MyTest.java:8)");
pwout.println("\tat junit.framework.Assert.fail(Assert.java:144)");
pwout.println("\tat junit.framework.Assert.assert(Assert.java:19)");
pwout.println("\tat junit.framework.Assert.assert(Assert.java:26)");
pwout.println("\tat MyTest.f(MyTest.java:13)");
pwout.println("\tat MyTest.testStackTrace(MyTest.java:8)");
fFiltered = swout.toString();
}

Expand Down
Loading