Skip to content
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 exploration player #388

Open
BenHenning opened this issue Nov 16, 2019 · 16 comments
Open

Add tests for exploration player #388

BenHenning opened this issue Nov 16, 2019 · 16 comments
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Issue: Needs Break-down Indicates that an issue is too large and should be broken into smaller chunks. Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Member

BenHenning commented Nov 16, 2019

Per #165 and #163 we did not thoroughly test the exploration & state experiences due to being blocked on #89 and #59. We need to complete this as it's essential to stabilize the exploration player and prevent regressions.

Test reference: https://github.com/oppia/oppia-android/blob/develop/app/src/sharedTest/java/org/oppia/app/player/state/StateFragmentTest.kt#L206.

UPDATE: This is not blocked anymore and anyone can start working on tests.

@BenHenning
Copy link
Member Author

This should include thoroughly test each supported interaction, including for its placeholders and hint text.

@BenHenning
Copy link
Member Author

From #410: Make sure we test the shortening of submitted answers.

@BenHenning BenHenning added this to the Beta milestone Jun 23, 2020
BenHenning added a commit that referenced this issue Aug 13, 2020
This does a bunch of refactoring & hacky workarounds to keep asset
priming support fully isolated (just a few files now need to be deleted
to clean things up). This also:
1. Fixes actual asset downloading (the GCS asset path templates have
changed, so priming didn't actually work anymore).
2. Disables audio file caching since we can't play audio when offline,
anyway (there's a dialog that prevents this in-player).
3. Fixes #1340 and #1341 by accounting for error cases when trying to
play audio. It turns out that playing audio crashes if you didn't have
internet access when going into an exploration (the no connectivity
dialog only appears if you lose connectivity within a lesson). There's
also some issues with existing view model code that wrongly assumed
audio couldn't be in a failure state at that point. This has been fixed.
4. Moves the list of topics to cache to be in the same position as the
flag enabling/disabling this functionality.

The download experience isn't perfect, but it's meant to be a helper for
user study coordinators so that they know when lessons can be brought
offline.

Note that the UI aspects of this change are really hacky. This is by
design--I didn't want to overcomplicate the solution, and I wanted to
keep the priming changes fully isolated to make future cleanup easier.

Finally, no new tests were added. I clarified in StateFragmentTest that
the edge cases fixed in this PR need to have corresponding tests, but
I'm actually not sure offhand how to test that audio's playing. I think
this will require additional work. I prefer to push this off to #388,
but I will follow up with tests if anyone wants to push back on this.
BenHenning added a commit that referenced this issue Aug 17, 2020
…line support (#1636)

* Show a dialog when preloading assets for offline support.

This does a bunch of refactoring & hacky workarounds to keep asset
priming support fully isolated (just a few files now need to be deleted
to clean things up). This also:
1. Fixes actual asset downloading (the GCS asset path templates have
changed, so priming didn't actually work anymore).
2. Disables audio file caching since we can't play audio when offline,
anyway (there's a dialog that prevents this in-player).
3. Fixes #1340 and #1341 by accounting for error cases when trying to
play audio. It turns out that playing audio crashes if you didn't have
internet access when going into an exploration (the no connectivity
dialog only appears if you lose connectivity within a lesson). There's
also some issues with existing view model code that wrongly assumed
audio couldn't be in a failure state at that point. This has been fixed.
4. Moves the list of topics to cache to be in the same position as the
flag enabling/disabling this functionality.

The download experience isn't perfect, but it's meant to be a helper for
user study coordinators so that they know when lessons can be brought
offline.

Note that the UI aspects of this change are really hacky. This is by
design--I didn't want to overcomplicate the solution, and I wanted to
keep the priming changes fully isolated to make future cleanup easier.

Finally, no new tests were added. I clarified in StateFragmentTest that
the edge cases fixed in this PR need to have corresponding tests, but
I'm actually not sure offhand how to test that audio's playing. I think
this will require additional work. I prefer to push this off to #388,
but I will follow up with tests if anyone wants to push back on this.

* Lint fixes.

* Introduce new caching module to simplify tests, and fix app module
tests.

* Lint fixes.
@BenHenning BenHenning changed the title Add tests for exploration player [Blocked: #89] Add tests for exploration player Sep 24, 2020
@BenHenning
Copy link
Member Author

Enough of #89 is now complete that this is no longer blocked.

@BenHenning BenHenning added issue_user_developer Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Z-ibt Temporary label for Ben to keep track of issues he's triaged. and removed Impact: Low Low perceived user impact (e.g. edge cases). labels Sep 16, 2022
@BenHenning BenHenning removed this from the Beta MR2 milestone Sep 16, 2022
@BenHenning
Copy link
Member Author

@MohitGupta121 will be working on auditing existing tests between StateFragmentTest, StateFragmentLocalTest, ExplorationActivityTest, and ExplorationActivityLocalTest to determine what gaps currently exist.

@MohitGupta121 MohitGupta121 removed their assignment Jan 5, 2023
@MohitGupta121
Copy link
Member

I am not getting proper time to concentrate on this issue as I'm first completing the dark mode issues for our next release that's why I unassigned myself this issue.
When I free from all other issues then I concentrate on this issue.
Thanks.

@BenHenning
Copy link
Member Author

Sounds good--thanks @MohitGupta121!

@seanlip seanlip added bug End user-perceivable behaviors which are not desirable. and removed issue_type_infrastructure labels Mar 28, 2023
@adhiamboperes adhiamboperes added Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. work label added labels May 3, 2023
@kkmurerwa kkmurerwa removed their assignment May 16, 2023
@kkmurerwa
Copy link
Collaborator

Unassigning myself from this issue because I don't think I will have enough time to complete it.

@adhiamboperes adhiamboperes removed Priority: Essential This work item must be completed for its milestone. Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. labels Mar 13, 2024
@adhiamboperes adhiamboperes added the Issue: Needs Break-down Indicates that an issue is too large and should be broken into smaller chunks. label May 17, 2024
@BenHenning BenHenning added this to the 1.0 Global availability milestone Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Issue: Needs Break-down Indicates that an issue is too large and should be broken into smaller chunks. Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Development

Successfully merging a pull request may close this issue.