-
Notifications
You must be signed in to change notification settings - Fork 499
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 #3674: Added new test cases in PlatformParameterIntegrationTest #3675
Fix #3674: Added new test cases in PlatformParameterIntegrationTest #3675
Conversation
…esting/platformparameter BUILD.bazel
…and Gae models also adding test cases in Service test and documentation over Gae models
…eModels and used a Map<String,Any> instead
…_DEPS of testing module
…ameter Service Impl in Network Module
…tionModule is used
@Arjupta I unfortunately didn't review this before merging the base PR, and trying to review the deltas isn't working very well. Can you please bring this up-to-date with develop & properly incorporate the base deltas so that the PR changes are correct? |
@BenHenning I have updated the PR with develop. You can see your requested changes now |
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.
Thanks @Arjupta. Just 2 nits otherwise the PR LGTM.
utility/src/main/java/org/oppia/android/util/platformparameter/PlatformParameterConstants.kt
Outdated
Show resolved
Hide resolved
app/src/test/java/org/oppia/android/app/testing/PlatformParameterIntegrationTest.kt
Show resolved
Hide resolved
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.
Thanks @Arjupta. LGTM barring CI checks.
Edit: also, given that the previous run passed on Bazel checks I'm going to enable auto-merge since the deltas are just comment changes.
app/src/test/java/org/oppia/android/app/testing/PlatformParameterIntegrationTest.kt
Show resolved
Hide resolved
Unassigning @BenHenning since they have already approved the PR. |
Hi @Arjupta, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
Explanation
Fixed #3674
Updated PlatformParameterIntegrationTest to include test based on refreshing with SyncUpWorker.
Target PR date: 11 August 2021
Target completion date: 16 August 2021
Checklist