Skip to content
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

Async code testing paradigm is broken and ugly #2977

Open
virl opened this issue Mar 15, 2017 · 5 comments
Open

Async code testing paradigm is broken and ugly #2977

virl opened this issue Mar 15, 2017 · 5 comments
Labels
scheduler Scheduler rewrite + cleanup

Comments

@virl
Copy link

virl commented Mar 15, 2017

Robolectric have very broken and ugly async code testing paradigm, because it doesn't allow execution of implementation's hidden loopers in the test (the loopers that test have no direct references to).

The real situation is even worse than that, because normal Threads/Executors are still running anyway and not paused.

As the result, it is almost impossible to write a test for complex async code system that tests it as blackbox, without requiring the test to depend on all code's implementation details (specific loopers).

Robolectric should behave like Xcode's XCTest in that regard: allowing to unpause ALL loopers in the app with call of single method like .unpauseAllLoopers() and executing code on them in exactly same manner (on separate threads, etc.) as it behaves in real app.
Frankly, this behaviour should be default and must not require even calling a method for this.

Such "unpausing" of loopers (especially if it it will be default behaviour, which it must be) should will execute as normal any Runnables submitted even after unpause.
Because current behaviour of unpauseLooper() when it doesn't execute any newly submitted Runnables is major bug.

And of course the Scheduler/ShadowLooper/etc. implementation should not substitute delayed (in next event loop interation) execution of runnables with inline/instant execution of them (on the same call stack), because it introduces major out of order execution errors!

Best solution would be to completely deprecate ShadowLooper mechanism and introduce analogue to XCTestExpectation: a condition objects that you can wait on from test's runloop and which will spin current runloop until condition or timeout is satisfied from another thread/runloop asynchronously.
But these expectations should not require the test to explicitly know about internal loopers of tested code — they should spin/await only test's current looper to allow waiting for events happening on that looper (and also for event on any other looper or thread without pausing it).

How it is properly implemented in iOS's XCTest framework: XCTestExpectation

To summarize, Robolectric makes a major mistake by deciding that test's multithreading reproducibility can be achieved by freezing of all threads and step-by-step iteration over their runnables — because you can't do that with Threads anyway and because for Loopers it prevents testing any minimally complex classes as black boxes and introduces out-of-order execution errors during tests.
Instead Robolectric should run async code in tests as it is in real app and provide developer strong API tools to check async expectations about its execution result, as Xcode's XCTest does.

Also see #1993 #1994 #1727 #1711 #2851 #2149 #1879
For current paradigm's ugly consequences see #3369 #3359 #3193 #3270 #2961 #2957 #2958 #2205 #2204 #1306 #2534 #3234 #3188
For failed workaround attempts (without changing the broken paradigm itself) see #3369 #2166 #2119 #2122 #2116

@mikesol
Copy link

mikesol commented Sep 5, 2017

Thanks for doing this research!
As you reference a couple bugs I reported, I thought I'd add a workaround attempt that I've done with pretty good results. I use cucumber to coordinate large-ish tests. Aside from the benefits it brings in human readability, it also allows for things to be parallelized - a CI pipeline ships off artifacts to parallel testing servers that run a test in an isolated environment and merge the result down the pipeline. This is, of course, a workaround, but at the end of the day it is a quite practical one that covers business-critical test cases and allows us to ship with confidence.

@virl
Copy link
Author

virl commented Sep 5, 2017

@mikesol I think you're talking about different, but partially related problem.
This issue is about Robolectric's broken API for testing modern code due to Robolectric's fundamentally wrong multithreading architecture.

Your #3359 related to this in the sense that Robolectric's concurrency and scalability problems, being a different issue, have the same root in its wrong multithreading architecture that tries to "Freeze the World" in futile attempt to make the simplest of UI tests totally deterministic.

@emartynov
Copy link

Let me drop my 5 cents here. Below is IMO.

Using Robolectric for integration tests is probably a bad choice. The Robolectric is mainly used to test single class integration with Android system. In such tests, it is easy and reasonable to assume that multithreading could be replaced/tested by sequential execution runnables.

The real tests for the system integration and maybe e2e (if you prefer) are instrumental tests. no matter what they are hard to write, maintain and execute.

@virl
Copy link
Author

virl commented Apr 19, 2018

@emartynov No, it is not so: even for single class multithreading is implementation detail, even if that class have only synchronous public API.
That is because class can (and will) accomplish intermediate parallel tasks to execute even for synchronous request. Or that class can use arbitrary third-party library (which is implementation detail too that tests should not rely on) that will use multithreading.

The Robolectric is mainly used to test single class integration with Android system. In such tests, it is easy and reasonable to assume that multithreading could be replaced/tested by sequential execution runnables

Multithreading can never be replaced or tested by sequential execution, because nor JVM, nor Android make such guarantees. JVM even have Java Memory Model that guarantees only happens-before execution barriers, not any deterministic behaviour between multiple threads without locks.

Therefore, replacing async testing with sequential runnables violates incapsulation of both tested code (tests will break when correct implementation changed to other correct implementation) and system API guarantees.

@jongerrish jongerrish added the scheduler Scheduler rewrite + cleanup label Nov 2, 2018
@AstralStorm
Copy link

AstralStorm commented Jun 4, 2019

How do we actually ensure all HandlerThreads do run? There should be some sort of barrier-like API to simulate old behavior.
Likewise, some API to allow running all loopers if it's force paused...

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

No branches or pull requests

5 participants