Fixes #1198 & 1200: Options - Tablet (Landscape) (Portrait) (Lowfi) #1677

MohamedMedhat1998 · 2020-08-19T11:11:27Z

Important Note

The newly added test cases in StoryTextSizeFragmentTest,DefaultAudioFragmentTest, and AppLanguageFragmentTest don't pass on robolectric, but when they are copied to espresso, they pass.

Explanation

Fixes Options - Tablet (Landscape) (Lowfi) #1198
Fixes Options - Tablet (Portrait) (Lowfi) #1200
Implemented low-fi UI for Options for tablet landscape and portrait mode
Mocks: portrait - landscape

Screenshots

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.

rt4914

Really nice implementation.
I am unable to run test cases but on glance they look correct. Still I will run them in second attempt but meanwhile make sure that your tests pass on Github Actions first.

app/src/main/java/org/oppia/app/options/OptionControlsViewModel.kt

app/src/main/java/org/oppia/app/options/OptionsActivity.kt

app/src/main/java/org/oppia/app/options/OptionsFragmentPresenter.kt

MohamedMedhat1998 · 2020-08-20T10:02:51Z

Really nice implementation.
I am unable to run test cases but on glance they look correct. Still I will run them in second attempt but meanwhile make sure that your tests pass on Github Actions first.

@rt4914 The test cases fail on Robolectric but pass on Espresso (when they are copied there).
So, the PR itself contains newly added test cases that fail, is it what causing the tests to fail on github actions?

rt4914 · 2020-08-21T12:19:25Z

Really nice implementation.
I am unable to run test cases but on glance they look correct. Still I will run them in second attempt but meanwhile make sure that your tests pass on Github Actions first.

@rt4914 The test cases fail on Robolectric but pass on Espresso (when they are copied there).
So, the PR itself contains newly added test cases that fail, is it what causing the tests to fail on github actions?

@MohamedMedhat1998 I was facing similar issue on #1682 and when I merged with latest develop and the test cases are not passing on CI. So can you please update it with latest develop and after that can easily investigate if we still face any issue.

MohamedMedhat1998 · 2020-08-22T11:22:59Z

Really nice implementation.
I am unable to run test cases but on glance they look correct. Still I will run them in second attempt but meanwhile make sure that your tests pass on Github Actions first.

@rt4914 The test cases fail on Robolectric but pass on Espresso (when they are copied there).
So, the PR itself contains newly added test cases that fail, is it what causing the tests to fail on github actions?

@MohamedMedhat1998 I was facing similar issue on #1682 and when I merged with latest develop and the test cases are not passing on CI. So can you please update it with latest develop and after that can easily investigate if we still face any issue.

@rt4914 I have updated with the latest develop.
Please check the tests in StoryTextSizeFragmentTest,DefaultAudioFragmentTest, and AppLanguageFragmentTest because some of them are failing on robolectric, but when they are moved to espresso, they work.

rt4914 · 2020-08-28T12:09:12Z

OptionsFragmentPresenter

      getOptionControlsItemViewModel().getAppLanguage(AppLanguage.ENGLISH_APP_LANGUAGE) -> {
        profileManagementController.updateAppLanguage(
          profileId,
          AppLanguage.ENGLISH_APP_LANGUAGE
        ).observe(fragment, Observer { appLanguageResult ->
          if (appLanguageResult.isSuccess()) {
            appLanguage = AppLanguage.ENGLISH_APP_LANGUAGE
          }
        })
      }

rt4914 · 2020-08-28T12:13:44Z

getOptionControlsItemViewModel().getAppLanguage(AppLanguage.ENGLISH_APP_LANGUAGE) -> {
profileManagementController.updateAppLanguage(
profileId,
AppLanguage.ENGLISH_APP_LANGUAGE
).observe(fragment, Observer { appLanguageResult ->
if (appLanguageResult.isSuccess()) {
appLanguage = AppLanguage.ENGLISH_APP_LANGUAGE
}
})
}

@MohamedMedhat1998 Next steps / commits:

Update test cases to use initialiseProfiles
Resolve merge conflicts.
Update implementation to observe results.
You might need to Recheck test cases after step 3.

MohamedMedhat1998 · 2020-08-28T12:34:03Z

Out of curiosity, how have these tests been tested on Espresso? Given that they're in the test/ folder, they're not actually configured to run with instrumentation (and when I try, I get 'No tests were found' as expected for a non-shared test suite).

I just copied the tests from Robolectric files to Espresso files :D

Edit: also, how did you use testCoroutineDispatchers if DefaultAudioFragmentTest isn't an injected test?

I didn't use testCoroutineDispatchers while I was trying on Espresso

MohamedMedhat1998 · 2020-08-28T12:45:54Z

* The click to change the language is changing the audioLanguage in `OptionsFragmentPresenter` (by the way, why do we need this variable?) and is initiating an update for the profile

Because when we are on a tablet device, the AudioLanguageActivity isn't started, so we update the language using the OptionsFragmentPresenter instead of AudioLanguageActivityPresenter

* Note that the notify data set changed is wrong here since the adapter list hasn't actually changed yet--that's contingent on the view model's livedata observer getting called back

I will fix that using Rajat snippet in his comment.

* It seems like the test isn't actually setting up or logging into any profiles--this is putting the profile system in an invalid state (**action item**: you should add observers to _all_ live datas returned by the profile controller's update methods since these will tell you if they pass/fail--they're clearly failing here. I suggest adding log statements for the cases when they fail.)

I think this will be solved also using the code Rajat added in his comment.

I suspect that the Espresso runs of these tests only pass because you already had profiles setup on the local device. Robolectric is more correct here: it's always running the tests in a clean state, uncovering the test setup issue. Restructuring your test like so fixes it for me:

https://gist.github.com/BenHenning/0582c3bc1f8cb03917b030dc8238f075/revisions

Thanks a lot, Ben. This really helped.

app/src/main/java/org/oppia/app/options/OptionsFragmentPresenter.kt

# Conflicts (resolved): # app/src/main/java/org/oppia/app/options/OptionControlsViewModel.kt # app/src/main/java/org/oppia/app/options/OptionsActivity.kt # app/src/main/java/org/oppia/app/options/OptionsFragment.kt # app/src/main/java/org/oppia/app/options/OptionsFragmentPresenter.kt # app/src/main/java/org/oppia/app/options/OptionsReadingTextSizeViewModel.kt # app/src/main/java/org/oppia/app/options/ReadingTextSizeFragmentPresenter.kt # app/src/sharedTest/java/org/oppia/app/options/OptionsFragmentTest.kt # app/src/test/java/org/oppia/app/testing/options/OptionsFragmentTest.kt # app/src/test/java/org/oppia/app/testing/options/ReadingTextSizeFragmentTest.kt

rt4914 · 2020-08-29T12:50:56Z

app/src/main/java/org/oppia/app/options/OptionsFragmentPresenter.kt

+            } else {
+              consoleLogger.e(
+                READING_TEXT_SIZE_TAG,
+                READING_TEXT_SIZE_ERROR,


You cannot have the same logger everywhere, the error should be different for each logger otherwise if anyone faces this error how would they know which error is related to which function?

consoleLogger.e( READING_TEXT_SIZE_TAG, READING_TEXT_SIZE_ERROR + ": small text size", it.getErrorOrNull() )

app/src/test/java/org/oppia/app/testing/options/AppLanguageFragmentTest.kt

rt4914

Nit change suggested in logger.

rt4914

LGTM, nit change suggested.

app/src/main/java/org/oppia/app/options/OptionsFragmentPresenter.kt

BenHenning

Approval for codeowners for the Bazel changes (didn't look at the rest of the PR in depth).

MohamedMedhat1998 added 4 commits August 18, 2020 14:42

-Implemented multipane options (with some issues)

c6f7dd3

-Fixed the issues that was facing the "OptionsFragment"

006001f

-Added a TextView for the title of the multipane options

379cdb4

-Added test cases for "OptionsFragment"

08d0f51

MohamedMedhat1998 requested a review from rt4914 August 19, 2020 11:11

MohamedMedhat1998 assigned rt4914 Aug 19, 2020

MohamedMedhat1998 added 3 commits August 19, 2020 14:51

-Added test cases for "AppLanguage", "DefaultAudio", and "StoryTextSize"

cbba5db

Merge remote-tracking branch 'origin/develop' into options-tablet-lowfi

5a37741

-Fixed ktlint issues

a0e6089

rt4914 suggested changes Aug 19, 2020

View reviewed changes

rt4914 assigned MohamedMedhat1998 and unassigned rt4914 Aug 19, 2020

MohamedMedhat1998 requested a review from rt4914 August 20, 2020 10:02

MohamedMedhat1998 assigned rt4914 and unassigned MohamedMedhat1998 Aug 20, 2020

MohamedMedhat1998 added 2 commits August 20, 2020 13:05

Merge remote-tracking branch 'origin/develop' into options-tablet-lowfi

809bbcd

-Some nit changes

838e523

rt4914 assigned MohamedMedhat1998 and unassigned rt4914 Aug 21, 2020

Merge remote-tracking branch 'origin/develop' into options-tablet-lowfi

20909c0

MohamedMedhat1998 assigned rt4914 and unassigned MohamedMedhat1998 Aug 22, 2020

rt4914 removed their assignment Aug 28, 2020

MohamedMedhat1998 added 2 commits August 28, 2020 18:57

-updated the failing test cases

13b4857

-some refactoring

b2e89a7

MohamedMedhat1998 assigned rt4914 and unassigned MohamedMedhat1998 Aug 28, 2020

BenHenning reviewed Aug 28, 2020

View reviewed changes

app/src/main/java/org/oppia/app/options/OptionsFragmentPresenter.kt Show resolved Hide resolved

MohamedMedhat1998 added 6 commits August 28, 2020 21:48

-Fixed lint issues

56ba897

-Added "Options" listeners to BUILD.bazel

c2239a7

-Updated "OptionsFragmentPresenter" "update" methods implementation

9c1861f

-Fixed ktlint issues

b93f163

-Added logs in the case of errors in "OptionsFragmentPresenter"

f060712

rt4914 reviewed Aug 29, 2020

View reviewed changes

rt4914 suggested changes Aug 29, 2020

View reviewed changes

rt4914 assigned MohamedMedhat1998 and unassigned rt4914 Aug 29, 2020

MohamedMedhat1998 assigned rt4914 and unassigned MohamedMedhat1998 Aug 29, 2020

rt4914 approved these changes Aug 29, 2020

View reviewed changes

app/src/main/java/org/oppia/app/options/OptionsFragmentPresenter.kt Outdated Show resolved Hide resolved

rt4914 assigned MohamedMedhat1998 and unassigned rt4914 Aug 29, 2020

MohamedMedhat1998 added 2 commits August 29, 2020 17:00

-updated the error logs in "OptionsFragmentPresenter"

0981187

-ktlint issues

8114802

BenHenning approved these changes Aug 29, 2020

View reviewed changes

MohamedMedhat1998 merged commit b11cf58 into develop Aug 29, 2020

MohamedMedhat1998 deleted the options-tablet-lowfi branch August 29, 2020 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1198 & 1200: Options - Tablet (Landscape) (Portrait) (Lowfi) #1677

Fixes #1198 & 1200: Options - Tablet (Landscape) (Portrait) (Lowfi) #1677

MohamedMedhat1998 commented Aug 19, 2020

rt4914 left a comment

MohamedMedhat1998 commented Aug 20, 2020

rt4914 commented Aug 21, 2020

MohamedMedhat1998 commented Aug 22, 2020

rt4914 commented Aug 28, 2020

rt4914 commented Aug 28, 2020

MohamedMedhat1998 commented Aug 28, 2020

MohamedMedhat1998 commented Aug 28, 2020

rt4914 Aug 29, 2020

MohamedMedhat1998 Aug 29, 2020

rt4914 left a comment

rt4914 left a comment

BenHenning left a comment

Fixes #1198 & 1200: Options - Tablet (Landscape) (Portrait) (Lowfi) #1677

Fixes #1198 & 1200: Options - Tablet (Landscape) (Portrait) (Lowfi) #1677

Conversation

MohamedMedhat1998 commented Aug 19, 2020

Important Note

Explanation

Screenshots

Checklist

rt4914 left a comment

Choose a reason for hiding this comment

MohamedMedhat1998 commented Aug 20, 2020

rt4914 commented Aug 21, 2020

MohamedMedhat1998 commented Aug 22, 2020

rt4914 commented Aug 28, 2020

rt4914 commented Aug 28, 2020

MohamedMedhat1998 commented Aug 28, 2020

MohamedMedhat1998 commented Aug 28, 2020

rt4914 Aug 29, 2020

Choose a reason for hiding this comment

MohamedMedhat1998 Aug 29, 2020

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment