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

Fix #3396 and #3397: Implement Show All Hints and Solution Feature #3705

Merged
merged 83 commits into from
Aug 22, 2021

Conversation

yashraj-01
Copy link
Contributor

@yashraj-01 yashraj-01 commented Aug 19, 2021

Explanation

Fixes #3396.
Fixes #3397.
Implemented feature to show all hints and solution.

Screenshots

  • When the feature is disabled:
Untitled.mp4
  • When the feature is enabled:
Untitled.1.mp4
  • Espresso tests passing for DeveloperOptionsFragmentTest
    Screenshot from 2021-08-19 06-26-59

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@yashraj-01
Copy link
Contributor Author

Thanks @yashraj-01! Per CI it seems HintsAndSolutionDebugModuleTest is failing. Please be sure to double check all changed tests are passing locally before sending the PR back for review to save time waiting on CI.

Overall, the PR looks pretty close to LGTM but I had a few more suggestions & nits. PTAL.

Yes. I just saw that the test was failing. It was due to the change in FactoryDebugImpl. I'll correct the tests in a few minutes. Apologies for the delay.

@oppiabot
Copy link

oppiabot bot commented Aug 21, 2021

Unassigning @yashraj-01 since a re-review was requested. @yashraj-01, please make sure you have addressed all review comments. Thanks!

Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not codeowner but reviewed my last test function name suggestion.

@yashraj-01
Copy link
Contributor Author

Not codeowner but reviewed my last test function name suggestion.

Thanks @anandwana001. Unassigning you now.

@yashraj-01
Copy link
Contributor Author

org.oppia.android.domain.platformparameter.syncup.PlatformParameterSyncUpWorkerTest > testSyncUpWorker_databaseIsNotEmpty_getCorrectPlatformParameters_verifyValuesAreUpdated FAILED expected: SUCCEEDED but was : RUNNING at org.oppia.android.domain.platformparameter.syncup.PlatformParameterSyncUpWorkerTest.testSyncUpWorker_databaseIsNotEmpty_getCorrectPlatformParameters_verifyValuesAreUpdated(PlatformParameterSyncUpWorkerTest.kt:240)

This test was failing even when I re-ran the tests. I don't think this PR affects this test.

@yashraj-01
Copy link
Contributor Author

There was another failure on Bazel StateFragmentLocalTest. This test was passing before and is passing locally. So, I have re-run the Bazel tests to see if it is broken by this PR or not (though I don't think this PR is affecting StateFragmentLocalTest).

@yashraj-01
Copy link
Contributor Author

There was another failure on Bazel StateFragmentLocalTest. This test was passing before and is passing locally. So, I have re-run the Bazel tests to see if it is broken by this PR or not (though I don't think this PR is affecting StateFragmentLocalTest).

This passed after re-running.

Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yashraj-01. Latest PR LGTM, but it seems there are still CI failures (possibly due to #3714). Could you resolve my last comment & see if a new run fixes CI? Happy to approve once CI is green.

@BenHenning BenHenning assigned yashraj-01 and unassigned BenHenning Aug 21, 2021
@yashraj-01
Copy link
Contributor Author

Thanks @yashraj-01. Latest PR LGTM, but it seems there are still CI failures (possibly due to #3714). Could you resolve my last comment & see if a new run fixes CI? Happy to approve once CI is green.

The CI checks have passed except the failing PlatformParameter test.

Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yashraj-01. LGTM. Re-running the Gradle test to see if it'll pass this time.

@oppiabot
Copy link

oppiabot bot commented Aug 22, 2021

Unassigning @BenHenning since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Aug 22, 2021
@BenHenning BenHenning enabled auto-merge (squash) August 22, 2021 05:04
@BenHenning BenHenning merged commit dc2fe41 into develop Aug 22, 2021
@BenHenning BenHenning deleted the show-all-hints-solution branch August 22, 2021 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement logic to show all hints and solution Introduce Debug Implementation of HintHandler
7 participants