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.

Sign up for GitHub

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

Jump to bottom

Introduce test module with testable main dispatcher #749

Merged
merged 10 commits into from
Dec 17, 2018
Merged

Introduce test module with testable main dispatcher #749

merged 10 commits into from
Dec 17, 2018

Conversation

qwwdfsad
Copy link
Collaborator

@qwwdfsad qwwdfsad commented Oct 22, 2018 •
edited
Loading

I am expecting this module to be developed in the nearest future, adding support for a virtual time, our expect-like mechanism and debug agent.

@qwwdfsad

This comment has been minimized.

Sign in to view
elizarov
elizarov suggested changes Oct 23, 2018
View reviewed changes
Copy link
Contributor

@elizarov elizarov left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API needs careful design. I personally don't like the public API to mutate the dispatcher -- neither its name MainDispatcherInjector (it does not inject anything, it just sets the global static variable), nor the very fact of its existence, even if this is only a test module.

Let's discuss the actual use-cases and see if they can be covered in a static way with an implementation of dispatcher that provides the necessarily functionality without exposing globally-mutating API.

@qwwdfsad qwwdfsad force-pushed the develop branch 5 times, most recently from 69dc390 to eaf9b7c Compare October 25, 2018 10:41
@qwwdfsad qwwdfsad force-pushed the develop branch 3 times, most recently from bc68d63 to 3179683 Compare November 12, 2018 11:49
@qwwdfsad
Copy link
Collaborator Author

Meanwhile #810

@elizarov elizarov mentioned this pull request Nov 14, 2018
Closed
@qwwdfsad
Do not use field initializers that can throw and leave HandlerDispatc…
e214c93 
…her in uninitialized state, use Dispatchers.Main everywhere.

Deprecate HandlerDispatcher.Main (was accessible only fom Java API)
@qwwdfsad
Re-implement main dispatcher replacement
ed0221b 
  * Make it extension on Dispatchers object
  * Move implementation details of Dispatchers to internal package
  * Try to instantiate original dispatcher after reset, do not cache failures
  * Add module with Android tests which use both Unconfined and Robolectrics
  * Rename module to coroutines-test
@qwwdfsad qwwdfsad changed the title Introduce test module with ability to inject custom Main dispatcher Introduce test module with testable main dispatcher Dec 7, 2018
@qwwdfsad
Copy link
Collaborator Author

qwwdfsad commented Dec 7, 2018

Feeling slightly sick, will proofread it again tomorrow/on Monday

@qwwdfsad qwwdfsad removed the design label Dec 7, 2018
objcode
objcode reviewed Dec 7, 2018
View reviewed changes
Copy link
Contributor

@objcode objcode left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

Added some review notes on the current implementation. Overall this solves the issue I raised in #568 very nicely!

I've been getting a lot of questions about how to do this from Android developers and want to share this is a great solution to their problem.

ui/kotlinx-coroutines-android/src/HandlerDispatcher.kt Show resolved Hide resolved
core/kotlinx-coroutines-test/src/internal/MainTestDispatcher.kt Outdated Show resolved Hide resolved
core/kotlinx-coroutines-test/src/internal/MainTestDispatcher.kt Outdated Show resolved Hide resolved
core/kotlinx-coroutines-core/src/internal/MainDispatchers.kt Show resolved Hide resolved
core/kotlinx-coroutines-test/src/internal/MainTestDispatcher.kt Show resolved Hide resolved
core/kotlinx-coroutines-test/src/TestDispatchers.kt Show resolved Hide resolved
...otlinx-coroutines-android/android-unit-tests/test/ordered/tests/CustomizedRobolectricTest.kt Outdated Show resolved Hide resolved
@objcode objcode mentioned this pull request Dec 7, 2018
Closed
@qwwdfsad
Copy link
Collaborator Author

qwwdfsad commented Dec 10, 2018 •
edited
Loading

@objcode thanks for the review!

@qwwdfsad
Test dispatcher improvements
c884fb3 
  * TestDispatchersKt -> TestDispatchers
  * Advice to use Dispatchers.setMain if AndroidDispatcherFactory throws an exception
  * Fail fast in all MissingCoroutineDispatcher methods
@qwwdfsad
Delegate delay-related calls in MainTestDispatcher to its delegate
a3c2a90
@qwwdfsad
Documentation fixes
84485dd
@qwwdfsad qwwdfsad mentioned this pull request Dec 12, 2018
Closed
@qwwdfsad
Copy link
Collaborator Author

TODO: add dependency to kotlinx-coroutines-debug before merging this

@objcode objcode mentioned this pull request Dec 14, 2018
Closed
@qwwdfsad
Merge branch 'develop' into test-module
20aa631 
# Conflicts:
#	README.md
#	RELEASE.md
#	binary-compatibility-validator/build.gradle
#	core/README.md
#	knit/resources/knit.properties
#	settings.gradle
@qwwdfsad
Post-merge
de9150a
@qwwdfsad qwwdfsad merged commit 4327b21 into develop Dec 17, 2018
@qwwdfsad qwwdfsad deleted the test-module branch December 17, 2018 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@objcode objcode objcode left review comments

@dmytrodanylyk dmytrodanylyk dmytrodanylyk left review comments

@elizarov elizarov elizarov requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@qwwdfsad @objcode @elizarov @dmytrodanylyk