Skip to content

keep dispatcher and scope config internal #36

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

Merged
merged 6 commits into from
Sep 13, 2021

Conversation

wenxi-zeng
Copy link
Contributor

Changes:

  • remove dispatchers and scope from Configuration constructors, keep them internal
  • fix broken tests

Additional Changes

  • temporary fix the blocking issue caused by sovran

@wenxi-zeng wenxi-zeng changed the title keep dispatcher config interal keep dispatcher and scope config internal Sep 13, 2021
@wenxi-zeng wenxi-zeng requested a review from prayansh September 13, 2021 15:47
@@ -364,7 +345,7 @@ class AndroidLifecyclePluginTests {
// Simulate activity startup
lifecyclePlugin.onActivityCreated(mockActivity, mockBundle)

assertTrue(mockPlugin.ran)
verify (timeout = 2000){ mockPlugin.updateState(true) }
Copy link
Contributor

Choose a reason for hiding this comment

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

is this timeout necessary due to us not being able to run the tests with the testDispatcher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct. because we are not able to use testDispatcher, things running async. there is a delay before the update happen. the verify waits until the statement happened or timeout reached, whichever one comes first.

internal var ioDispatcher: CoroutineDispatcher = Executors.newFixedThreadPool(1)
.asCoroutineDispatcher()

internal var analyticsScope: CoroutineScope = MainScope()
Copy link
Contributor

Choose a reason for hiding this comment

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

lets make this CoroutineScope(SupervisorJob()) rather than using the main scope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree. I was also worried about it running in main thread. changing it now

@wenxi-zeng wenxi-zeng merged commit f3a545e into main Sep 13, 2021
@wenxi-zeng wenxi-zeng deleted the wenxi/keep-dispatcher-config-interal branch September 13, 2021 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants