https://kotlinlang.org logo
Join Slack
Powered by
Hey guys, I’m kinda dumb, but can’t make it work: ...
# orbit-mvi
o
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
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
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
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
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
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
I mean 
runBlockingTest(myTestCoroutineDispatcherInstance)
m
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
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
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
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
started with the scope or the dispatcher?
o
dispatcher should be
m
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
could the 
assert
func use runBlockingTest under the hood? 🤔
m
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
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
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
YAS, that’s the worse
could you please share your working branch? I’d try to join
m
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
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
@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
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
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
yes, I see this is quite a complex task to juggle the contexts through the structure
m
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
yeap, the only *simplifying I see here is the defaults…
m
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
thanks 🙏 I’ll take a look as well
m
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
maybe, but till it doesn’t conflict
m
I don’t think it does tbh
o
also, I feel like runBlocking or runBlockingTest is defacto standart thing for testing 🤔 but I’m lacking experience here
m
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
@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
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
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
@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
@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
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
I mean I’d like to have a statically safe solutions, not a conventions. test + assert and testBlocking{…} + assert
m
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
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
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
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
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
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
well you wouldn’t touch it normally
but you should be able to change it should you wish to do so
➕ 1
o
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
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
btw, yes 👍
as Android-only dev I’d vote for this 😆
m
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
yes, the testing and dispatchers are really big… hopefully one day it would be available
m
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
🤞 🤞 🤞
m
@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
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
@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
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
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
It looks really promising 🤘 I’ve left couple minor comments
m
👍
I’ve responded to everything
9 Views
Open in Slack
PreviousNext
Powered by