Skip to content

Commit

Permalink
prioritize exceptions thrown directly in soft assertion blocks
Browse files Browse the repository at this point in the history
If you had
```kotlin
assertAll {
  assertThat(1).isEqualTo(2)
  throw IllegalStateException("Broken!")
}
```
the `IllegalStateException` would be swallowed. It'll now be the primary
exception you see. This more closely matches the mental model where soft
assertions are thrown at the end of the block.

For convenience the assertion failure is added as a suppressed
exception. Kotlin has mixed support for this (jvm shows the suppressed
exception trace and error message, native just shows the trace, and js
doesn't show it at all). This is acceptable as the inline exception is
likely the one you care about fixing anyway.

Fixes #475
  • Loading branch information
evant committed Dec 5, 2023
1 parent 9470bb6 commit 9e0e6ef
Show file tree
Hide file tree
Showing 8 changed files with 77 additions and 28 deletions.
23 changes: 19 additions & 4 deletions assertk/src/commonMain/kotlin/assertk/failure.kt
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,31 @@ internal interface Failure {
* Run the given block of assertions with its Failure.
*/
@PublishedApi
@Suppress("TooGenericExceptionCaught", "ThrowingExceptionFromFinally")
internal inline fun <F: Failure, T> F.run(f: F.() -> T): T {
pushFailure()
var otherException: Throwable? = null
try {
return f()
} catch (e: Throwable) {
if (e.isOutOfMemory()) {
throw e
}
otherException = e
} finally {
popFailure()
invoke()
if (otherException != null) {
try {
invoke()
} catch (e: Throwable) {
otherException.addSuppressed(e)
}
throw otherException
} else {
invoke()
}
}
throw UnsupportedOperationException("unreachable!")
}

/**
Expand Down Expand Up @@ -171,9 +188,7 @@ fun notifyFailure(e: Throwable) {
FailureContext.fail(e)
}

@Suppress("EXTENSION_SHADOWED_BY_MEMBER")
internal expect inline fun Throwable.addSuppressed(error: Throwable)

@PublishedApi
internal expect inline fun Throwable.isOutOfMemory(): Boolean

internal expect inline fun failWithNotInStacktrace(error: Throwable): Nothing
Expand Down
35 changes: 32 additions & 3 deletions assertk/src/commonTest/kotlin/test/assertk/AssertAllTest.kt
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
package test.assertk

import assertk.*
import assertk.assertions.*
import assertk.all
import assertk.assertAll
import assertk.assertThat
import assertk.assertions.endsWith
import assertk.assertions.isEqualTo
import assertk.assertions.isFailure
import assertk.assertions.isSuccess
import assertk.assertions.startsWith
import assertk.assertions.support.show
import assertk.fail
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFailsWith
import kotlin.test.assertIs
import kotlin.test.assertTrue

class AssertAllTest {
Expand Down Expand Up @@ -45,6 +53,16 @@ class AssertAllTest {
error.message!!.lines()
)
}

@Test fun all_prioritizes_exceptions_thrown_in_block_over_soft_assertions() {
val error = assertFailsWith<IllegalStateException> {
assertThat(1).all {
isEqualTo(2)
throw IllegalStateException("Test")
}
}
assertEquals("Test", error.message)
}
//endregion

//region assertAll
Expand Down Expand Up @@ -144,6 +162,17 @@ class AssertAllTest {
assertTrue(error.message!!.contains("\t${opentestPackageName}AssertionFailedError: expected success but was failure:${show(Exception("error1"))}"))
assertTrue(error.message!!.contains("\t${opentestPackageName}AssertionFailedError: expected success but was failure:${show(Exception("error2"))}"))
}
//endregion

@Test
fun assertAll_prioritizes_exceptions_thrown_in_block_over_soft_assertions() {
val error = assertFailsWith<IllegalStateException> {
assertAll {
assertThat(1).isEqualTo(2)
throw IllegalStateException("Test")
}
}
assertEquals("Test", error.message)
assertIs<AssertionError>(error.suppressedExceptions.first())
}
//endregion
}
5 changes: 1 addition & 4 deletions assertk/src/jsMain/kotlin/assertk/failure.kt
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,5 @@ internal actual inline fun failWithNotInStacktrace(error: Throwable): Nothing {
throw error
}

internal actual inline fun Throwable.addSuppressed(error: Throwable) {
// ignore
}

@PublishedApi
internal actual inline fun Throwable.isOutOfMemory(): Boolean = false
6 changes: 1 addition & 5 deletions assertk/src/jvmMain/kotlin/assertk/failure.kt
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,5 @@ internal actual inline fun failWithNotInStacktrace(error: Throwable): Nothing {
throw error
}

@Suppress("EXTENSION_SHADOWED_BY_MEMBER")
internal actual inline fun Throwable.addSuppressed(error: Throwable) {
throw NotImplementedError()
}

@PublishedApi
internal actual inline fun Throwable.isOutOfMemory(): Boolean = this is OutOfMemoryError
14 changes: 11 additions & 3 deletions assertk/src/jvmTest/kotlin/test/assertk/JVMAssertAllTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import kotlin.test.assertFailsWith
import kotlin.test.assertFalse

class JVMAssertAllTest {
@Test fun assert_all_is_thread_safe() {
@Test fun assertAll_is_thread_safe() {
runOnMultipleThreads {
assertAll {
assertThat("one").isEqualTo("one")
Expand All @@ -30,7 +30,7 @@ class JVMAssertAllTest {
}
}

@Test fun assert_all_includes_exceptions_as_suppressed() {
@Test fun assertAll_includes_exceptions_as_suppressed() {
val error = assertFailsWith<AssertionError> {
assertAll {
assertThat(1).isEqualTo(2)
Expand All @@ -42,7 +42,15 @@ class JVMAssertAllTest {
assertEquals("expected:<[1]> but was:<[2]>", error.suppressed[1].message)
}

@Test fun assert_all_does_not_catch_out_of_memory_errors() {
@Test fun assertAll_does_not_catch_out_of_memory_errors() {
assertFailsWith<OutOfMemoryError> {
assertAll {
throw OutOfMemoryError()
}
}
}

@Test fun assertAll_does_not_catch_out_of_memory_errors_in_nested_assert() {
var runs = false
assertFailsWith<OutOfMemoryError> {
assertAll {
Expand Down
5 changes: 1 addition & 4 deletions assertk/src/nativeMain/kotlin/assertk/failure.kt
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,5 @@ internal actual inline fun failWithNotInStacktrace(error: Throwable): Nothing {
throw error
}

internal actual inline fun Throwable.addSuppressed(error: Throwable) {
// ignore
}

@PublishedApi
internal actual inline fun Throwable.isOutOfMemory(): Boolean = this is OutOfMemoryError
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ import assertk.assertions.doesNotContain
import assertk.assertions.isEqualTo
import kotlin.native.concurrent.Worker
import kotlin.native.concurrent.Future
import kotlin.native.concurrent.ObsoleteWorkersApi
import kotlin.native.concurrent.TransferMode
import kotlin.test.*

@ObsoleteWorkersApi
class NativeAssertAllTest {

@Test fun assert_all_is_thread_safe() {
Expand All @@ -35,7 +37,15 @@ class NativeAssertAllTest {
}
}

@Test fun assert_all_does_not_catch_out_of_memory_errors() {
@Test fun assertAll_does_not_catch_out_of_memory_errors() {
assertFailsWith<OutOfMemoryError> {
assertAll {
throw OutOfMemoryError()
}
}
}

@Test fun assertAll_does_not_catch_out_of_memory_errors_in_nested_assert() {
var runs = false
assertFailsWith<OutOfMemoryError> {
assertAll {
Expand Down
5 changes: 1 addition & 4 deletions assertk/src/wasmJsMain/kotlin/assertk/failure.kt
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,5 @@ internal actual inline fun failWithNotInStacktrace(error: Throwable): Nothing {
throw error
}

internal actual inline fun Throwable.addSuppressed(error: Throwable) {
// ignore
}

@PublishedApi
internal actual inline fun Throwable.isOutOfMemory(): Boolean = false

0 comments on commit 9e0e6ef

Please sign in to comment.