-
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
Add tests for QR sharing and import #3886
Conversation
add291c
to
4e839a8
Compare
We expect debug builds not to work on Android < 7 for the same reasons as #3772 (comment). @andrewsiew2 You may be interested to take a look. We'd love your review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through this relatively quickly and focused on the high-level patterns. As noted in the description, the PR is a bit all over the place but it's relatively easy to follow the reasoning going commit by commit so I'm comfortable with it.
I didn't take the time to review the circleci rework in detail. Things look reasonable, I like the references to the CircleCI docs, and I trust that if it doesn't work we'll know immediately.
I also don't know much about Kotlin Coroutines so it's hard for me to evaluate CoroutineScheduler
. It certainly looks straightforward, but I don't know if there are any tradeoffs to consider. @seadowg were there any particular decisions you had to make? Like is Dispatchers.IO the only viable background scope? It would also be good for @grzesiek2010 to take a quick look to know this is coming and have a chance to raise objections or questions. The good news is that with the Scheduler
interface it's not a big deal to swap out the implementation if needed for some reason. Is there any impact on APK size? In the last version we already went up by 1mb for some reason so we should be careful about watching for bumps.
PreferencesProvider
is a positive change to me.
Other than that the QRCodeViewModel and testing changes look good to me with a couple small things inline.
collect_app/src/test/java/org/odk/collect/android/utilities/CachingQRCodeGeneratorTest.java
Outdated
Show resolved
Hide resolved
collect_app/src/test/java/org/odk/collect/android/utilities/CachingQRCodeGeneratorTest.java
Outdated
Show resolved
Hide resolved
collect_app/src/androidTest/java/org/odk/collect/android/support/pages/MainMenuPage.java
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/preferences/qr/QRCodeViewModel.java
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/audio/AudioPlayerViewModel.java
Outdated
Show resolved
Hide resolved
@lognaturel in terms of coroutines I'm honestly not that experienced either. As far as decisions go:
|
I'm satisfied by those answers. It can be iterated on, anyway.
That likely explains the size increase with the last release. Could you please file an issue for the v1.28 milestone? When you make those couple small changes above it's an accept from me. |
@lognaturel ach I made those changes and then didn't push them. My bad. They're up now. |
Best tactic appeared to be to seperate out separate submodules into different steps. Also made quality and test checks run at the same time for faster feedback.
I'm going to set this to "needs testing" as I'm starting to get into problems where I want to work on top of some of this stuff AND other PRs that are in review. @grzesiek2010 if you have any comments happy to chat here or on Slack! @getodk-bot label "needs testing" |
@seadowg I have started testing but unfortunately Collect is crashing when open Configure via QR code from both menu and admin settings on Android 7 and 10 - it is not on Android 5.1 so it is connected to permissions. When I enabled Camera permissions through App settings I was able to open it and the crash was not visible. I have also checked the master where the crash does not occur. |
@kkrawczyk123 ach I see the problem. Pushing a fix now. |
Tested with success! Tested cases:
@getodk-bot unlabel "needs testing" |
This involved a bunch of refactoring to make the QR flows more testable.
This also:
Scheduler
abstraction (using Coroutiner under the hood) to deal with async work (as a replacement for AsyncTask and Rx)PreferencesProvider
object for grabbing general, admin or meta shared preferences without dealing with the shared preferences abstractions (update to STATE.md about this as well)What has been done to verify that this works as intended?
Ran through test lab and CI a whole bunch of times
Why is this the best possible solution? Were any other approaches considered?
I think for this it would be best to bring up questions and discuss rather than run through the different changes here.
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?
There have been changes to both the configure/share QR flow and the audio widget so both would be good to test.
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.