-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix external apps returning data when Collect is being restored #5663
Conversation
collect_app/src/main/java/org/odk/collect/android/activities/FormFillingActivity.java
Show resolved
Hide resolved
0452e51
to
6074f21
Compare
@getodk/testers I'm marking this as "needs testing" early as I'm currently not able to write tests for the fix for the issue or for the new one @grzesiek2010 found:
Please verify that both problems are fixed and also have a go at finding any other related problems around process restores and "external" data. I can look at working out automated testing once we know my fixes actually work! |
What would be the expected result now for the issue #5646? |
I think ideally it should return to the hierarchy in the same place (same group) as it was in before. Do you think that makes sense? Is that currently how it's behaving? |
Yes, it returns to the hierarchy view - for me looks great as it's consistent with other cases of returning to a form |
@dbemke does it look like the issue that @grzesiek2010 reported is fixed as well? It is as far as I can tell! |
I wrote a post going deeper into how all testing works here which might be useful for @grzesiek2010: https://www.seadowg.com/2023/08/14/simulating-activity-recreation-with-robolectric.html. |
Yes. The issue is fixed |
I don't know if the PR should influence also this case. In video widget minimizing the app while recording a video with no background processes settings, after going back the recording is stopped/disappears (no crash and possible to start a new recording, but the recording which was on is not saved). |
I wouldn't expect it to fix this. Does that issue also occur on |
On master version after minimizing it's possible to start a new recording but after tapping "V" to confirm to upload the video, the user is moved to the first page of the form and none of the recordings is saved. In the PR the second recording (the one started after minimizing) is saved. |
So it's changed, but it's (arguably) better now? Sounds like there is still a bug around not being able to record a a video in the background though that we should file separately? |
I think it's better. I'll file the issue after merging this PR. |
Sharing source sets this way is no longer supported: https://issuetracker.google.com/issues/232007221#comment17
We finished testing for now so we'll remove the label "needs testing". As discussed in Slack, please let us know when/if we should continue testing in case of some changes. |
0d4e14f
to
385ebaf
Compare
..._app/src/androidTest/java/org/odk/collect/android/support/rules/FormEntryActivityTestRule.kt
Show resolved
Hide resolved
..._app/src/androidTest/java/org/odk/collect/android/support/rules/FormEntryActivityTestRule.kt
Outdated
Show resolved
Hide resolved
collect_app/src/androidTest/java/org/odk/collect/android/TestSettingsProvider.kt
Outdated
Show resolved
Hide resolved
testshared/src/main/java/org/odk/collect/testshared/EspressoHelpers.kt
Outdated
Show resolved
Hide resolved
Fix external apps returning data when Collect is being restored
Closes #5646
Potentially fixes this crash
As well as fixing the issue above, this fixes the problem discovered by @grzesiek2010:
What has been done to verify that this works as intended?
New (local) tests written to both replace and extend (the now deleted)
ProcessRestoreTest
. These tests use raw Robolectric rather than AndroidX as testing lifecycles like this requires features that AndroidX does not support. As part of writing these tests, I've pulled a few things out ofandroidTest
to shared modules (such astestshared
andandroidtest
for example) so as not to duplicate test support code intest
. I also moved our test forms to a new module that allows local tests to access them (also to prevent duplication).Why is this the best possible solution? Were any other approaches considered?
I've just made the minimum changes required to get the new tests passing. I think there's a lot we could do to make this nicer using the new save instance state/activity result APIs, removing reliance on static state etc, but most of the effort here was getting reliable testing in place which I think has given us a big enough PR.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
As well as the issues this fixes, I think testing form loading/entry in general and having a play with "Don't keep activities" and "Background process limit" would be good.
Before submitting this PR, please make sure you have:
./gradlew checkAll
and confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTest
locally.