Skip to content

kotlinx-coroutines-test: why the name runTest? #3050

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

Closed
joffrey-bion opened this issue Nov 25, 2021 · 11 comments
Closed

kotlinx-coroutines-test: why the name runTest? #3050

joffrey-bion opened this issue Nov 25, 2021 · 11 comments
Assignees
Labels

Comments

@joffrey-bion
Copy link
Contributor

joffrey-bion commented Nov 25, 2021

I understand that runBlockingTest was no longer an appropriate name because it is not always like runBlocking anymore, due to the new JS backend support where we cannot block.

But I find the runTest name a bit too vague for the uninitiated. I mean the difference between these 2 is unclear:

@Test
fun test() {
    // do stuff
}

@Test
fun test() = runTest {
    // do stuff
}

Because both of those "run a test". I feel like the name of runTest should somehow convey the idea that it's about running coroutines / suspending functions / async stuff, as opposed to "regular tests".

Were names like runSuspendingTest or runAsyncTest considered? Is it because it's expected to have many usages, and shorter is better?

@dkhalanskyjb dkhalanskyjb self-assigned this Nov 26, 2021
@dkhalanskyjb
Copy link
Collaborator

Hi!

I feel like the name of runTest should somehow convey the idea that it's about running coroutines / suspending functions / async stuff, as opposed to "regular tests".

I don't think this is an issue, as the package name kotlinx.coroutines.test conveys this. There are plenty of things that are only distinguished by their package name. What does the async function do? Ok, maybe something asynchronous, which likely means coroutines in Kotlin. Then what about the launch function? If one doesn't know beforehand that it launches a coroutine, what is the giveaway? What about Clock? Before you check to see that it's in java.time and so is related to the JSR 310 API, it could really be anything.

There are exceptions, of course, like CoroutineScope not just being a Scope, but we expect CoroutineScope to be useful often, as well as other people and libraries defining their own Scope classes. So, in order to avoid constant name clashes, CoroutineScope can't simply be Scope.

runAsyncTest

Not really true, as both in Native and on the JVM, the tests are not run asynchronously.

runSuspendingTest

This seems ok.

it's expected to have many usages

It is!

and shorter is better?

Not quite how I would phrase it. We're trying to make each word in a name count, as each one of them adds mental load. runBlockingTest before this conveyed that, this thing kotlinx.coroutines.test.runs a Test, but hey, this is a Blocking function, so be careful not to nest those unless you're really confident!

What misuse could we prevent by having a longer name? I don't see any: if someone tries to launch a suspending function without runTest, forgetting that runTest is not implied, the code will not compile; if someone accidentally runs non-suspending code in runTest, nothing bad will happen.

As a side note, we do want people to actually read the docs on runTest, as there is a very important point there that

@Test
fun testFoo() {
  runTest {
    // NO, WON'T WORK, DON'T WRITE CODE LIKE THIS
  }
}

will work incorrectly on the JS.

What do you think?

@joffrey-bion
Copy link
Contributor Author

joffrey-bion commented Nov 26, 2021

I feel like the name of runTest should somehow convey the idea that it's about running coroutines / suspending functions / async stuff, as opposed to "regular tests".

I don't think this is an issue, as the package name kotlinx.coroutines.test conveys this. There are plenty of things that are only distinguished by their package name

Mmh if this rationale was really followed, then run would be sufficient (name clash aside), but I don't think this is how people read code, especially since star imports are encouraged in Kotlin (so the package name won't be visible in code reviews).
I don't believe it was named runTest just to avoid the clash with the stdlib's run.

what about the launch function? If one doesn't know beforehand that it launches a coroutine, what is the giveaway?

You have a point regarding launch, although this is an extension on CoroutineScope, so the meaning must be understood in the context of the receiver (it's the point of a receiver). Process.kill() and Person.kill() are enough to tell the difference, and I believe it is different for top-level functions.

I disagree for async and Clock which do have a lot of meaning in themselves IMO. It's about running something asynchronously, and about managing time (enough information). Now if I saw runTest from another library, it wouldn't tell me anything. Maybe it's a performance testing library and this should be runTimedTest or something.

runAsyncTest

Not really true, as both in Native and on the JVM, the tests are not run asynchronously.

That would be the difference between runTestAsync and runAsyncTest, the order matters a lot in the meaning.

runTestAsync would mean "run this test asynchronously" (Async in this place refers to the verb), which indeed is not true on JVM and Native.
runAsyncTest means "run this asynchronous test" (in which Async is a qualifier of Test), which is quite different and true on all platforms.

We do use this function to run asynchronous stuff inside, in a synchronous or asynchronous way depending on the platform.

Note that, in this respect, runBlockingTest was slightly incorrect because it was not running a blocking test, it was running a test while blocking.

What misuse could we prevent by having a longer name?

My argument was mostly about readability rather than code correctness at writing time. I agree a different name wouldn't really prevent misuse, if we put aside the fact that any testing library of any topic could define a runTest function with as much legitimacy as the kotlinx-coroutines-test one.

@dkhalanskyjb
Copy link
Collaborator

then run would be sufficient (name clash aside)

Why put the name clash aside?

You have a point regarding launch, although this is an extension on CoroutineScope

You wouldn't know it just by looking at it in the code, where this particular receiver is usually implicit.

Now if I saw runTest from another library, it wouldn't tell me anything.

Do you have any particular library in mind, or are these concerns theoretical?

That would be the difference between runTestAsync and runAsyncTest, the order matters a lot in the meaning.

Ah, yes, you're right here. runBlockingTest sort of set the standard for the order here, so I just assumed that you proposed replacing "blocking" with "async" in it, preserving the semantics of the middle word established there.

That said, if we could imagine runTest from another library, we could also imagine a runAsyncTest function from RxJava or someone else. We would have more legitimacy by being the de-facto implementation of non-blocking programming for Kotlin, sure, but I'd argue that we have more legitimacy anyway. More on that below.

the fact that any testing library of any topic could define a runTest function with as much legitimacy as the kotlinx-coroutines-test one.

You may be right, but it's not obvious to me. I don't see an arbitrary library creating a function with this name, because… what would that function do? You provide an example of measuring execution time, but this is not a test-only thing to do, so why would test be in the name? Kotlin does have https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.system/measure-time-millis.html, by the way.

Also, runTest additionally implies that the test ends when the function call ends, which is true for us, but wouldn't be true for a runTimedTest, as one would want to access the measurements and probably act on them.

kotlinx.coroutines.test.runTest is in a unique position in that it has a special signature:

// very simplified, but the essence is this:
fun runTest(block: suspend () -> Unit): (Unit | Promise<Unit>)

It provides an entry point to test suspend-functions, a fundamental language concept. We could conceivably even make it the default behavior of tests. In fact, at some point, we wanted to (https://youtrack.jetbrains.com/issue/KT-22228), but ultimately decided against this.

So, why would any testing library really define a runTest function at all? Would it have the same legitimacy in doing so? I don't see why.

@joffrey-bion
Copy link
Contributor Author

joffrey-bion commented Nov 26, 2021

First of all, thanks a lot for even having the discussion, by the way.

Why put the name clash aside?

Because "I don't believe it was named runTest just to avoid the clash with the stdlib's run." - but maybe I was wrong here.

You have a point regarding launch, although this is an extension on CoroutineScope

You wouldn't know it just by looking at it in the code, where this particular receiver is usually implicit.

I agree it's not always obvious of course, but I find it more legitimate to assume the reader is aware of the receiver type rather than the package of the function. Awareness can come from explicit receiver specified, or in scope of the current function/method, while imports are likely to be star imports, especially in this case.

Do you have any particular library in mind, or are these concerns theoretical?

Only theoretical. Note that this is not a concern about actual name clash, it's about finding a name that could represent less things semantically (and being a possible choice for other totally unrelated testing libraries is a way to measure the semantic precision of the name).

That said, if we could imagine runTest from another library, we could also imagine a runAsyncTest function from RxJava or someone else.

Again it's more about naming precision than actual clash prevention. runAsyncTest has more meaning than runTest, so it enormously reduces the amount of potential interpretations for the name here. It's not really about reducing "competitors for the name", but it also does have this effect too. I would be OK to see runAsyncTest from RxJava even if there is runAsyncTest in the coroutines library, because they would both deal with running tests with asynchronous stuff.

I don't expect a mocking library, or testcontainers, etc. to declare a global runAsyncTest (they would have less legitimacy to do so than a library whose purpose is asynchronous stuff). On the other hand runTest seems a good fit for anything that runs a test - all bets are off.

I don't see an arbitrary library creating a function with this name, because… what would that function do?

Yet, kotlinx-coroutines-test did. Technically the same could be said by other library authors.

You provide an example of measuring execution time, but this is not a test-only thing to do, so why would test be in the name

I mentioned performance testing, but it's true that maybe this example was not the best to illustrate my point after all, sorry.

Also, runTest additionally implies that the test ends when the function call ends, which is true for us, but wouldn't be true for a runTimedTest, as one would want to access the measurements and probably act on them.

This is a valid argument for a bigger legitimacy of this library over others, thanks. So indeed this kinda invalidates my previous statement about "all bets are off" for other libraries - it implies somehow that it runs the whole test, so it also limits the possible interpretations in a way. It doesn't prevent us from going one step further and also mention suspensions/asynchronism, though.

kotlinx.coroutines.test.runTest is in a unique position in that it has a special signature
It provides an entry point to test suspend-functions, a fundamental language concept.
We could conceivably even make it the default behavior of tests. In fact, at some point, we wanted to (https://youtrack.jetbrains.com/issue/KT-22228), but ultimately decided against this.

Yes, I remember the discussion about making it a language feature. That being said, I bet the language feature would have included some hint at asynchronous or suspending stuff (like a suspend keyword on the test function).

I wonder what exactly would be a drawback of naming it runAsyncTest or runSuspendingTest for instance? It's one extra word, yes, but given the additional semantics I personally find it appropriate. In any case these are just my 2 cents, of course, I'm sorry if I'm splitting hairs 😄

@dkhalanskyjb
Copy link
Collaborator

Sorry, I caught a cold and won't be responding for a while. I wouldn't even write this if I didn't think we wanted to reach a conclusion on this while we're still in the RC phase.

@qwwdfsad, could you take this from here? I can sort of see both sides and will accept any outcome.

  • runTest/TestResult are good names:
    • They do not repeat the contents of their signatures.
    • I can't imagine a name clash happening with runTest; maybe with TestResult, but it should almost never be directly mentioned.
    • The meaning of runTest and TestResult need only be internalized once, afterwards these names should never be an issue.
    • The succinctness makes the signature lines of tests less cumbersome, and there are a lot of tests, so the effects are cumulative.
  • Something like runSuspendTest/SuspendTestResult also has its merits:
    • Not that long; still shorter than runBlockingTest.
    • A lot of work happens outside of the IDE, and you can't look up a signature of a function used in a StackOverflow answer. In these cases, runSuspendTest at least imparts some knowledge about what a function does. There are many names in Kotlin that don't make sense until looked up, like also, let, apply, run, etc., but these are all stdlib functions.

@russhwolf
Copy link

When considering name collisions, don't forget that there's also user code in addition to stdlib/kotlinx. For example, KaMPKit defines its own runTest function. While in that case, we'd probably replace it with this one anyway, I can easily imagine another project wrapping its own particular test setup code in a similar function that might conflict with the coroutines-test one.

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Dec 6, 2021

We've discussed it one more time and decided to keep things as they are.

runTest is a rare case of the library-based solution for a language design problem being the solution, meaning that we expect this very signature to be used when the problem of testing suspendable code arise.

It will eventually be well-known and advised to be used, it will be internalized by the developers who write code with coroutines and we want the name to be short and concise. It is in fact no different from @Test or assertEquals from the standard library, or Flow (the name does not reflect a lot and clashes with Flow from Java 9).
I'd say that the benefits of the runTest name outweigh its drawback.

One of the main benefits of having something like runSuspendTest -- it won't clash with existing runTest functions.
After a grace stabilization period, we don't really expect many people to have kotlinx-coroutines-test in the classpath and in the imports and not using runTest. And for the rare cases when it is a problem, it is possible to use named imports to avoid the clash.

@joffrey-bion
Copy link
Contributor Author

joffrey-bion commented Dec 6, 2021

@qwwdfsad thanks for the consideration and for reporting back here.

It will eventually be well-known and advised to be used, it will be internalized by the developers who write code with coroutines and we want the name to be short and concise.

That's fair enough.

It is in fact no different from @test or assertEquals from the standard library

I'm not sure I understand the point here. @Test marks a test, any test, not only suspending ones. It doesn't need a more specific name because the name is as specific as can be. Same for assertEquals it asserts that 2 things are equal. runTest doesn't run any test, it runs a test that has suspend functions/coroutines, it is pointless in any other case. My point was that the name could be more specific to reflect the intent better.

or Flow (the name does not reflect a lot and clashes with Flow from Java 9).

On the contrary, Flow is pretty classic for representing asynchronous flow. It's also similar to Flux.
Especially because Java uses it to represent a similar concept, the name fits perfectly.
Again please note that my point was never about the name clash, but only about the semantic imprecision.

Trying to avoid clashes with similar libraries that operate on the same concepts shouldn't really be a goal in my opinion, because sometimes a name for a concept is just very good, regardless of which library implements it. If Function is a type that represents a function, I don't care that it conflicts with other reflection libraries that deal with general functions. But if I'm creating an async library that defines a specific type to represent a completion callback with a specific signature, it would be inappropriate to name it just Function - better be specific and name it Callback or CompletionCallback.

In any case, thanks again for letting us know the final decision. I'm just nitpicking here, and I'm pretty ok with runTest too. In the end it's your call. Feel free to close the issue ;)

@dkhalanskyjb
Copy link
Collaborator

I'm back.

my point was never about the name clash, but only about the semantic imprecision.

I'll respond directly to this point then.

Semantic precision is not our goal, because it is not always viable. Flow is a good example: this name does not denote anything other than a vague feeling of something flowing somewhere. We could begin by calling it AsynchronousFlow—this way, it would be obvious that something happens asynchronously. However, this still wouldn't mean much. AsynchronousSubscribableValueGenerator is somewhat close to being semantically precise. However, it still requires reading the documentation to understand what it does, and if we were (or everyone else who uses the term "flow") to adopt such a name, then… well, someone would have to invent a shorter name, because, obviously, no one would bother with pronouncing all of this.

Instead, "Flow" (and "Flux") are just some vague names that only get their meanings from their wide use. There is zero semantic precision in them, and yet, they are fine, as everyone just accepted them as a separate abstraction.

Likewise, runSuspendTest, runAsyncTest, etc., are not semantically precise at all. The issue, interestingly, is not in the middle word, but in the last one: what does it mean to run some block (asynchronous or not) as a test? What discerns tests from not-tests? Tests are self-contained, isolated pieces of functionality without side-effects—so, does runTest ensure that the thing we're running won't have side effects? It does not. So, were we to strive for semantic precision, the "test" part would have to go, as it is completely meaningless and misleading if one were to rely only on the name to understand the behavior of the function.

A more semantically precise name would be runSuspendBlockSkippingDelays, which is the behavior that the people using the function are usually interested in. This, of course, still ignores the various ways in which this function integrates with the rest of the test framework, like attempting to use the source of virtual time with which Dispatchers.setMain was called, or integrating wth TestScope, or handling uncaught exceptions, or timing out on long periods of apparent inactivity…

Flow is a complex concept. runTest is a complex function. Both are so to the point where it's not viable to strive for semantic precision. So, we use other considerations to decide on the names. If you know what runTest is, we assume that you won't get confused using it. If you don't, you won't learn it just from the name runSuspendTest, you will need further information. runSuspendTest is better in the case of someone new to coroutines learning about them from StackOverflow answers, as then there's some additional context available immediately, but other than that, does it have any other benefits? We don't see any.

@joffrey-bion
Copy link
Contributor Author

joffrey-bion commented Dec 10, 2021

@dkhalanskyjb Welcome back :) Thanks a lot for taking the time to answer, these are all fair points.

Semantic precision is a spectrum. I totally agree that it's pointless to try to be the most precise in a name. It's technically not possible, because the name is an abstraction of the implementation, so you have to leave out some details. It's all about balance, and finding what is worth leaving out and what is worth including in the name (naming is hard, as the saying goes).

Flow does not have zero semantic precision, it has some (but I agree it's quite little). run or thing would be closer to zero, and runTest is slightly above that. Flow seems to be enough for all intents and purposes, but probably I am biased because it's now well known, as you said.

runSuspendTest would indeed not cover all the meaning of what it does, but it is closer than runTest. runSuspendBlockSkippingDelays is even more precise, but also needlessly complicated. I guess it's all about choosing the sweet spot, and this is a subjective point.

From what I can see, it just seems we believe the sweet spot to be in a different place, and this is perfectly fine. Again, I'm splitting hair and I don't want you to waste more time in these discussions. runTest will likely become well-know pretty soon like other coroutine functions, and at that point having a short name is probably more beneficial than a precise name.

Thanks again for this discussion!

@dkhalanskyjb
Copy link
Collaborator

From what I can see, it just seems we believe the sweet spot to be in a different place, and this is perfectly fine.

Yeah, in cases of methods that are going to be used all the time, practicality is more important to us than esthetics. I would wholly agree with your arguments if runTest was supposed to be used once or twice in any given codebase, if even that, and would require re-learning it each time it's encountered. As is though, not really.

Thanks again for this discussion!

Sure, thanks for sharing your doubts! You never know, sometimes such discussions lead to surprising insights, and in turn, to overturning decisions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants