Skip to content

Relax Kotlin contracts for Executable parameters #4481

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Relax Kotlin contracts for Executable parameters #4481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6669b93
Relax Kotlin contracts for `Executable` parameters
marcphilipp Apr 14, 2025
ab26f26
Treat Kotlin compiler warnings as errors again
marcphilipp Apr 14, 2025
5cac4e3
Improve test as suggested
marcphilipp May 15, 2025
0646eba
Improve `assertDoesNotThrow` implementation to allow stricter contract
marcphilipp May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions junit-jupiter-api/junit-jupiter-api.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@ dependencies {
}

tasks {
compileKotlin {
// https://github.com/junit-team/junit5/issues/4371
compilerOptions.allWarningsAsErrors = false
}
jar {
bundle {
val version = project.version
Expand Down
5 changes: 4 additions & 1 deletion junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertDoesNotThrow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@

package org.junit.jupiter.api;

import static org.apiguardian.api.API.Status.INTERNAL;
import static org.junit.jupiter.api.AssertionFailureBuilder.assertionFailure;

import java.util.function.Supplier;

import org.apiguardian.api.API;
import org.junit.jupiter.api.function.Executable;
import org.junit.jupiter.api.function.ThrowingSupplier;
import org.junit.platform.commons.util.StringUtils;
Expand Down Expand Up @@ -76,7 +78,8 @@ private static <T> T assertDoesNotThrow(ThrowingSupplier<T> supplier, Object mes
}
}

private static AssertionFailedError createAssertionFailedError(Object messageOrSupplier, Throwable t) {
@API(status = INTERNAL, since = "6.0")
public static AssertionFailedError createAssertionFailedError(Object messageOrSupplier, Throwable t) {
return assertionFailure() //
.message(messageOrSupplier) //
.reason("Unexpected exception thrown: " + t.getClass().getName() + buildSuffix(t.getMessage())) //
Expand Down
40 changes: 16 additions & 24 deletions junit-jupiter-api/src/main/kotlin/org/junit/jupiter/api/Assertions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import org.apiguardian.api.API
import org.apiguardian.api.API.Status.EXPERIMENTAL
import org.apiguardian.api.API.Status.STABLE
import org.junit.jupiter.api.function.Executable
import org.junit.jupiter.api.function.ThrowingSupplier
import org.junit.platform.commons.util.UnrecoverableExceptions.rethrowIfUnrecoverable
import java.time.Duration
import java.util.stream.Stream
import kotlin.contracts.ExperimentalContracts
Expand Down Expand Up @@ -354,7 +354,12 @@ inline fun <R> assertDoesNotThrow(executable: () -> R): R {
callsInPlace(executable, EXACTLY_ONCE)
}

return Assertions.assertDoesNotThrow(evaluateAndWrap(executable))
try {
return executable()
} catch (t: Throwable) {
rethrowIfUnrecoverable(t)
throw AssertDoesNotThrow.createAssertionFailedError(null, t)
}
}

/**
Expand All @@ -374,7 +379,7 @@ inline fun <R> assertDoesNotThrow(
executable: () -> R
): R {
contract {
callsInPlace(executable, EXACTLY_ONCE)
Copy link
Contributor

Choose a reason for hiding this comment

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

This executable should be called EXACTLY_ONCE, as other assertDoesNotThrow methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Addressed in ef78d82 (#4551)

callsInPlace(executable, AT_MOST_ONCE)
}

return assertDoesNotThrow({ message }, executable)
Expand All @@ -401,24 +406,11 @@ inline fun <R> assertDoesNotThrow(
callsInPlace(message, AT_MOST_ONCE)
}

return Assertions.assertDoesNotThrow(
evaluateAndWrap(executable),
message
)
}

@OptIn(ExperimentalContracts::class)
@PublishedApi
internal inline fun <R> evaluateAndWrap(executable: () -> R): ThrowingSupplier<R> {
contract {
callsInPlace(executable, EXACTLY_ONCE)
}

return try {
val result = executable()
ThrowingSupplier { result }
} catch (throwable: Throwable) {
ThrowingSupplier { throw throwable }
try {
return executable()
} catch (t: Throwable) {
rethrowIfUnrecoverable(t)
throw AssertDoesNotThrow.createAssertionFailedError(message(), t)
}
}

Expand All @@ -439,7 +431,7 @@ fun <R> assertTimeout(
executable: () -> R
): R {
contract {
callsInPlace(executable, EXACTLY_ONCE)
callsInPlace(executable, AT_MOST_ONCE)
}

return Assertions.assertTimeout(timeout, executable)
Expand All @@ -463,7 +455,7 @@ fun <R> assertTimeout(
executable: () -> R
): R {
contract {
callsInPlace(executable, EXACTLY_ONCE)
callsInPlace(executable, AT_MOST_ONCE)
}

return Assertions.assertTimeout(timeout, executable, message)
Expand All @@ -487,7 +479,7 @@ fun <R> assertTimeout(
executable: () -> R
): R {
contract {
callsInPlace(executable, EXACTLY_ONCE)
callsInPlace(executable, AT_MOST_ONCE)
callsInPlace(message, AT_MOST_ONCE)
}

Expand Down
17 changes: 9 additions & 8 deletions jupiter-tests/src/test/kotlin/org/junit/jupiter/api/KotlinAssertTimeoutAssertionsTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,26 +156,27 @@ internal class KotlinAssertTimeoutAssertionsTests {
}

@Test
fun `assertTimeout with value initialization in lambda`() {
fun `assertTimeout with value assignment in lambda`() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that this test will pass even when no contract is specified. The AT_LEAST_ONE contract allows us to assign vals in lamda body. A test like this would be a better candidate, as it will pass only with AT_LEAST_ONCE or EXACTLY_ONCE contracts. This applies to other changes in this file too.

       @Test
    fun `assertTimeout with value assignment in lambda`() {
        val value: Int

        assertTimeout(ofMillis(500)) {
            value = 10 // Val can be assigned in the message supplier lambda.
            assertEquals(10, value)
        }
    }

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 for the suggestion. Applied in 5cac4e3

val value: Int

assertTimeout(ofMillis(500)) { value = 10 }

assertEquals(10, value)
assertTimeout(ofMillis(500)) {
value = 10 // Val can be assigned in the message supplier lambda.
assertEquals(10, value)
}
}

@Test
fun `assertTimeout with message and value initialization in lambda`() {
val value: Int
fun `assertTimeout with message and value assignment in lambda`() {
var value = 0

assertTimeout(ofMillis(500), "message") { value = 10 }

assertEquals(10, value)
}

@Test
fun `assertTimeout with message supplier and value initialization in lambda`() {
val value: Int
fun `assertTimeout with message supplier and value assignment in lambda`() {
var value = 0

@Suppress("ASSIGNED_BUT_NEVER_ACCESSED_VARIABLE")
val valueInMessageSupplier: Int
Expand Down