Improves Failures implementation #465

LeoColman · 2018-11-07T15:29:59Z

As it was, the Failures class would fail to filter classes that are neither from sun.reflect.* nor from java.*. This commit aims to create a generic implementation that correctly filters stacktraces.

The implemented behavior observes a pattern to clean the stacktrace:

There may be elements that are not from Kotlintest and are not from User Classes, such as Reflection elements or Java elements (but may be others). These elements are removed.
Then the code tries to find elements that are indeed from Kotlintest, such as runners and other internals
After this, it's expected for user classes to be in the stacktrace, so the algorithm stops and returns the stacktrace

By observation of the FailuresTest class, one may believe that only classes from io.kotlintest and sun.reflect would be in the way of user classes, but that's not the case, and this commit fixes it.

Very related to this commit

As it was, the Failures class would fail to filter classes that are neither from `sun.reflect.*` nor from `java.*`. This commit aims to create a generic implementation that correctly filters stacktraces. The implemented behavior observes a pattern to clean the stacktrace: - There may be elements that are not from Kotlintest and are not from User Classes, such as Reflection elements or Java elements (but may be others). These elements are removed. - Then the code tries to find elements that are indeed from Kotlintest, such as runners and other internals - After this, it's expected for user classes to be in the stacktrace, so the algorithm stops and returns the stacktrace By observation of the FailuresTest class, one may believe that only classes from `io.kotlintest` and `sun.reflect` would be in the way of user classes, but that's not the case, and this commit fixes it.

LeoColman · 2018-11-07T15:30:30Z

@ajalt
Requested your review because you originally implemented the Failures algorithm. Would you mind taking a look?

sksamuel · 2018-11-07T15:38:27Z

Some context here - the reason for my change overnight was because when you have threads > 1, we execute these on a thread pool, and if threads == 1 we execute in the main thread. So the stack trace can vary. It won't always be the case that the "last" io.kotlintest stack element is directly before the user's own code.

LeoColman · 2018-11-07T15:53:23Z

That's correct.
However, as I see, it won't always be sun.reflect or java.*, so I changed it to a way to find the user classes every time

ajalt

Do you have examples of stacktraces that were being filtered incorrectly before? We should add tests for them.

kotlintest-assertions/src/main/kotlin/io/kotlintest/Failures.kt

LeoColman · 2018-11-12T12:30:25Z

@ajalt
The FailuresTest itself started failing when we made the change in the aforementioned commit. It was failing with the stacktrace:

java.lang.AssertionError: msg
	at io.kotlintest.runner.jvm.TestSetExecutor.execute(TestSetExecutor.kt:40)
	at io.kotlintest.runner.jvm.TestCaseExecutor.executeTestIfActive(TestCaseExecutor.kt:69)
	at io.kotlintest.runner.jvm.TestCaseExecutor.access$executeTestIfActive(TestCaseExecutor.kt:14)
	at io.kotlintest.runner.jvm.TestCaseExecutor$execute$3.invoke(TestCaseExecutor.kt:43)
	at io.kotlintest.runner.jvm.TestCaseExecutor.execute(TestCaseExecutor.kt:53)
	at io.kotlintest.runner.jvm.spec.SharedInstanceSpecRunner$execute$1.invoke(SharedInstanceSpecRunner.kt:16)
	at io.kotlintest.runner.jvm.spec.SharedInstanceSpecRunner$execute$1.invoke(SharedInstanceSpecRunner.kt:10)
	at io.kotlintest.runner.jvm.spec.SpecRunner$interceptSpec$chain$1.invoke(SpecRunner.kt:37)
	at io.kotlintest.runner.jvm.spec.SpecRunner$interceptSpec$chain$1.invoke(SpecRunner.kt:12)
	at io.kotlintest.runner.jvm.spec.SpecRunner.interceptSpec(SpecRunner.kt:38)
	at io.kotlintest.runner.jvm.spec.SharedInstanceSpecRunner.execute(SharedInstanceSpecRunner.kt:13)
	at io.kotlintest.runner.jvm.TestEngine.executeSpec(TestEngine.kt:118)
	at io.kotlintest.runner.jvm.TestEngine.access$executeSpec(TestEngine.kt:20)
	at io.kotlintest.runner.jvm.TestEngine$submitSpec$1.run(TestEngine.kt:97)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	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)
Caused by: java.lang.RuntimeException
	at com.sksamuel.kotlintest.FailuresTest$1$1$1.invoke(FailuresTest.kt:15)
	at com.sksamuel.kotlintest.FailuresTest$1$1$1.invoke(FailuresTest.kt:10)
	at io.kotlintest.runner.jvm.TestSetExecutor.execute(TestSetExecutor.kt:40)
	at io.kotlintest.runner.jvm.TestCaseExecutor.executeTestIfActive(TestCaseExecutor.kt:69)
	at io.kotlintest.runner.jvm.TestCaseExecutor.access$executeTestIfActive(TestCaseExecutor.kt:14)
	at io.kotlintest.runner.jvm.TestCaseExecutor$execute$3.invoke(TestCaseExecutor.kt:43)
	at io.kotlintest.runner.jvm.TestCaseExecutor.execute(TestCaseExecutor.kt:53)
	at io.kotlintest.runner.jvm.spec.SharedInstanceSpecRunner$callingThreadContext$1.registerTestCase(SharedInstanceSpecRunner.kt:31)
	at io.kotlintest.TestContext.registerTestCase(TestContext.kt:41)
	at io.kotlintest.specs.AbstractFreeSpec$FreeSpecScope.invoke(AbstractFreeSpec.kt:47)
	at com.sksamuel.kotlintest.FailuresTest$1$1.invoke(FailuresTest.kt:14)
	at com.sksamuel.kotlintest.FailuresTest$1$1.invoke(FailuresTest.kt:10)
	at io.kotlintest.specs.AbstractFreeSpec$minus$1.invoke(AbstractFreeSpec.kt:18)
	at io.kotlintest.specs.AbstractFreeSpec$minus$1.invoke(AbstractFreeSpec.kt:11)
	... 19 more

With the fix the stacktrace is correct

… of val Users may want to change this property dynamically, at runtime, for whichever reason. Changing it to a `var` won't impact anything else, so it's good to give users this freedom.

sksamuel · 2018-11-12T12:59:49Z

@Kerooker @ajalt Why would it be a var ?

It's detected from the system properties so is still settable.
Or are you expecting people to do Failures.shouldRemoveKotlintestElementsFromStacktrace = false in code?

If so, this is better off being part of ProjectConfig which contains all the other "settings" that uses can override ?

LeoColman · 2018-11-12T13:05:02Z

From what @ajalt said, I expected users to use Failures.shouldRemoveKotlintestElementsFromStacktrace = false. However, I cannot picture anyone actually doing this. I can revert the commit if necessary

sksamuel · 2018-11-12T13:14:50Z

Having the programmatic version is useful, but I think it should be controlled from ProjectConfig that's all. That's where all the other settings are held.

ajalt · 2018-11-12T16:37:13Z

We had the same discussion when I implemented this. Quoting from that PR:

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

LeoColman · 2018-11-12T16:42:30Z

Makes sense to me to keep them separated. But my concern is another one: Why is this customizable?
Do we have any user that alters this behavior at runtime?

Do we have any use for displaying Kotlintest internal stacktraces ever? I mean as an user. As a KT developer it may be interesting, but as an user I don't see use for this. What do you guys think?

ajalt · 2018-11-12T16:45:20Z

Kotlintest is generally a very customizable framework. There isn't any cost to allowing this functionality to be configured, so why not? I'm sure that someone will want to turn off the magic at some point to help them debug.

sksamuel · 2018-11-12T17:08:37Z

We had the same discussion when I implemented this. Quoting from that PR:

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

Yes of course. Sorry I've blinked since we had that convo 😨

sksamuel · 2018-11-12T17:09:03Z

Kotlintest is generally a very customizable framework. There isn't any cost to allowing this functionality to be configured, so why not? I'm sure that someone will want to turn off the magic at some point to help them debug.

Agreed. Val back to a var and I think we're good.

LeoColman · 2018-11-12T21:14:00Z

Alright. Can we merge this then?

sksamuel · 2018-11-12T22:44:35Z

I'm happy.

* Improves Failures implementation As it was, the Failures class would fail to filter classes that are neither from `sun.reflect.*` nor from `java.*`. This commit aims to create a generic implementation that correctly filters stacktraces. The implemented behavior observes a pattern to clean the stacktrace: - There may be elements that are not from Kotlintest and are not from User Classes, such as Reflection elements or Java elements (but may be others). These elements are removed. - Then the code tries to find elements that are indeed from Kotlintest, such as runners and other internals - After this, it's expected for user classes to be in the stacktrace, so the algorithm stops and returns the stacktrace By observation of the FailuresTest class, one may believe that only classes from `io.kotlintest` and `sun.reflect` would be in the way of user classes, but that's not the case, and this commit fixes it. * Changes `shouldRemoveKotlintestElementsFromStacktrace` to var instead of val Users may want to change this property dynamically, at runtime, for whichever reason. Changing it to a `var` won't impact anything else, so it's good to give users this freedom. (cherry picked from commit 7b17e62)

LeoColman requested review from sksamuel and ajalt November 7, 2018 15:30

ajalt requested changes Nov 7, 2018

View reviewed changes

kotlintest-assertions/src/main/kotlin/io/kotlintest/Failures.kt Outdated Show resolved Hide resolved

Changes shouldRemoveKotlintestElementsFromStacktrace to var instead…

abd9b10

… of val Users may want to change this property dynamically, at runtime, for whichever reason. Changing it to a `var` won't impact anything else, so it's good to give users this freedom.

LeoColman requested a review from ajalt November 12, 2018 13:08

ajalt approved these changes Nov 12, 2018

View reviewed changes

LeoColman merged commit 7b17e62 into master Nov 13, 2018

LeoColman deleted the kerooker/improve-failures-stacktrace-cleanse branch November 13, 2018 15:23

LeoColman mentioned this pull request Nov 29, 2018

3.1.11 bugfix and small features release #489

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves Failures implementation #465

Improves Failures implementation #465

LeoColman commented Nov 7, 2018

LeoColman commented Nov 7, 2018

sksamuel commented Nov 7, 2018

LeoColman commented Nov 7, 2018

ajalt left a comment •

edited

Loading

LeoColman commented Nov 12, 2018

sksamuel commented Nov 12, 2018

LeoColman commented Nov 12, 2018

sksamuel commented Nov 12, 2018

ajalt commented Nov 12, 2018

LeoColman commented Nov 12, 2018

ajalt commented Nov 12, 2018 •

edited

Loading

sksamuel commented Nov 12, 2018

sksamuel commented Nov 12, 2018

LeoColman commented Nov 12, 2018

sksamuel commented Nov 12, 2018

Improves Failures implementation #465

Improves Failures implementation #465

Conversation

LeoColman commented Nov 7, 2018

LeoColman commented Nov 7, 2018

sksamuel commented Nov 7, 2018

LeoColman commented Nov 7, 2018

ajalt left a comment • edited Loading

Choose a reason for hiding this comment

LeoColman commented Nov 12, 2018

sksamuel commented Nov 12, 2018

LeoColman commented Nov 12, 2018

sksamuel commented Nov 12, 2018

ajalt commented Nov 12, 2018

LeoColman commented Nov 12, 2018

ajalt commented Nov 12, 2018 • edited Loading

sksamuel commented Nov 12, 2018

sksamuel commented Nov 12, 2018

LeoColman commented Nov 12, 2018

sksamuel commented Nov 12, 2018

ajalt left a comment •

edited

Loading

ajalt commented Nov 12, 2018 •

edited

Loading