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

[Bug] The getter for WorkManager considers the app context #2122

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Jun 12, 2024

Description

One Line Summary

Allow custom work manager to be initialized on demand, which resolves Hilt dependency manager conflict.

Details

  • With helpful comments from the community and revisiting WorkManager source code.
  • We were mimicking the isInitialized() API that checks for a WorkManager instance via WorkManagerImpl.getInstance() and initializing it with a blank Configuration when the getter returned null.
  • However, we should use WorkManagerImpl.getInstance(Context) instead as this getter will allow for on-demand initialization with the app's custom WorkManager.
  • Because Hilt dependency manager or other app configurations such as using lateinit may not have the WorkManager instance initialized at the time we check.

Motivation

Scope

Work Manager initialization.

Testing

Unit testing

  • None due to limited ability to reproduce original crashes
  • Except to run a bare unit test that experiences the original "not found" crash when given a unit test context.
  • Then using the OSWorkManagerHelper's getter instead does not crash and returns a new Work Manager instance.
test("start application service with non-activity shows entry state as closed") {
    // Crashes
    WorkManager.getInstance(ApplicationProvider.getApplicationContext<Context>())

    // Works
    OSWorkManagerHelper.getInstance(ApplicationProvider.getApplicationContext<Context>())
}

Manual testing

Limited ability to test manually due to unable to reproduce original crash

  • Tested normal app flow in an emulator
  • Receiving notifications in background and swiped away state
  • Opening app, etc.
  • Receive receipt worker runs and succeeds

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@nan-li nan-li requested review from jkasten2 and emawby June 12, 2024 20:37
* We were getting the WorkManager instance via `WorkManagerImpl.getInstance()` and initializing it with a blank Configuration when the getter returned null.
* However, we should use `WorkManagerImpl.getInstance(Context)` instead as this getter will allow for on-demand initialization with the app's custom WorkManager.
@nan-li nan-li force-pushed the fix/work_manager_uses_context branch from fac32f5 to c4b664c Compare June 13, 2024 18:00
@nan-li nan-li merged commit d4a2a69 into main Jun 13, 2024
2 checks passed
@nan-li nan-li deleted the fix/work_manager_uses_context branch June 13, 2024 21:26
@nan-li nan-li mentioned this pull request Jun 18, 2024
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.

[Bug]: Version 4.8.5 breaks Hilt dependency injection of CoroutineWorkers
2 participants