Do consumers of junit-jupiter-api also have to opt-in? If so, what happens of they don't?

No, since we opt-in in the junit-jupiter-api, consumers don't need to specify any opt-ins in their code

I think fun fail(message: (() -> String)?) should also have a contract added to it?

Also, can contracts be added to vararg executables: () -> Unit?

callsInPlace supports only non-nullable arguments.
It would be great to have something like:

fun fail(message: (() -> String)?): Nothing {
    contract {
        if (message != null) {
            callsInPlace(message, EXACTLY_ONCE)
        }
    }

    Assertions.fail<Nothing>(message)
}

but if is not allowed in a contract definition.

Similar thing applies to vararg executables: () -> Unit

Could the if be partially resolved like this? So that more (most) calls end up using the contract description for analysis?

// Covers method references `fail(::foo)`, inline lambdas `fail { }`.
fun fail(message: () -> String): Nothing {
    contract {
        callsInPlace(message, EXACTLY_ONCE)
    }
    Assertions.fail<Nothing>(message)
}
// Covers backwards compatibility and potential edge cases like `fail(foo.bar)` where bar is a nullable functional type.
fun fail(message: (() -> String)?): Nothing {
    Assertions.fail<Nothing>(message)
}

The method signatures are same from JVM perspective.

Platform declaration clash: The following declarations have the same JVM signature (fail(Lkotlin/jvm/functions/Function0;)Ljava/lang/Void;):
    fun fail(message: (() -> String)?): Nothing defined in org.junit.jupiter.api in file Assertions.kt
    fun fail(message: () -> String): Nothing defined in org.junit.jupiter.api in file Assertions.kt

Huh, I didn't get that when I tried. Anyway, @JvmName usually helps. The question is if this is a feasible solution to get contract in for most, and still keep supporting other edge cases. What's the use case for nullable lambda here?

Yes, creating a separate method with a contract and non-nullable lambda allows kotlin compiler to carry out contract checks when it's sure the passed lambda is not null.
I assume, lambda is made nullable to resemble existing java api where messageSupplier can be nullable

callsInPlace for fail method is also specified with InvocationKind.UNKNOWN .
If we set it to EXACTLY_ONCE the following code compiles:

val expectedValue = "value"
val value: String

try {
    fail {
        value = expectedValue
        "message"
    }
} catch (_: AssertionFailedError) {
    value = "another value"
}

assertEquals(expectedValue, value)

Here we reassign value albeit it is val. I reckon it isn't desirable to allow this code to compile

Is it actually true for things that can/expected to throw?
(I have a feeling we can't say "exactly once", but I might be misinterpreting the enum.)
Examples where it would not have compiled without the contract, and now it's a runtime error:

val value: String
// Swapped version of fun `assertThrows with value initialization in lambda`() in this PR
assertThrows<AssertionError> {
    Assertions.fail("message")
    value = "string"
}
assertEquals("string", value) // AssertionFailedError: expected: <string> but was: <null>

val x: String
assertThrows<IOException> {
    File("//").readText() // Deterministic IOException due to invalid name.
    x = "abc" // Will never execute, because it always throws.
}
// Kotlin thinks x is initialized here, because of the EXACTLY_ONCE.
assertEquals(3, x.length) // Consistent NPE, due to "effectively unreachable code" above.

val x: String
assertThrows<AssertionError> {
    assertTimeoutPreemptively(Duration.seconds(1)) {
        Thread.sleep(2000)
        x = "" // Will never execute, because it always times out.
    }
}
// Kotlin thinks x is initialized here, because of the chain of EXACTLY_ONCE.
println(x.length) // Consistent NPE, due to "effectively unreachable code" above.

What happens if you write this in Kotlin with try/catch? I presume the compiler pitches a fit?

val x: String
try {
    File("//").readText() // Deterministic IOException due to invalid name.
    x = "abc" // Will never execute, because it always throws.
} catch (e: IOException) {
    
}
assertEquals(3, x.length)

What about this?

val x: String
File("//").apply {
	readText() // Deterministic IOException due to invalid name.
    x = "abc" // Will never execute, because it always throws.
}

assertEquals(3, x.length)

If the apply case fails in the same way as your example, I think it's probably fine?

This looks like it will compile fine:

import java.io.File

var x: String
File("//").apply {
    readText() // Deterministic IOException due to invalid name.
    x = "abc" // Will never execute, because it always throws.
}

if (3 == x.length) {
    
}

Huh, very good point! 😁

try-catch example will not compile, but because not all code paths initialize x, but the point is valid there too, you can resolve this thread (I can't).

You could theoretically reimplement this logic, in kotlin, and expose it as an inline function. Then the try/catch will be inlined. Then there will be no need for the contract

Contracts don't work well when we suppress exception. Issues one and two.
To prevent any kind of errors and confusion, I suggest to replace callsInPlace(executable, EXACTLY_ONCE) with callsInPlace(executable) in all assertThrows methods.

What do you think?

Makes sense, I would be explicit with second param though, just for readability. If the Kotlin compiler is improved, is it easy to swap to a "better exactly once"? Or would it break users?

I haven't noticed any improvements in contracts. InvocationKind.UNKNOWN is set for assertTimeoutPreemptively methods because of the same reasons as for assertThrows methods

Should this just be

considering this is Kotlin code and there's a : Nothing signature top level fail.

Contracts work really weird with Nothing return type. Compiler warns that expression after assertThrows is not reachable while it is.

In the thread above I proposed not to specify InvocationKind for assertThrows methods. If there's no InvocationKind specified, value assignment in lambda will be forbidden and this test will make no sense

In that case I agree with UNKNOWN for invocation kind, because not being able to call production code that declares Nothing as return type is a big restriction. The point of assertThrows is that it tests exceptions thrown from a method, and Nothing declares exactly that.

In such case I'll set InvocationKind.UNKNOWN for assertThrows methods

Makes sense, I would be explicit with second param though, just for readability. If the Kotlin compiler is improved, is it easy to swap to a "better exactly once"? Or would it break users?

JUnit Experimental?

Is it possible to test for this contract?
As in: if it's removed, something should stop compiling or fail a test, right?
Probably the one in the related convo: #3259 (comment)

(This is probably true for all of them.)

I haven't found a way to test callsInPlace(..., UNKNOWN) contracts.
I though having another function with a contract inside could work, but it seems, kotlin compiler doesn't check whether a function is really called in place in the functions it's passed to.

fun notWorkingTest(value: () -> Unit) {
    contract {
        callsInPlace(value, UNKNOWN)
    }

    assertThrows<AssertionError>(value) // works fine even when assertThrows doesn't have a contract
}

This feels meta, using a Kotlin assertion to test a Kotlin assertion, but it's probably ok, since they have different use cases for these two.

We have a few tests for assertThrows. Also, a similar approach is used for assertDoesNotThrow tests

Reading this on GitHub makes me confused about the return type.
Again, not your code, but all your return types are nice and clean, probably worth aligning.

I've noticed that return type is not specified only for one-liners that return Unit. I assume, to resemble usual methods, where it isn't mandatory to specify Unit return type. For one-liners that return anything besides Unit, a return type is already specified

Why is this unknown?

See these two:

• Add Kotlin contracts to exposed Kotlin API #3259 (comment)
• Add Kotlin contracts to exposed Kotlin API #3259 (comment)

Contracts doesn't work well as being discussed in this conversation.
Perhaps, not specifying contracts for assertThrows and assertTimeoutPreemptively is a better option. For example, kotlin's runCatching method doesn't specify any contracts.
What do you think?

Contracts have been removed for all lambdas that may throw an exception

Would it be worth adding a comment into those methods that they intentionally don't have contracts and why?

Imo, adding comments regarding a contract absence everywhere adds nothing but noise. A person who's concerned by the missing contracts can check the commits and see why they aren't in place

My only note is that there are 18 methods affected, but only 6 new tests, and this may be normal because some contract stuff is not testable directly.

Added missing tests for assertion method overloads with message and message supplier

Nice 👍