-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
Inline lambdas passed to assertDoesNotThrows to support suspending functions #2684
Changes from 5 commits
3213dcf
b8154ec
14407dd
80aeb8d
67cd7a4
de6c87c
d48eeb4
69b5a17
ba06ff1
5fd5d97
742d7af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,12 +62,21 @@ class KotlinAssertionsTests { | |
} | ||
|
||
@Test | ||
fun `expected context exception testing`() = runBlocking<Unit> { | ||
fun `assertThrows with coroutines`() = runBlocking<Unit> { | ||
assertThrows<AssertionError>("Should fail async") { | ||
suspend { fail("Should fail async") }() | ||
} | ||
} | ||
|
||
@Test | ||
fun `assertDoesNotThrow with coroutines`() = runBlocking { | ||
val result = assertDoesNotThrow { | ||
suspend { "Result" }() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just noticed the test factory below, you could even add one dynamic test in each There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! d48eeb4 |
||
} | ||
|
||
assertEquals("Result", result) | ||
} | ||
|
||
@TestFactory | ||
fun assertDoesNotThrow(): Stream<out DynamicNode> = Stream.of( | ||
dynamicContainer("succeeds when no exception thrown", Stream.of( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://stackoverflow.com/a/41905907
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, possibly dumb question here, but why do we need this method to be
inline
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, could you please move this method below the other ones calling it? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this function is ultimately the one responsible for calling the provided
executable
. We want to run theexecutable
in the same context as the callsite ofassertDoesNotThrow
(so that it can suspend, etc.), so we need to make sure the full call stack fromassertDoesNotThrow
until here uses inlined functions. If we were to remove theinline
modifier on this function, we'll get a compiler error in all of theassertDoesNotThrow
functions saying that we're violating the inlineability (I totally just made that word up) of theexecutable
argument, and that we need to change the function signature toassertDoesNotThrow(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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! Thanks for the explanation 😄