https://kotlinlang.org logo
#orbit-mvi
Title
# orbit-mvi
o

Oleksii Malovanyi

06/10/2021, 3:51 PM
Hey guys, I’m kinda dumb, but can’t make it work: how you test the
delay()
if occurs inside the
intent
block? I’ve tried to pass
TestCoroutineDispatcher
as the
orbitDispatcher
and then
advanceTimeBy
but it has no effect and I have to wait for the delay to finish 😞
I guess, currently it’s not possible due to TestContainer() -> it swaps the dispatcher with own (depending on blocking)
I guess, the solution could be to remove
blocking
param from the
test()
extensions and swap it with the dispatcher (unconfined) by default -> then it would be easy to use own
TestCoroutineDispatcher
m

Mikolaj Leszczynski

06/11/2021, 7:25 AM
Yeah you’re absolutely right, I think we need to make this change
thanks for the feedback!
I might be able to fix it today
❤️ 2
got the implementation done, but I need to check whether we can indeed remove
blocking
as it’s not as simple as just setting a dispatcher - it also uses
runBlocking
whenever you invoke
orbit
on the container. I’m wondering whether in some circumstances, like launching a coroutine or switching context to something other than unconfined in your orbit flow might cause the blocking constraint to be violated. My gut feeling is yes but I need a unit test for this corner case. Will continue in the evening.
o

Oleksii Malovanyi

06/11/2021, 2:33 PM
thanks, for the update! IMHO I’d like to have tools to manipulate them with: e.g.:
runBlockingTest
could re-use
TestCoroutineDispatcher
which could be passed through to the VM dependencies
m

Mikolaj Leszczynski

06/11/2021, 2:35 PM
the thing is the
blocking
flag not only changes the dispatchers, it also does this:
Copy code
override fun orbit(orbitFlow: suspend ContainerContext<STATE, SIDE_EFFECT>.() -> Unit) {
        if (!isolateFlow || dispatched.compareAndSet(0, 1)) {
            if (blocking) {
                runBlocking {
                    orbitFlow(pluginContext)
                }
            } else {
                super.orbit(orbitFlow)
            }
        }
    }
if you use
runBlocking
the test will block whatever you do inside your flow, not sure using just the
TestCoroutineDuispatcher
will give you that guarantee
o

Oleksii Malovanyi

06/11/2021, 2:36 PM
If I got the code correctly, it makes no sense if you use the runBlockingTest and own dispatcher -> the behaviour should be the same 🤔 But I see your point: it’s not that straightforward as blocking option..
TestCoroutineDuispatcher + runBlockingTest should do the same as runBlocking 👀
m

Mikolaj Leszczynski

06/11/2021, 2:37 PM
thanks for the clarification 🙂
I have a test that breaks when we don’t use
runBlocking
, I will re-verify with
runBlockingTest
later
1
of course I agree with you - would be better because then this behaviour can be controlled from the outside using standard coroutine mechanisms
o

Oleksii Malovanyi

06/11/2021, 2:38 PM
I mean
runBlockingTest(myTestCoroutineDispatcherInstance)
m

Mikolaj Leszczynski

06/16/2021, 6:38 AM
so to be clear @Oleksii Malovanyi you reckon something like the following would guarantee a blocking test execution?
Copy code
runBlockingTest(myTestDispatcher) {
  val action = Random.nextInt()
  val middleware = MyMiddleware().test(initialState)

  middleware.doSomething(action)

  container.assert(initialState) {
    ...
  }

}
o

Oleksii Malovanyi

06/16/2021, 6:46 AM
I’ve never used this approach, but as far as I understand the source code, it does exactly what is needed
also, 1 more problem with
runBlocking
-> as we swap the context completely from the container, the original Settings’ exceptionHandler no longer could be found in tests’s context: so what is working in production is not working in test mode…
m

Mikolaj Leszczynski

06/16/2021, 6:51 AM
I’ll take that into account
I think for this to work in a blocking way all coroutines would have to be launched in the runBlockingTest scope which is not true.
intent
is not a suspending function - it triggers a coroutine launch using orbit’s internal scope so this method as it is right now will never block without some
runBlocking
hidden in the test container. Writing tests to confirm.
o

Oleksii Malovanyi

06/16/2021, 6:56 AM
this simple case works flawlessly
Copy code
class TestCoroutineDispatcherTest {

    private val dispatcher = TestCoroutineDispatcher()

    @Test
    fun `on executor no delay`() = runBlockingTest(dispatcher) {
        Executor(dispatcher).invoke()
    }
}

class Executor(private val dispatcher: CoroutineDispatcher = Dispatchers.Default) {

    suspend operator fun invoke() {
        withContext(dispatcher) {
            delay(10_000)
            println("Yeah")
        }
    }
}
Copy code
runBlockingTest
under the hood creates it’s own scope, but if we provide test dispatcher, it uses it to wait for all the coroutines started with it to finish
m

Mikolaj Leszczynski

06/16/2021, 7:00 AM
started with the scope or the dispatcher?
o

Oleksii Malovanyi

06/16/2021, 7:00 AM
dispatcher should be
m

Mikolaj Leszczynski

06/16/2021, 7:00 AM
I’m not disputing the fact it will block until all the coroutines in the test complete 🙂
what I’m concerned about is the fact that after invoking an action on the container we need to wait for that action to fully complete before we do any assertions
simply replacing the dispatcher gives us no such guarantee
o

Oleksii Malovanyi

06/16/2021, 7:03 AM
could the
assert
func use runBlockingTest under the hood? 🤔
m

Mikolaj Leszczynski

06/16/2021, 7:04 AM
I mean now that I think about it it doesn’t necessarily need to - we already support “awaiting” for results to arrive
however this unnecessarily slows down failing tests, which is why blocking mode was introduced in the first place
we “await” for results that e.g. never arrive
o

Oleksii Malovanyi

06/16/2021, 7:05 AM
the main reason why to use
TestCoroutineDispatcher
is to deal with the
delay
func to test the timeouts
runBlocking
can’t rewind the time, so tests still pass but then the real time ticks..
m

Mikolaj Leszczynski

06/16/2021, 7:07 AM
yep - not disputing we need this either 🙂
just trying to find a way that would satisfy your need and provide the best experience when testing
🤝 1
there is also a bit of a wrinkle in that
kotlinx-coroutines-test
is not multiplatform
o

Oleksii Malovanyi

06/16/2021, 7:08 AM
YAS, that’s the worse
could you please share your working branch? I’d try to join
m

Mikolaj Leszczynski

06/16/2021, 7:19 AM
getting it in a presentable state
FYI I’m not sure you need
runBlockingTest(myTestDispatcher)
as
runBlockingTest
uses a
TestCoroutineScope
internally which sets this dispatcher if one is not provided
Copy code
public fun TestCoroutineScope(context: CoroutineContext = EmptyCoroutineContext): TestCoroutineScope {
    var safeContext = context
    if (context[ContinuationInterceptor] == null) safeContext += TestCoroutineDispatcher()
    if (context[CoroutineExceptionHandler] == null) safeContext += TestCoroutineExceptionHandler()
    return TestCoroutineScopeImpl(safeContext)
}
o

Oleksii Malovanyi

06/16/2021, 7:58 AM
the thing is dispatcher has to be passed if needs to be shared with the production code, otherwise -> you’r right
in case of orbit, we do use dispatcher, so it has to be shared (swapable)
m

Mikolaj Leszczynski

06/16/2021, 7:59 AM
@Oleksii Malovanyi after running some tests I think there’s another issue. Notably, only coroutines launched directly with the
TestCoroutineDispatcher
can have their scheduling controlled. So, even if we set orbitDispatcher =
TestCoroutineDispatcher
the orbit event loop substitutes it with
Dispatchers.Unconfined
and
advanceTimeBy
has no effect. We could let you replace the event loop dispatcher, but this doesn’t mean you can’t just
withContext(Dispatchers.Default
in your flow and stop it working again…
o

Oleksii Malovanyi

06/16/2021, 8:01 AM
withContext(Dispatchers.Default)
is the code smell for any coroutine-based code ¯\_(ツ)_/¯ till kotlin team doesn’t introduce the way to swap it like RxPlugins
code we have a sneaky way to swap the loop dispatcher for the tests purpose then? 🤔
as for the runBlockingTest, you’re right - this works:
Copy code
@OptIn(ExperimentalStdlibApi::class)
class TestCoroutineDispatcherTest {

    @Test
    fun `on executor no delay`() = runBlockingTest() {
        val dispatcher = coroutineContext[CoroutineDispatcher]
        Executor(dispatcher!!).invoke()
    }
}
m

Mikolaj Leszczynski

06/16/2021, 8:07 AM
I mean sure, maybe making it customizable is the right thing to do, considering like you said - one can’t replace dispatchers globally yet…
it’s just really tedious having to go through all these hoops to get this done
o

Oleksii Malovanyi

06/16/2021, 8:08 AM
yes, I see this is quite a complex task to juggle the contexts through the structure
m

Mikolaj Leszczynski

06/16/2021, 8:09 AM
it just doesn’t feel like the kind of simple experience I’d like people to have y’know
but at the same time there’s probably no way around this
o

Oleksii Malovanyi

06/16/2021, 8:10 AM
yeap, the only *simplifying I see here is the defaults…
m

Mikolaj Leszczynski

06/16/2021, 8:18 AM
test-dispatcher-overrides
branch if you want to play around with it… There are a couple of hacks still in there, very much a WIP
I have to park it for the day but will continue over the week
o

Oleksii Malovanyi

06/16/2021, 8:18 AM
thanks 🙏 I’ll take a look as well
m

Mikolaj Leszczynski

06/16/2021, 8:19 AM
anyway I got rid of the
runBlocking
that was there in
TestContainer
, so I had to hack the assertions to
await
, this is not exactly the experience I had in mind
maybe the right thing to do is to have both -
runBlocking
in TestContainer and allowing you to override the dispatchers
it would at least allow you to do what you want to do
o

Oleksii Malovanyi

06/16/2021, 8:20 AM
maybe, but till it doesn’t conflict
m

Mikolaj Leszczynski

06/16/2021, 8:20 AM
I don’t think it does tbh
o

Oleksii Malovanyi

06/16/2021, 8:20 AM
also, I feel like runBlocking or runBlockingTest is defacto standart thing for testing 🤔 but I’m lacking experience here
m

Mikolaj Leszczynski

06/16/2021, 8:21 AM
coroutine testing still feels like pre-alpha to be honest
1
definitely not enough focus has been put on it
I don’t have much experience here either sadly
o

Oleksii Malovanyi

06/16/2021, 12:22 PM
@Mikolaj Leszczynski I’ve pushed a fix (see 2 latest commits) to the branch you’ve proposed: https://github.com/almozavr/orbit-mvi/commits/test-dispatcher-overrides
m

Mikolaj Leszczynski

06/16/2021, 12:25 PM
hey - we can’t set it to the background dispatcher. This would break serial ordering when all you do is reducing and you’re not switching contexts
Unconfined
is used on purpose here - it will execute on the
orbit
dispatcher until the first suspension point
if anything, we need another
Settings
entry
o

Oleksii Malovanyi

06/16/2021, 12:28 PM
overall I feel like this testing structure is too fragile and implicit not to make a mistake in the future, so I’d suggest to think about introducing another testing-only container wrapper with the runBlocking underneath, e.g.: hide runBlockingTest inside the test call, test should return Assertable interface which only allows call to assert method
Copy code
StateTestMiddleware().test(initialState = initialState) {
    somethingInBackground(action)
}.assert(initialState) {
    states(
        { copy(count = action) }
    )
}
m

Mikolaj Leszczynski

06/16/2021, 12:30 PM
@appmattus thoughts? We’ve decided against such a wrapper in the past. @Oleksii Malovanyi
too fragile and implicit not to make a mistake in the future
- could you please elaborate?
o

Oleksii Malovanyi

06/16/2021, 12:33 PM
@Mikolaj Leszczynski the
runBlockingTest
block has to go always before the
assert
call to guarantee any delays are rewinded (until idel) before we call the assert
so either we do this by hiding runBlockingTest under the hood (which I believe a good idea in terms of automagic solution) and forcing user to call assert after this block, or we should think about changes to the assert block to get rid of the GlobalScope with timeout, maybe?
m

Mikolaj Leszczynski

06/16/2021, 12:37 PM
I see
it’s a bit of a hot mess isn’t it
partly caused by only rudimentary coroutine testing facilities provided by Jetbrains, partly due to our own lack of experience testing coroutines
o

Oleksii Malovanyi

06/16/2021, 12:38 PM
I mean I’d like to have a statically safe solutions, not a conventions. test + assert and testBlocking{…} + assert
m

Mikolaj Leszczynski

06/16/2021, 12:39 PM
runBlockingTest under the hood (which I believe a good idea in terms of automagic solution)
This is a no-go as long as coroutines test is not multiplatform 😞
o

Oleksii Malovanyi

06/16/2021, 12:39 PM
okay, I see
then I’d like to be un-blocked at least by the dispatchers, so I could create this sugar-solution for my internal needs
what do you think about the
backgroundDispatcher
as long as it stays
Unconfined
by default?
I think it could be too dangerous for the legacy, as we don’t know if people currently use it or not ¯\_(ツ)_/¯
as far as I see till now backgroundDispatcher is unused but part of the public api… so, maybe, I’d suggest to deprecate it while introducing new property for the testing purpose
m

Mikolaj Leszczynski

06/16/2021, 12:43 PM
huh hang on
I never noticed that it’s actually unused
it used to be there in the
complex
syntax
but the simple syntax doesn’t automatically schedule background tasks… 🤔 I think the solution you’re suggesting is fine
the only danger I see is if people have set it somewhere to something else than the default
I find it highly unlikely though
o

Oleksii Malovanyi

06/16/2021, 12:44 PM
the same
the only safe thing is to create lib-visible properties up to the test method, but then it would be opiniated solution you don’t want to come up with yet as I understand
m

Mikolaj Leszczynski

06/16/2021, 12:45 PM
We can always mark this as a breaking change.
OK, I have another couple of ideas I want to play around with
👍 1
the best way to keep this clean is to have as much information in settings as possible
maybe we can have a
test mode
setting in there too
currently - you’re right it’s far too implicit
o

Oleksii Malovanyi

06/16/2021, 12:55 PM
yeap. My pain point was to understand that blocking/non-blocking is a convention compared to the assert’s timeout and GlobalScope. As well as why my CoroutineExceptionHandler no longer works (due to test method made a complete dispatchers swap previously). If you ask me, I’d try to improve this syntax to make it more strict, I mean test+assert calls
also I don’t see why people would like to keep their prod Settings’ dispatchers when invoking the
test
method 🤔
m

Mikolaj Leszczynski

06/16/2021, 12:58 PM
well you wouldn’t touch it normally
but you should be able to change it should you wish to do so
1
o

Oleksii Malovanyi

06/16/2021, 12:59 PM
so, IMHO I’d always use test+runblockingtest+assert, but I have a very narrow picture from the android point of view ¯\_(ツ)_/¯ In the end, I guess, the less opinion the lib has and the more open it is (of course with some defaults and a sugar out of the box), the better for the end user
m

Mikolaj Leszczynski

06/16/2021, 1:01 PM
agreed - to be honest though what we have is not too far off in a way - it’s just that the configuration is all over the place and it overrides production configs
maybe the solution to
coroutines-test
not being multiplatform is to use
runBlockingTest
on jvm and
runBlocking
everywhere else
o

Oleksii Malovanyi

06/16/2021, 1:02 PM
btw, yes 👍
as Android-only dev I’d vote for this 😆
m

Mikolaj Leszczynski

06/16/2021, 1:07 PM
hah, well. We’d like to make it fully multiplatform but there are a number of stumbling blocks, most of them on Jetbrains’ side 🙂
o

Oleksii Malovanyi

06/16/2021, 1:08 PM
yes, the testing and dispatchers are really big… hopefully one day it would be available
m

Mikolaj Leszczynski

06/16/2021, 1:09 PM
I’ll also check if there is any mileage at all in the idea to expose
Container.orbit
as suspending…
that could make testing a little less magic
cause you could just use standard coroutine constructs to control the dispatching behaviour in tests
but I think this is not simple to implement
@Oleksii Malovanyi FYI still working on it, currently prototyping a radical change, keep your fingers crossed
o

Oleksii Malovanyi

06/18/2021, 6:58 AM
🤞 🤞 🤞
m

Mikolaj Leszczynski

06/22/2021, 9:26 AM
@Oleksii Malovanyi I have a rough implementation in the
test-overhaul
branch that you might want to look at. TL;DR “blocking” test mode now completely circumvents orbit internal dispatching (which is well tested anyway). Actually it’s not “blocking” at all any more - you invoke an
intent
as a suspending function. This means you can test it exactly as you would a normal suspending function e.g. using
runBlockingTest
. Without any dispatcher related magic necessary. Let me know what you think.
the way this works is that we intercept calls to
Container.orbit
and send lambdas to a channel which then runs the suspending lambdas in a suspend function
less magic than I thought it would be
in other words - testing the contents of the
intent
block.
o

Oleksii Malovanyi

06/22/2021, 12:42 PM
Hi @Mikolaj Leszczynski First of all, I wanna say thanks for your effort. I’ve run tests locally and it works – that’s amazing 🎉 The overall API looks okay for me, despite my own preference would still be smth builder-like (host.test().assert()), but this is something that could be sugared. Also, I’m a bit warned by the
withTimeout
call, but I realise it to be a defensive tactics we have to make due to async test/assert calls
m

Mikolaj Leszczynski

06/22/2021, 1:08 PM
@Oleksii Malovanyi the
withTimeout
is there just to guard against someone not actually calling any method on the containerHost in
testIntent
. This implementation is very rough around the edges right now anyway 🙂 There are a few issues that still need solving. I’ll keep working on this this week and tag you on the PR once I’m done. What do you think about the general approach of using suspending functions for tests?
re: builder style - I’ll make sure that
testIntent
returns the test container host so you can chain it in a builder-like pattern
our own preference is for
arrange/act/assert
but I don’t see why we can’t have both 😉
o

Oleksii Malovanyi

06/22/2021, 1:13 PM
yeap, I guess it depends on the testing style 👍
overall I like suspending functions much more as it explicitly tells the client about the contract
👍 1
m

Mikolaj Leszczynski

06/22/2021, 1:16 PM
thanks for your feedback, glad it’s an improvement 🙂
I got feedback from @appmattus as well, he seems happy with it so I’ll continue on this course. I just have to take care of the corner cases and make sure everything works
metal 1
FYI, if you want to take a look
👀 1
o

Oleksii Malovanyi

06/24/2021, 8:54 AM
It looks really promising 🤘 I’ve left couple minor comments
m

Mikolaj Leszczynski

06/24/2021, 8:59 AM
👍
I’ve responded to everything
2 Views