-
Notifications
You must be signed in to change notification settings - Fork 531
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 #4632, #3779: Add support for checkpoint upgrading #4696
Conversation
This includes what should be a working implementation, but only specs for verifying it through tests. Manually verifying will be tricky and thus will be done at the end of the PR (and more extensively during release testing).
This error (corresponding to #3779) is noise since it's not actually a real "error."
@seanlip PTAL. As an FYI I have also self-reviewed the whole PR. |
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.
LGTM, but with the disclaimer that I've only been able to take a cursory look and didn't see anything obviously wrong (but I can't certify correctness). Thanks!
...oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointController.kt
Outdated
Show resolved
Hide resolved
Merging this without @rt4914's approval since it's release blocking. |
Explanation
Fixes #4632
Fixes #3779
Adds support for upgrading checkpoints to support the latest versions of explorations if the checkpoint was saved for an older version, in cases where it's very likely reasonable to upgrade the checkpoints. The methodology applied is to simulate "replaying" the full lesson using the past answers that were submitted in the checkpointing, and verifying that the same outcome would more or less be achieved by ensuring:
The cases that currently can't be handled (but may be reasonable to in the future):
As part of testing this:
ExplorationRetriever
needed to be introduced in order to support proxying exploration loading (so that newer versions can be force-loaded). This required splitting the existing ExplorationRetriever into an interface & implementation.ExplorationStorageModule
has been moved one package up for better organization (which led to a bunch of other files being changed).Separately, ExplorationRetriever was updated to mostly force background thread interaction by using a suspend method for actual exploration loading (which led to some small test changes).
#3779 was also fixed (since I was changing this code, anyway) by returning a default details object instead of failing whenever a checkpoint can't be found for a given exploration. Note that no test was added for this solution since doing so would be a bit painful (as it requires loading and verifying an exploration at the activity level where a bunch of the lower-level tools from StateFragment{Local}Test aren't available), and the failure case here is something that will definitely be caught by e2e tests so the risk is low for a serious regression.
Essential Checklist
For UI-specific PRs only
While this PR does affect UI flows for checkpoints, I'm opting to not include a walkthrough since it's difficult and time consuming to simulate the local upgrade flow. We'll be testing this specially during manual QA testing for this release to ensure it works as expected.
Similarly, app module tests haven't changed in a major way so I'm skipping showing Espresso results here. The Robolectric tests results ought to be a sufficiently good proxy for correctness.