Add contracts to Kotlin-specific assertions by awelless · Pull Request #3259 · junit-team/junit-framework

awelless · 2023-04-17T13:25:11Z

Overview

Resolves #1866.
Introduced assertNull, assertNotNull methods.
Introduced contracts for assertNull, assertNotNull, assertThrows and assertDoesNotThrow methods.

Added smart casting tests for assertInstanceOf methods.

I hereby agree to the terms of the JUnit Contributor License Agreement.

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests including corner cases, errors, and exception handling
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes

JLLeitschuh · 2023-05-26T14:08:41Z

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

TWiStErRob · 2023-05-26T14:12:36Z

-    Assertions.assertTimeoutPreemptively(timeout, executable)
+fun <R> assertTimeoutPreemptively(timeout: Duration, executable: () -> R): R {
+    contract {
+        callsInPlace(executable, EXACTLY_ONCE)


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

https://github.com/junit-team/junit5/blob/ffab3a37f4a4e33529cca4ef9a464c244756c6fc/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertThrows.java#L49-L74

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

TWiStErRob · 2023-05-26T14:30:40Z

+
+        assertThrows<AssertionError> {
+            value = "string"
+            Assertions.fail("message")


Should this just be

Suggested change

Assertions.fail("message")

fail("message")

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

TWiStErRob

Looks to be in a reasonable state. Have some thoughts though.

TWiStErRob · 2023-08-10T16:53:28Z

-    Assertions.assertTimeoutPreemptively(timeout, executable)
+fun <R> assertTimeoutPreemptively(timeout: Duration, executable: () -> R): R {
+    contract {
+        callsInPlace(executable, EXACTLY_ONCE)


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?

TWiStErRob

Did a full review, wrote down what I noticed. Feel free to ignore some of them :)

Please resolve the conversations that are resolved, I can't do it, because only the PR Author and repo Members can.

And then we'll probably need a round of review from Marc or Jonathan.

TWiStErRob · 2023-08-27T15:27:41Z

+inline fun <reified T : Throwable> assertThrows(message: String, executable: () -> Unit): T {
+    contract {
+        callsInPlace(executable, UNKNOWN)
+    }


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 }

TWiStErRob · 2023-08-27T15:29:26Z

+        val error = assertThrows<AssertionFailedError> {
+            assertInstanceOf<String>(null)
+        }


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

TWiStErRob · 2023-08-27T15:42:34Z

 fun assertAll(heading: String?, vararg executables: () -> Unit) =
    assertAll(heading, executables.toList().stream())


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

JLLeitschuh · 2023-09-04T17:26:03Z

 */
 inline fun <reified T : Throwable> assertThrows(noinline message: () -> String, executable: () -> Unit): T {
+    contract {
+        callsInPlace(executable, UNKNOWN)


Why is this unknown?

See these two:

Add contracts to Kotlin-specific assertions #3259 (comment)

Add contracts to Kotlin-specific assertions #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

awelless · 2023-09-23T13:22:45Z

Is there anything else that should be added/adjusted? Otherwise, I'd say the pr is in its final form

TWiStErRob

LGTM

awelless · 2023-10-03T15:10:30Z

What do you think about the pr, @marcphilipp, @JLLeitschuh?

awelless · 2023-12-15T22:36:47Z

Hi. I believe the change is still relevant and the pr is ready to be reviewed.

marcphilipp · 2023-12-18T07:35:04Z

We'll consider it for 5.11, just need to find some time to review.

TWiStErRob · 2024-11-12T15:57:13Z

Bump, we're really missing this PR in Kotlin world. What needs to be done still?

marcphilipp · 2024-11-13T17:13:16Z

I think the PR needs to be updated to resolve the conflicts, checked for completeness, and then reviewed by someone from the core team.

@awelless Do you have time to do a rebase?

JLLeitschuh · 2024-11-13T17:56:07Z

I'd still love to see these added, happy to do a re-review after someone does a rebase

awelless · 2024-11-14T07:25:40Z

@marcphilipp
yes, I'll rebasethe PR this weekend.

marcphilipp · 2024-12-05T07:36:31Z

@awelless Thank you for your contribution! 🎉

JLLeitschuh · 2024-12-08T14:21:00Z

-): R = Assertions.assertTimeoutPreemptively(timeout, executable, message)
+): R {
+    contract {
+        callsInPlace(message, AT_MOST_ONCE)


Shouldn't executable also have a contract?

Good catch! Added in #4192.

It was ignored deliberately. Please, see this comment.

Oh, I had forgotten that discussion. Thanks for the reminder! 👍

Now I wonder, whether we should add the explanation why some of the lambdas don't have a contract. I'm not sure, if the actual code is the best place, though.

I think that's a good idea and I don't know a better place. Do you have time to submit a PR?

Could be a single sentence in code (each method), and a section at the top/bottom of the file?

I think a single comment in each of those methods should suffice. Something like

// No contract for `executable` because ...

Most likely I won't be able to do this in December.
I may take a look in January, if someone else hasn't done that by then.

I've taken care of it in ebf60bb

Follow-up on #3259

TWiStErRob · 2024-12-16T14:06:08Z

@marcphilipp did this not make 5.11.4?

marcphilipp · 2024-12-16T14:07:34Z

No, it's targeting 5.12 which should be released in January if things go according to plan.

marcphilipp · 2025-03-05T14:31:32Z

After bumping Kotlin's apiVersion and languageVersion this is causing some compiler warnings:

Resolve Kotlin 2.1 compiler warnings for contracts #4371

@awelless @TWiStErRob @JLLeitschuh Does one of you have time to take a look?

marcphilipp reviewed Apr 22, 2023

View reviewed changes

Comment thread junit-jupiter-api/junit-jupiter-api.gradle.kts Outdated

marcphilipp added the status: waiting-for-feedback label Apr 22, 2023

awelless requested a review from marcphilipp April 22, 2023 21:36

marcphilipp removed the status: waiting-for-feedback label Apr 23, 2023

marcphilipp reviewed Apr 23, 2023

View reviewed changes

Comment thread junit-jupiter-api/src/main/kotlin/org/junit/jupiter/api/Assertions.kt Outdated

awelless requested a review from marcphilipp April 23, 2023 19:06

JLLeitschuh reviewed May 26, 2023

View reviewed changes

TWiStErRob reviewed May 26, 2023

View reviewed changes

awelless requested review from JLLeitschuh and TWiStErRob May 26, 2023 20:15

TWiStErRob reviewed Jun 3, 2023

View reviewed changes

Comment thread junit-jupiter-api/src/main/kotlin/org/junit/jupiter/api/Assertions.kt Outdated

awelless requested a review from TWiStErRob June 3, 2023 21:39

awelless force-pushed the main branch from ed02ba8 to 19922d6 Compare August 10, 2023 07:12

TWiStErRob approved these changes Aug 10, 2023

View reviewed changes

TWiStErRob reviewed Aug 25, 2023

View reviewed changes

Comment thread junit-jupiter-api/src/main/kotlin/org/junit/jupiter/api/Assertions.kt Outdated

awelless requested a review from TWiStErRob August 27, 2023 14:36

TWiStErRob reviewed Aug 27, 2023

View reviewed changes

JLLeitschuh reviewed Sep 4, 2023

View reviewed changes

awelless requested a review from JLLeitschuh September 4, 2023 20:37

TWiStErRob approved these changes Sep 23, 2023

View reviewed changes

Comment thread junit-jupiter-engine/src/test/kotlin/org/junit/jupiter/api/KotlinAssertionsTests.kt Outdated

sbrannen changed the title ~~Added Kotlin Contracts to Exposed Kotlin API~~ Add Kotlin contracts to exposed Kotlin API Dec 16, 2023

JLLeitschuh reviewed Dec 8, 2024

View reviewed changes

marcphilipp added a commit that referenced this pull request Dec 9, 2024

Add contracts for executable parameters

8ad2527

Follow-up on #3259

marcphilipp mentioned this pull request Dec 9, 2024

Add contracts for executable parameters #4192

Closed

marcphilipp added a commit that referenced this pull request Dec 13, 2024

Add comments explaining lack of contracts for Executable parameter

ebf60bb

Follow-up on #3259

sbrannen added type: enhancement theme: programming model component: Jupiter component: Kotlin labels Jan 26, 2025

sbrannen assigned marcphilipp Jan 26, 2025

		fun assertAll(heading: String?, vararg executables: () -> Unit) =
		assertAll(heading, executables.toList().stream())

Uh oh!

Conversation

awelless commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TWiStErRob May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awelless May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TWiStErRob May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TWiStErRob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TWiStErRob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

awelless commented Apr 17, 2023 •

edited

Loading

TWiStErRob May 26, 2023 •

edited

Loading

awelless May 27, 2023 •

edited

Loading

TWiStErRob May 26, 2023 •

edited

Loading

JLLeitschuh May 26, 2023 •

edited

Loading