Topic Info Tablet Robolectric Tests [DON'T MERGE] #1480

MohamedMedhat1998 · 2020-07-17T13:15:03Z

Notes

The current test-case fails because of some flickering effect that I noticed when I ran the app on my real device.
Here is the code

  @Test
  fun test() {
    launch(TopicInfoTestActivity::class.java).use {
      it.onActivity { activity ->
        val seeMoreTextView = getSeeMoreTextView(activity)
        val descriptionTextView = getTopicDescriptionTextView(activity)
        assertThat(seeMoreTextView?.text.toString()).isEqualTo("See More")
        assertThat(descriptionTextView?.maxLines).isEqualTo(5)
        seeMoreTextView?.performClick()
        assertThat(descriptionTextView?.maxLines).isEqualTo(12)
        assertThat(seeMoreTextView?.text.toString()).isEqualTo("See Less")
      }
    }
  }

Which means that the text start in expanded state instead of collapsed state.
When I inverted the order of the assertions, the test passed.

  @Test
  fun test() {
    launch(TopicInfoTestActivity::class.java).use {
      it.onActivity { activity ->
        val seeMoreTextView = getSeeMoreTextView(activity)
        val descriptionTextView = getTopicDescriptionTextView(activity)
        assertThat(seeMoreTextView?.text.toString()).isEqualTo("See Less")
        assertThat(descriptionTextView?.maxLines).isEqualTo(12)
        seeMoreTextView?.performClick()
        assertThat(descriptionTextView?.maxLines).isEqualTo(5)
        assertThat(seeMoreTextView?.text.toString()).isEqualTo("See More")
      }
    }
  }

So, is this a problem in the code itself?
If so, should I continue writing these tests tell the problem gets resolved, or there is something else that I can do?

Explanation

Implemented Robolectric-Test for "Topic-Info" Fragment

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.

…See More"

BenHenning · 2020-07-18T03:17:07Z

app/src/main/java/org/oppia/app/testing/TopicInfoTestActivityPresenter.kt

+      TopicInfoFragment.newInstance(internalProfileId = 0, topicId = RATIOS_TOPIC_ID)
+    activity.supportFragmentManager.beginTransaction()
+      .add(R.id.topic_info_container, topicInfoFragment, TopicInfoFragment.TOPIC_INFO_FRAGMENT_TAG)
+      .commit()


You should use commitNow to ensure there's no delay in the UI showing up.

BenHenning

@MohamedMedhat1998 can you include a video of the observed behavior to provide context on the issue you're observing? Also, what have you tried to do to fix this, & what are your thoughts on what's going wrong?

vinitamurthi

Is it possible that the initial set up has something to do with it being created in an expanded state?

vinitamurthi · 2020-07-20T08:22:35Z

app/src/main/res/layout/topic_info_test_activity.xml

+  android:id="@+id/topic_info_container"
+  android:layout_width="match_parent"
+  android:layout_height="match_parent"
+  tools:context=".testing.TopicInfoTestActivity" />


Add a new line at EOF

vinitamurthi · 2020-07-20T08:23:54Z

@BenHenning for some reason this PR is allowed to merge even without a review. Any idea why?

Edit: Nevermind, I just noticed this isn't being merged to develop!

app/src/main/java/org/oppia/app/topic/info/TopicInfoFragment.kt

anandwana001 · 2020-07-20T08:47:36Z

app/src/main/java/org/oppia/app/testing/TopicInfoTestActivityPresenter.kt

+
+  fun handleOnCreate() {
+    activity.setContentView(R.layout.topic_info_test_activity)
+    val topicInfoFragment =


How about putting a check here if a fragment already exist or not? WDYT
if (getTopicInfoFragment() == null)

anandwana001 · 2020-07-20T08:50:35Z

app/src/test/java/org/oppia/app/testing/TopicInfoContentTest.kt

+
+  private fun getTopicDescriptionTextView(activity: TopicInfoTestActivity): TextView? {
+    val topicInfoFragment =
+      activity.supportFragmentManager.findFragmentByTag(TopicInfoFragment.TOPIC_INFO_FRAGMENT_TAG)


How about if we use the placeholder_id rather than TAG? This will remove using extra TAG part.

anandwana001 · 2020-07-20T08:56:48Z

app/src/test/java/org/oppia/app/testing/TopicInfoContentTest.kt

+    Intents.release()
+  }
+
+  private fun getTopicDescriptionTextView(activity: TopicInfoTestActivity): TextView? {


How about if we move these helper methods at the bottom of the file and bringing test cases at the top after @After method? This will make it looks good and if in future some more test cases a consistency will be there that all helper methods are at bottom and test cases in between.

anandwana001 · 2020-07-20T09:03:32Z

app/src/test/java/org/oppia/app/testing/TopicInfoContentTest.kt

+  @Test
+  fun test() {
+    launch(TopicInfoTestActivity::class.java).use {
+      it.onActivity { activity ->


How about if we use scenario -> rather than using it?

anandwana001 · 2020-07-20T09:08:38Z

app/src/test/java/org/oppia/app/testing/TopicInfoContentTest.kt

+  }
+
+  @Test
+  fun test() {


I agree with the Ben comment if you could add some more about the current problem you are seeing for this.
Name of the test case can be more understandable just to understand what the test is all about.

MohamedMedhat1998 · 2020-07-21T20:03:24Z

@MohamedMedhat1998 can you include a video of the observed behavior to provide context on the issue you're observing? Also, what have you tried to do to fix this, & what are your thoughts on what's going wrong?

@BenHenning
What I'm trying to do is testing the see_more TextView in this way:

Start the activity/fragment and check the text inside the see_more TextView
I expect that the text will be "See More", but it was actually "See Less" (according to the test result)
I tried running the application on my real device to observe what happens, and noticed a flickering effect

So, the test case is failing.

Also, what have you tried to do to fix this, what are your thoughts on what's going wrong?

I haven't tried fixing that but I think the initial state of the fragment is set up incorrectly.

Is it possible that the initial set up has something to do with it being created in an expanded state?

That is what I'm thinking too.

BenHenning · 2020-07-22T00:20:53Z

Thanks @MohamedMedhat1998. This seems like the same issue as #1379 to me. The problem is likely that the topic page is in an initial loading state before becoming fully loaded. If this were Robolectric-specific, using TestCoroutineDispatchers would almost certainly fix the issue. Unfortunately, I haven't had time to figure out how to adapt these to an IdlingResource yet (it's not straightforward because Espresso isn't running with a fake clock, and it's not clear to me if it's valid to run background coroutines on a different clock than the wall clock being used by the main thread).

You could try to integrate TestCoroutineDispatchers and make this test first pass with Robolectric, then create a custom IdlingResource to see if it'll work on Espresso. I'm not sure exactly what the custom idling resource will look like yet.

MohamedMedhat1998 · 2020-07-25T11:40:12Z

Thanks @MohamedMedhat1998. This seems like the same issue as #1379 to me. The problem is likely that the topic page is in an initial loading state before becoming fully loaded. If this were Robolectric-specific, using TestCoroutineDispatchers would almost certainly fix the issue. Unfortunately, I haven't had time to figure out how to adapt these to an IdlingResource yet (it's not straightforward because Espresso isn't running with a fake clock, and it's not clear to me if it's valid to run background coroutines on a different clock than the wall clock being used by the main thread).

You could try to integrate TestCoroutineDispatchers and make this test first pass with Robolectric, then create a custom IdlingResource to see if it'll work on Espresso. I'm not sure exactly what the custom idling resource will look like yet.

@BenHenning @rt4914 I have tried using IdlingResources but the test failed for the same reason.
Also I have pushed a new commit that uses TestCoroutineDispatchers but the test case failed for the same reason.
I'm not sure if I'm using it correctly or not, here is the code below

@ExperimentalCoroutinesApi
  @InternalCoroutinesApi
  @Test
  fun secondTest() {
    launchTopicActivityIntent().use {
      testCoroutineDispatchers.advanceUntilIdle()
      onView(withId(R.id.topic_description_text_view))
        .check(
          matches(
            maxLines(
              /* lineCount= */ 5
            )
          )
        )
    }
  }

Error:

Expected: isTextInLines: 5 Got: 12

…olectric-tests # Conflicts (Resolved): # app/src/main/java/org/oppia/app/activity/ActivityComponent.kt -Added some test case

BenHenning · 2020-07-28T02:33:55Z

@MohamedMedhat1998 when you used TestCoroutineDispatchers did you run the test with Robolectric? Given that this is an app module test, I'm not actually expecting it to work without #59 being resolved since we can't properly sync the two different application components being created in app module tests.

MohamedMedhat1998 · 2020-07-28T07:01:06Z

@MohamedMedhat1998 when you used TestCoroutineDispatchers did you run the test with Robolectric? Given that this is an app module test, I'm not actually expecting it to work without #59 being resolved since we can't properly sync the two different application components being created in app module tests.

@BenHenning yes, I was running it with robolectric.

BenHenning · 2020-07-29T01:53:34Z

@MohamedMedhat1998 in that case I suggest migrating the test suite or moving the tests in question to a local test suite like StateFragmentLocalTest & following the same Dagger setup we have there (where we're actually specifying the application class to use via a @Config parameter). A local test with Robolectric & coroutine dispatchers should have very high predictability for all parts of the UI, including the background async loading pieces.

References

https://github.com/oppia/oppia-android/blob/develop/app/src/test/java/org/oppia/app/player/state/StateFragmentLocalTest.kt#L1080
https://github.com/oppia/oppia-android/blob/develop/app/src/test/java/org/oppia/app/player/state/StateFragmentLocalTest.kt#L98 (note that the paused looper mode is important here)

MohamedMedhat1998 · 2020-08-08T14:43:57Z

@MohamedMedhat1998 in that case I suggest migrating the test suite or moving the tests in question to a local test suite like StateFragmentLocalTest & following the same Dagger setup we have there (where we're actually specifying the application class to use via a @Config parameter). A local test with Robolectric & coroutine dispatchers should have very high predictability for all parts of the UI, including the background async loading pieces.

References
* https://github.com/oppia/oppia-android/blob/develop/app/src/test/java/org/oppia/app/player/state/StateFragmentLocalTest.kt#L1080

* https://github.com/oppia/oppia-android/blob/develop/app/src/test/java/org/oppia/app/player/state/StateFragmentLocalTest.kt#L98 (note that the paused looper mode is important here)

I think the main problem we are facing now is the line count in Robolectric tests.
I have posted this question on StackOverflow hoping that someone will have an answer

rt4914 · 2020-09-03T22:15:51Z

@MohamedMedhat1998 Either update/finish this PR or if this is not needed then close this PR.

MohamedMedhat1998 · 2020-09-03T23:21:56Z

closing this PR because it was created to demonstrate the issue with robolectric tests and lineCount of the TextView

MohamedMedhat1998 and others added 5 commits July 10, 2020 13:28

-Implemented high-fi UI for "Topic-Info" for portrait mode - tablet

b404ca6

-Removed "See More" TextView

1a2fba0

-Updated the test cases that were failing because of the removal of "…

7a82c02

…See More"

Merge branch 'develop' into t-info-tablet-port-highfi

bc98bc8

Merge branch 'develop' into t-info-tablet-port-highfi

5565543

MohamedMedhat1998 requested review from rt4914, vinitamurthi, BenHenning and anandwana001 July 17, 2020 13:15

MohamedMedhat1998 assigned rt4914, vinitamurthi, BenHenning and anandwana001 Jul 17, 2020

MohamedMedhat1998 changed the base branch from develop to t-info-tablet-port-highfi July 17, 2020 13:16

-Added 1 robolectric test

5b431f2

BenHenning reviewed Jul 18, 2020

View reviewed changes

BenHenning removed their assignment Jul 18, 2020

vinitamurthi reviewed Jul 20, 2020

View reviewed changes

vinitamurthi removed their assignment Jul 20, 2020

anandwana001 reviewed Jul 20, 2020

View reviewed changes

anandwana001 removed their assignment Jul 20, 2020

MohamedMedhat1998 assigned vinitamurthi and BenHenning Jul 21, 2020

BenHenning assigned MohamedMedhat1998 Jul 22, 2020

rt4914 unassigned rt4914 and vinitamurthi Jul 22, 2020

rt4914 unassigned BenHenning Jul 22, 2020

-Some nit changes

df23c3b

MohamedMedhat1998 changed the base branch from t-info-tablet-port-highfi to develop July 25, 2020 11:36

MohamedMedhat1998 changed the base branch from develop to t-info-tablet-port-highfi July 25, 2020 11:36

MohamedMedhat1998 requested a review from BenHenning July 25, 2020 11:40

MohamedMedhat1998 assigned rt4914 and BenHenning Jul 25, 2020

-Merge remote-tracking branch 'origin/develop' into t-info-tablet-rob…

66656ac

…olectric-tests # Conflicts (Resolved): # app/src/main/java/org/oppia/app/activity/ActivityComponent.kt -Added some test case

rt4914 mentioned this pull request Jul 27, 2020

Fixes #1157: Topic-info - Tablet (Portrait) (Highfi) #1463

Merged

8 tasks

Base automatically changed from t-info-tablet-port-highfi to develop July 28, 2020 07:06

BenHenning removed their assignment Jul 29, 2020

rt4914 removed their assignment Sep 3, 2020

MohamedMedhat1998 closed this Sep 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Topic Info Tablet Robolectric Tests [DON'T MERGE] #1480

Topic Info Tablet Robolectric Tests [DON'T MERGE] #1480

MohamedMedhat1998 commented Jul 17, 2020

BenHenning Jul 18, 2020

MohamedMedhat1998 Jul 21, 2020

BenHenning left a comment

vinitamurthi left a comment

vinitamurthi Jul 20, 2020

MohamedMedhat1998 Jul 21, 2020

vinitamurthi commented Jul 20, 2020 •

edited

Loading

anandwana001 Jul 20, 2020

anandwana001 Jul 20, 2020

anandwana001 Jul 20, 2020

anandwana001 Jul 20, 2020

anandwana001 Jul 20, 2020

MohamedMedhat1998 commented Jul 21, 2020

BenHenning commented Jul 22, 2020 •

edited

Loading

MohamedMedhat1998 commented Jul 25, 2020

BenHenning commented Jul 28, 2020

MohamedMedhat1998 commented Jul 28, 2020

BenHenning commented Jul 29, 2020 •

edited

Loading

MohamedMedhat1998 commented Aug 8, 2020

rt4914 commented Sep 3, 2020

MohamedMedhat1998 commented Sep 3, 2020

Topic Info Tablet Robolectric Tests [DON'T MERGE] #1480

Topic Info Tablet Robolectric Tests [DON'T MERGE] #1480

Conversation

MohamedMedhat1998 commented Jul 17, 2020

Notes

Explanation

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

vinitamurthi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinitamurthi commented Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohamedMedhat1998 commented Jul 21, 2020

BenHenning commented Jul 22, 2020 • edited Loading

MohamedMedhat1998 commented Jul 25, 2020

BenHenning commented Jul 28, 2020

MohamedMedhat1998 commented Jul 28, 2020

BenHenning commented Jul 29, 2020 • edited Loading

MohamedMedhat1998 commented Aug 8, 2020

rt4914 commented Sep 3, 2020

MohamedMedhat1998 commented Sep 3, 2020

vinitamurthi commented Jul 20, 2020 •

edited

Loading

BenHenning commented Jul 22, 2020 •

edited

Loading

BenHenning commented Jul 29, 2020 •

edited

Loading