expect..toThrow does not support suspend functions

[matejdro]

Platform (jvm, js, android): all
Extension (none, kotlin 1.3, jdk8): none

Code related feature

Following code that uses coroutines's suspend functions does not compile:

suspend fun doSomething() {
    throw IllegalStateException()
}

@Test
fun test() {
    runBlocking {
        expect {
            doSomething()
        }.toThrow<IllegalStateException>()
    }
}

Problem stems from the fact that only suspend functions can call other suspend functions and expect is not a suspend function.

Other assertion libraries (such as assertFails from kotlin-test) work around this problem by marking all assertion methods that accept lambda inline. Not sure how feasible is this for atrium.

---

Please react with 👍 if you would like to see this feature implemented in Atrium, the more upvotes the more likely I (@robstoll) will implement it myself -- feel free to sponsor me, that would be a motivation too.
You are of course welcome to work on this issue. Write I'll work on it as comment so that we can assign the task to you.

[jGleitz]

Couldn’t you rewrite your example to

suspend fun doSomething() {
    throw IllegalStateException()
}

@Test
fun test() {
    expect {
        runBlocking {
            doSomething()
        }
    }.toThrow<IllegalStateException>()
}

and it would compile?

I think that it would only be a disadvantage if you have a test runner that supports coroutines. Because in that case, you have to add runBlocking and if atrium supported coroutines, you wouldn’t.

Are there any test runners that support coroutines?

[matejdro]

Yes, that could work, but it creates pretty ugly nested test code. I guess I could create expectBloking extension for myself.

[robstoll]

and share it with use afterwards 🙂
how about we introduce:

expect {

}.toThrowWhenRunning<IllegalStateException>()

? (open for better names)

[jGleitz]

@robstoll If we adopt this use case, I would prefer if we could make it so that you don’t need to use any differently named functions when using coroutines.

[robstoll]

I would prefer that as well but... I can imagine that the compiler won't be able to infer it correctly. Have to check though

[matejdro]

I came up with this:

/**
 * Variant of [expect] that wraps the code in [runBlocking], allowing
 * for usage of suspending function inside the body.
 */
fun <T> expectBlocking(act: suspend () -> T): Expect<() -> T> {
    return expect {
        runBlocking {
            act()
        }
    }
}

simple extension but reduces the boilerplate.

[robstoll]

The following would be possible:

inline fun <reified TExpected : Throwable> Expect<out suspend () -> Any?>.toThrow(): Expect<TExpected> =
    ExpectImpl.changeSubject(this).unreported { { runBlocking { it() } } }.toThrow<TExpected>()

But the usage is not so nice, as you need a suspend lambda after expect or what do you think @matejdro ?

expect(suspend {  
  doSomething() 
}).toThrow<IllegalStateException>()

In case you prefer your expectBlocking would you depend on an additional dependency something like atrium-verbs-coroutines which provides expectBlocking or would you just write your extension function as you did above?

[matejdro]

Is there an use case this suspend method covers that expectBlocking doesn't? Otherwise I prefer expectBlocking since it's just one function call.

Also, what is the difference between just calling runBlocking {} instead of `suspend {}´?

[robstoll]

runBlocking basically runs where the suspend { ... } only defines a lambda. In your expectBlocking runBlocking does not run immediately as you define a lambda as well (you wrapped runBlocking with { ... }).

What you could do with suspend { ... } what you cannot with expectBlocking is to make feature assertions which are themselves suspend functions. For instance:

class A {
  status: boolean = true
  suspend fun foo() = 1
}

fun test() {
  expect(A()) {
     feature { f(it::status) }.toBe(true)
     feature { sl(it::foo) }.toThrow<IllegalStateException>()
  }
}

Assuming we would add sl which stands for suspend lambda which is basically a shortcut for

feature("foo") { suspend { foo() } }.toThrow<IllegalStateException>()

[jGleitz]

In Kotlin, you can have overloads that only differ in the parameter being suspend, i.e. the following compiles:

fun myExpect(block: () -> String): String = TODO()
fun myExpect(block: suspend () -> String): String = TODO()

I propose we just add suspend versions for all relevant functions. From my point of view, this would be the best API, because the user does not have to think about whether a function uses coroutines or not. I think that the semantics are clear and that the implied runBlocking will not lead to ambiguities.

[matejdro]

If this is true, I think the same name overload solution is the best.

[robstoll]

It's a nice idea but does not work out as Kotlin cannot be able to figure out which overload it should take if you write:

myExpect { doSomething() }

It will always be ambiguous unless you prefix it with suspend, i.e. myExpect(suspend { ... })

We actually don't have an overload fun <R> expect(block: () -> R>): Expect<() -> R> but the problem remains since we have fun <T> expect(subject: T): RootExpect<T>
And if you write:

expect { doSomething() }

Then Kotlin assumes you want to pass a regular lambda as T and not that you wanted to pass a suspend lambda as T: suspend () -> R.
Which means we still need a different identifier e.g. expectBlocking to avoid ambiguities.
Of course we only need a different identifier if we think writing suspend { ... } is not good.

I have pushed a prototype so that you can try out things yourself:
https://github.com/robstoll/atrium/tree/toThrow-for-suspend-fun
See commit
78de3c3

[jGleitz]

Okay, multiple things to unwrap here:

It will always be ambiguous unless you prefix it with suspend

Yes, that makes sense, because you can pass a normal lambda to a function taking a suspend lambda. I did not know this, even though it makes a lot of sense.

Which means we still need a different identifier e.g. expectBlocking to avoid ambiguities.

I do not think so. If we add an expect like that:

fun <T> expect(subject: suspend CoroutineScope.() -> T): RootExpect<() -> T> =
    ExpectBuilder.forSubject { runBlocking(block = subject) }
        .withVerb(AssertionVerb.EXPECT)
        .withoutOptions()
        .build()

(but only that) everything works fine, as far as I can see:

expect("foo").toBe("bar") // resolved to the “normal” version

// normal lambda
expect { bar() }.toThrow<IllegalArgumentException>() // resolved to the suspend version, but that’s okay

// suspend lambda
expect { foo() }.toThrow<IllegalArgumentException>() // resolved to the suspend version

see test.kt on my branch

Then Kotlin assumes you want to pass a regular lambda as T and not that you wanted to pass a suspend lambda as T: suspend () -> R.

I ran resolving difficulties myself, but at least for me, the problem was something different. Kotlin does prefer the suspend expect over the T one (which it should since the suspend version is more specific). But Kotlin also prefers imports from packages over symbols from the unnamed package. So the real problem for me was that I had to define the suspend expect in the same package as the normal one, which I did

[robstoll]

Kotlin does prefer the suspend expect over the T one (which it should since the suspend version is more specific

You're basically right, that's the way it should be but unfortunately it's not how the the current type inference system behaves. However, it seems it is fixed with the new type inference system (I guess you have not noticed the problem since you have activated the new type inference in IntelliJ, right?). Therefore we would need to wait until Kotlin 1.4 is out.

Personally I would not use CoroutineScope but suspend () -> T since this way it is not bound to kotlinx.coroutines and we don't have to expose it to the user. Or what was the reason that you have chosen this one?

Also, we should think more about if it would not make sense to keep the type as suspend () -> T and not immediately apply the runBlocking in the assertion verb. I can imagine that under some circumstances you might want to use something different.
On the other hand, the normal use case, i.e. not a suspend lambda; would also result in Expect<suspend () -> T> which might be quite confusing

[robstoll]

but unfortunately it's not how the the current type inference system behaves

must have been a glitch, it works now 🎉

[jGleitz]

must have been a glitch, it works now 🎉

Great! So we got ourselves a solution?

---

Personally I would not use CoroutineScope but suspend () -> T
what was the reason that you have chosen this one?

Function like launch and async need a CoroutineScope. So I thought it make sense to provide it through the receiver, so clients can use it.

You could make the argument that most users will not actually launch new coroutines but rather only call existing suspend functions. And those who do could use croutineScope. So with that argument, we could omit CoroutineScope. I would personally leave it in but declare kotlinx.coroutines as a compileOnly dependency. This way, we don’t force the dependency on all users but offer a nice API to the coroutine users. 👈 that does not make sense because “normal” lambdas will also get routed to the suspend expect. Because of that, we need to include kotlinx.coroutines at least as an implementation dependency (because runBlocking must be available to all users).

---

Also, we should think more about if it would not make sense to keep the type as suspend () -> T and not immediately apply the runBlocking in the assertion verb.

Currently, I can not see any such use case. So if we do not find one, I would not propagate the suspend type because it would mean making more changes throughout atrium.

As far as I can see, we can change the suspend expect down the road and remain source-code compatible.

[robstoll]

Great! So we got ourselves a solution?

I think a basic idea at least. But the final implementation and test will show it, especially regarding other platforms than JVM. I don't know yet how to run a suspend fun on JS.

Function like launch and async need a CoroutineScope. So I thought it make sense to provide it through the receiver, so clients can use it.

Can you add examples to test.kt please, I am not really familiar with kotlin coroutines yet.

I would personally leave it in but declare kotlinx.coroutines as a compileOnly dependency.

that won't work as we need kotlinx.coroutines during runtime. We need an implementation dependency to it at least. Yet, if we want to use CoroutineScope then we should use api IMO as it is part of the API. I think implementation could even cause problems on the consumer part.

Currently, I can not see any such use case. So if we do not find one, I would not propagate the suspend type because it would mean making more changes throughout atrium.

That's true and we can still add it if someone requires it

[robstoll]

I have pushed a new commit to the branch, seems implementation would work, did not get problems in intellij. Thus we should start with implementation and move to api in case we see it is annoying

[robstoll]

One way to go for JS: Kotlin/kotlinx.coroutines#885

[jGleitz]

I am not really familiar with kotlin coroutines yet

I also only have basic knowledge. I can recommend Roman Elizarov’s “Structured Concurrency” on the topic of CoroutineScope, it helped me understand the ideas behind it.

Can you add examples to test.kt

I added a simple (and dump) example of something that relys on the receiver being CoroutineScope.

---

I have pushed a new commit to the branch, seems implementation would work, did not get problems in intellij.

That’s great. To me, it seems like we can offer a good API to coroutine users without exposing coroutine functions to the users that do not need them. Neat.

---

One way to go for JS: Kotlin/kotlinx.coroutines#885

Sounds promising!

[robstoll]

added a simple (and dump) example

I think that makes a good point, I don't see why one would want to do async operations within expect. If we want to support it, then one should be able to do something like:

expect {
    async { fetchTotal() }
} // Expect<Deferred<Int>>
    .notToThrow() // should wait for the result and convert to Expect<Int>
    .toBe(2)    

But... I am really not sure if Atrium should help out here. One could also write:

expect {
    fetchTotal()
} // Expect<() -> Int>
    .notToThrow() // converts to Expect<Int>
    .toBe(2)

Even for more complicated things like:

expect {
    fetchSubTotal1() + fetchSubTotal2()
}.notToThrow().toBe(2)

But maybe I wander from the subject.

One point we need to discuss is how we want to provide this new assertion verb. Personally I would be in favour of creating an additional atrium-verbs-coroutines module. This way we don't depend on kotlinx.coroutines for the normal Atrium user and also don't run into problems like:

• we need to fix the verb because kotlinx.coroutines made a breaking change in minor version x
• we need to provide multiple versions since we want that users of kotlinx.coroutines version x as well as version y can use it

if we provide it in an additional module then I definitely don't see a problem of using CoroutineScope either

[robstoll]

I think the build just gave us the answer:

'runBlocking(CoroutineContext = ..., suspend CoroutineScope.() -> T): T' is only available since Kotlin 1.3 and cannot be used in Kotlin 1.2

so we need another module as long as we support kotlin 1.2

[jGleitz]

Okay, so we go with the proposal but drop the CoroutineScope until a proper use case for it pops up?

[robstoll]

another suggestion by @tenor-dev would be coExpect instead of expectBlocking