Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[android_alarm_manager] Fixed issue where callback lookup was failing in the background #2453

Merged
merged 1 commit into from
Jan 8, 2020

Conversation

bkonyi
Copy link
Contributor

@bkonyi bkonyi commented Jan 8, 2020

If callback lookup was attempted before Flutter was initialized, the callback cache would not yet be populated resulting in a failed callback lookup.

Fixes flutter/flutter#47406

the background

If callback lookup was attempted before Flutter was initialized, the
callback cache would not yet be populated resulting in a failed callback
lookup.
Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

Any way to test this? I get that this is inherently kind of tricky to try and e2e test since it needs to be backgrounded at some point. Would it be possible to unit test FlutterBAckgroundExecutor#startBackgroundIsolate? All PRs need to have automated tests before we can merge them into the tree, see Tree Hygiene #4.

@bkonyi
Copy link
Contributor Author

bkonyi commented Jan 8, 2020

Any way to test this? I get that this is inherently kind of tricky to try and e2e test since it needs to be backgrounded at some point. Would it be possible to unit test FlutterBAckgroundExecutor#startBackgroundIsolate? All PRs need to have automated tests before we can merge them into the tree, see Tree Hygiene #4.

I honestly wouldn't know how to go about doing that since my Android testing experience is non-existent. We'd need to be able to run the application in the foreground once to initialize state, kill it, and then wait for it to come up.

@mklim
Copy link
Contributor

mklim commented Jan 8, 2020

@bkonyi I typed up a long comment explaining how to write Java unit tests in our repo, looked at the method more closely, and then realized that it's doing way too much work with JNI and global static state to be really unit tested anyway. Disregard that suggestion. :)

We'd need to be able to run the application in the foreground once to initialize state, kill it, and then wait for it to come up.

I think we could potentially do this in a Dart e2e test. Check out camera_e2e_test.dart, where we use adb from the test runner to grant the test app permissions before it runs. What if we killed the test app with adb shell force-stop from the test runner and then asserted that it came back after some amount of time? I'm thinking that should work but I could be missing something here, WDYT?

@bkonyi
Copy link
Contributor Author

bkonyi commented Jan 8, 2020

I think we could potentially do this in a Dart e2e test. Check out camera_e2e_test.dart, where we use adb from the test runner to grant the test app permissions before it runs. What if we killed the test app with adb shell force-stop from the test runner and then asserted that it came back after some amount of time? I'm thinking that should work but I could be missing something here, WDYT?

Hm... I think that would work. Let me hack something together and give it a shot.

@Hixie
Copy link
Contributor

Hixie commented Jan 8, 2020

You know, the thing that would be really good to test is whether it comes back after a reboot. I don't suppose we can reboot the phone as well during the test? :-)

Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

LGTM

Discussed offline, we don't quite have the capabilities to test this yet, but we should be able to once we have better support for android integration tests. We've got an exemption to land this as-is with a TODO to test it once we have the capability (flutter/flutter#38983).

@bkonyi
Copy link
Contributor Author

bkonyi commented Jan 8, 2020

Filed an issue to add tests when possible: flutter/flutter#48439

@bkonyi bkonyi merged commit de56da5 into master Jan 8, 2020
@bkonyi bkonyi deleted the alarm_manager_callback_lookup_fix branch January 8, 2020 20:50
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
… in the background (flutter#2453)

If callback lookup was attempted before Flutter was initialized, the
callback cache would not yet be populated resulting in a failed callback
lookup.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

android_alarm_manager no longer works in background
4 participants