Inline lambdas passed to assertDoesNotThrows to support suspending functions #2684

williamboman · 2021-08-11T00:26:42Z

Resolves #2674.

Overview

Support for suspending functions was added for the assertThrows() counterpart (see #1851), but it seems like calling suspending functions in an assertDoesNotThrow() block is currently not supported.

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
All continuous integration builds pass

…nctions Resolves junit-team#2674.

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

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

williamboman · 2021-08-13T15:15:47Z

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

@@ -147,6 +147,14 @@ inline fun <reified T : Throwable> assertThrows(noinline message: () -> String,
    }, Supplier(message))
 }

+@PublishedApi


https://stackoverflow.com/a/41905907

Again, possibly dumb question here, but why do we need this method to be inline?

Also, could you please move this method below the other ones calling it? :)

Again, possibly dumb question here, but why do we need this method to be inline?

Because this function is ultimately the one responsible for calling the provided executable. We want to run the executable in the same context as the callsite of assertDoesNotThrow (so that it can suspend, etc.), so we need to make sure the full call stack from assertDoesNotThrow until here uses inlined functions. If we were to remove the inline modifier on this function, we'll get a compiler error in all of the assertDoesNotThrow functions saying that we're violating the inlineability ^{(I totally just made that word up)} of the executable argument, and that we need to change the function signature to assertDoesNotThrow(noinline executable: () -> R). Doing so breaks the tests that now calls a suspending function, because the callback is no longer inlined.

edit: This is at least my understanding

I see! Thanks for the explanation 😄

…ow-coroutines * upstream/main: Introduce AnnotationSupport.findAnnotation() that searches enclosing classes Introduce unit tests for AnnotationUtils.findAnnotation(Class,Class,boolean) Always include classes selected via @SelectClasses in suites Polishing Polishing Fix TypedArgumentConverter example in User Guide Ensure TypedArgumentConverter can convert to a primitive Introduce ReflectionUtils.isAssignableTo(Class,Class) Fix Javadoc tag in TypedArgumentConverter

juliette-derancourt · 2021-08-13T17:14:34Z

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

+    @Test
+    fun `assertDoesNotThrow with coroutines`() = runBlocking {
+        val result = assertDoesNotThrow {
+            suspend { "Result" }()


We might want to have another test with a suspending function that actually throws an exception (thus making the test fail), since we want to make sure that the assertion is actually waiting for the "throwing"

Just noticed the test factory below, you could even add one dynamic test in each dynamicContainer for this new suspending function use case :)

Done! d48eeb4

codecov · 2021-08-13T17:25:31Z

Codecov Report

Merging #2684 (742d7af) into main (718d730) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2684      +/-   ##
==========================================
- Coverage   91.47%   91.37%   -0.11%     
==========================================
  Files         427      427              
  Lines       12067    12075       +8     
  Branches      941      941              
==========================================
- Hits        11038    11033       -5     
- Misses        788      801      +13     
  Partials      241      241

Impacted Files	Coverage Δ
...rc/main/kotlin/org/junit/jupiter/api/Assertions.kt	`40.00% <0.00%> (-22.97%)`	⬇️
.../junit/platform/commons/logging/LoggerFactory.java	`71.87% <0.00%> (-3.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 718d730...742d7af. Read the comment docs.

juliette-derancourt · 2021-08-16T12:39:00Z

@williamboman could you please add an entry in the release notes?

…ow-coroutines * upstream/main: Upgradle to 7.2-rc-3 Improve documentation on tags in User Guide Polishing Discover suite tests using parent configuration parameters Add support for private TempDir fields Document junit-team#2582 in release notes Simplify test Report exceptions thrown by CloseableResource as suppressed exceptions Use temp dir in constructor resolution test case Document scope in Javadoc Document configuration parameter Rename TempDirBehavior to TempDirectory.Scope Document in user guide Add to release notes Change TempDir to create a separate dir per declaration Split test case Reduce scope of SuppressWarnings annotation Revert "Add identifier element to `TempDir` annotation"

williamboman · 2021-08-16T15:35:11Z

@williamboman could you please add an entry in the release notes?

Done! Struggled a bit with the wording (for example, not sure if need to clarify it's Kotlin API or if that's implied), feel free to amend!

documentation/src/docs/asciidoc/release-notes/release-notes-5.8.0-M2.adoc

marcphilipp · 2021-08-16T17:30:41Z

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

+    )
+
+@PublishedApi
+internal inline fun <R> getThrowingSupplier(executable: () -> R): ThrowingSupplier<R> = try {


Not sure about this method name which is "effectively public" due to @PublishedApi. Maybe evaluateAndWrap or sth. similar would be clearer.

Yeah that's a good call, agreed.

742d7af

…8.0-M2.adoc Co-authored-by: Marc Philipp <marc@gradle.com>

…nctions (junit-team#2684) This commit adds support for suspending functions when `assertDoesNotThrow` is called from Kotlin. Closes junit-team#2674.

Inline lambdas passed to assertDoesNotThrows to support suspending fu…

3213dcf

…nctions Resolves junit-team#2674.

williamboman mentioned this pull request Aug 11, 2021

Support assertDoesNotThrow() with suspending functions in Kotlin #2674

Closed

1 task

pass explicit type param to avoid unreachable code workaround

b8154ec

williamboman commented Aug 11, 2021

View reviewed changes

junit-jupiter-api/src/main/kotlin/org/junit/jupiter/api/Assertions.kt Outdated Show resolved Hide resolved

williamboman commented Aug 11, 2021

View reviewed changes

junit-jupiter-engine/src/test/kotlin/org/junit/jupiter/api/KotlinAssertionsTests.kt Outdated Show resolved Hide resolved

overload function for Nothing return types

14407dd

williamboman commented Aug 13, 2021

View reviewed changes

williamboman added 2 commits August 13, 2021 17:24

remove reified types

80aeb8d

juliette-derancourt reviewed Aug 13, 2021

View reviewed changes

juliette-derancourt marked this pull request as ready for review August 13, 2021 17:14

williamboman added 2 commits August 13, 2021 20:16

move method to below all usage

de6c87c

add dynamic tests

d48eeb4

juliette-derancourt approved these changes Aug 16, 2021

View reviewed changes

williamboman added 2 commits August 16, 2021 17:32

add to release notes

69b5a17

marcphilipp approved these changes Aug 16, 2021

View reviewed changes

williamboman and others added 2 commits August 16, 2021 19:51

Update documentation/src/docs/asciidoc/release-notes/release-notes-5.…

5fd5d97

…8.0-M2.adoc Co-authored-by: Marc Philipp <marc@gradle.com>

s/getThrowingSupplier/evaluateAndWrap/

742d7af

sbrannen added component: Jupiter component: Kotlin type: enhancement labels Aug 17, 2021

juliette-derancourt merged commit 3f605f1 into junit-team:main Aug 17, 2021

juliette-derancourt added this to the 5.8 RC1 milestone Aug 17, 2021

sbrannen assigned williamboman Aug 17, 2021

sbrannen removed this from the 5.8 RC1 milestone Aug 17, 2021

williamboman deleted the assert-does-not-throw-coroutines branch August 17, 2021 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline lambdas passed to assertDoesNotThrows to support suspending functions #2684

Inline lambdas passed to assertDoesNotThrows to support suspending functions #2684

williamboman commented Aug 11, 2021 •

edited by sbrannen

Loading

williamboman Aug 13, 2021

juliette-derancourt Aug 13, 2021

juliette-derancourt Aug 13, 2021

williamboman Aug 13, 2021 •

edited

Loading

juliette-derancourt Aug 16, 2021

juliette-derancourt Aug 13, 2021

juliette-derancourt Aug 13, 2021

williamboman Aug 13, 2021

codecov bot commented Aug 13, 2021 •

edited

Loading

juliette-derancourt commented Aug 16, 2021

williamboman commented Aug 16, 2021

marcphilipp Aug 16, 2021

williamboman Aug 16, 2021

Inline lambdas passed to assertDoesNotThrows to support suspending functions #2684

Inline lambdas passed to assertDoesNotThrows to support suspending functions #2684

Conversation

williamboman commented Aug 11, 2021 • edited by sbrannen Loading

Overview

Definition of Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamboman Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 13, 2021 • edited Loading

Codecov Report

juliette-derancourt commented Aug 16, 2021

williamboman commented Aug 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamboman commented Aug 11, 2021 •

edited by sbrannen

Loading

williamboman Aug 13, 2021 •

edited

Loading

codecov bot commented Aug 13, 2021 •

edited

Loading