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

Remove matcher frames from stacktraces #369

Merged
merged 2 commits into from
Jun 26, 2018

Conversation

ajalt
Copy link
Contributor

@ajalt ajalt commented Jun 25, 2018

Currently when an assertion fails, the error's stack trace starts witha number of frames of internal kotlintest functions. #366 makes this even worse by adding several frames of reflection calls.

User's just want to see the line in their test that caused the failure, so this PR removes kotlintest frames off of the top of the stack in the AssertionErrors that it creates. This puts the user's code at the top of the stack. It does not remove frames associated with the kotlintest runner, since these occur below user code.

Current behavior:

org.opentest4j.AssertionFailedError: expected: true but was: false
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
    at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
    at io.kotlintest.DslKt.callPublicConstructor(dsl.kt:184)
    at io.kotlintest.DslKt.junit5assertionFailedError(dsl.kt:161)
    at io.kotlintest.DslKt.equalsError(dsl.kt:148)
    at io.kotlintest.DslKt.shouldBe(dsl.kt:49)
    at com.example.MyTest.myTest(MyTest.kt:21)
    at io.kotlintest.runner.jvm.TestCaseExecutor$executeTestSet$1.run(TestCaseExecutor.kt:96)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
    at java.lang.Thread.run(Thread.java:748)

Behavior with this PR:

org.opentest4j.AssertionFailedError: expected: true but was: false
    at com.example.MyTest.myTest(MyTest.kt:21)
    at io.kotlintest.runner.jvm.TestCaseExecutor$executeTestSet$1.run(TestCaseExecutor.kt:96)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
    at java.lang.Thread.run(Thread.java:748)

Users can turn this behavior off and see the full stack trace by setting Failures.shouldRemoveKotlintestElementsFromStacktrace = false.

Currently when an assertion fails, the error's stack trace starts witha number of frames of internal kotlintest functions. kotest#366 makes this even worse by adding several frames of reflection calls.

User's just want to see the line in their test that caused the failure, so this PR removes kotlintest frames off of the top of the stack in the `AssertionErrors` that it creates. This puts the user's code at the top of the stack. It does not remove frames associated with the kotlintest runner, since these occur below user code.

Current behavior:

```
org.opentest4j.AssertionFailedError: expected: true but was: false
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
    at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
    at io.kotlintest.DslKt.callPublicConstructor(dsl.kt:184)
    at io.kotlintest.DslKt.junit5assertionFailedError(dsl.kt:161)
    at io.kotlintest.DslKt.equalsError(dsl.kt:148)
    at io.kotlintest.DslKt.shouldBe(dsl.kt:49)
    at com.example.MyTest.myTest(MyTest.kt:21)
    at io.kotlintest.runner.jvm.TestCaseExecutor$executeTestSet$1.run(TestCaseExecutor.kt:96)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
    at java.lang.Thread.run(Thread.java:748)
```

Behavior with this PR:

```
org.opentest4j.AssertionFailedError: expected: true but was: false
    at com.example.MyTest.myTest(MyTest.kt:21)
    at io.kotlintest.runner.jvm.TestCaseExecutor$executeTestSet$1.run(TestCaseExecutor.kt:96)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
    at java.lang.Thread.run(Thread.java:748)
```
Users can turn this behavior off and see the full stack trace by setting `Failures.shouldRemoveKotlintestElementsFromStacktrace = false`.
@sksamuel
Copy link
Member

More nice work. The only comment would be to have the flag to disable this as part of the Project class, which will read from either a system property kotlintest.failure.stack.truncated=false or something in addition to a var inside the ProjectConfig.

@ajalt
Copy link
Contributor Author

ajalt commented Jun 25, 2018

That makes sense, but Failures is part of kotlintest-assertions, and ProjectConfig is part of kotlintest-core, and neither depends on the other.

@sksamuel
Copy link
Member

Yes that's a good point :)
I would like to see it also available via a system property in addition so it is consistent with the core module.
Do you not only use kotlintest-assertions out of interest?

@ajalt
Copy link
Contributor Author

ajalt commented Jun 26, 2018

Ok, I added a system property. Feel free to suggest a better property name.

I frequently use kotlintest-assertions with JUnit4 as the test runner. I work with a lot of code bases that have their tests written with Junit4 and hamcrest or assertJ. It's straightforward to swap out the assertions libraries; it's a lot more work to change the test runner. I also use kotlintest-assertions on Android codebases, where the kotlintest runner isn't an option.

@sksamuel sksamuel merged commit bb6fdcc into kotest:master Jun 26, 2018
@sksamuel
Copy link
Member

Lovely.

And you use kotlintest-assertions because you prefer it over say hamcrest or the other kotlin options, for what reason? Just curious as to what you like about it.

@ajalt ajalt deleted the clean-stacktrace branch June 26, 2018 16:22
@ajalt
Copy link
Contributor Author

ajalt commented Jun 26, 2018

Hamcrest is hard to type, since there's no auto complete and lots of conflicting imports, and hard to read, since the function calls are awkward. AssertJ is my choice for java, but it's still noisy.

Compare:

assertThat(foo, is(equalTo(bar)))
assertThat(foo).isEqualTo(bar)
foo shouldBe bar

kotlintest-assertions also has some features that I use a lot that aren't present in the java libraries: table testing, shouldThrow, etc. When I was using assertj in kotlin, I would end up writing my own version of those features in every project anyway.

@sksamuel
Copy link
Member

Good to know, thanks for the feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants