Fix #2209: Create a Helper class to provide methos for [RecyclerViewActions] scrolling actions #2451

srikanthkb · 2021-01-11T09:54:47Z

Explanation

Created a new class which provides all of the potential [RecyclerViewActions] required by our codebase.
Replaced scrollToPosition() with actionOnItemAtPosition() [RecyclerViewActions].
This change allows both Espresso and Roboelectric tests for TopicLessonsFragmentTest.kt to be successful.

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.

Created a new file in testing module to provide for the different recyclerview actions. TODO: * Update the Testing class to use scrolling action from this api * Include the third method of scrolling actions * Work on the documentation - for the class, and each method in it * Check and confirm gradle dependencies

(TODO) Improve clarity on the names, Kdocs, usage of the methods described in the new file.

…s-testing-module

into scrolling-actions-testing-module

srikanthkb · 2021-01-11T09:56:31Z

@BenHenning , @anandwana001 , @rt4914
Please provide your inputs on the method names, documentation, test cases etc.

BenHenning

Thanks @Srikanth-Kb! Left my thoughts.

testing/src/main/java/org/oppia/android/testing/RecyclerViewScrollingActions.kt

BenHenning · 2021-01-12T04:17:27Z

testing/src/main/java/org/oppia/android/testing/RecyclerViewScrollingActions.kt

+/**
+ * Provides the different scrolling options for RecyclerView
+ *
+ * This is needed because few Roboelectric tests require different scrolling actions for the tests
+ * to be successful.
+ * See https://github.com/oppia/oppia-android/issues/2209 for more information.
+ */


Suggested change

/**

* Provides the different scrolling options for RecyclerView

*

* This is needed because few Roboelectric tests require different scrolling actions for the tests

* to be successful.

* See https://github.com/oppia/oppia-android/issues/2209 for more information.

*/

/**

* Provides different scrolling actions for [RecyclerView]s.

*

* This is needed because different testing scenarios in Robolectric tests

* potentially require different scrolling actions for the tests to be successful.

* See #2209 for more information.

*/

(May need line wrapping--you should ensure the KDocs lines take up as much space in the 100 character column limit).

Note that I rephrased this slightly to try and clarify the meaning. Note also that we can just reference issues within our repository directly by '#'--that's conventional on the team so others will understand what that means when they come across it.

Noted. Updated the changes.

BenHenning · 2021-01-12T04:19:10Z

testing/src/main/java/org/oppia/android/testing/RecyclerViewScrollingActions.kt

+   * 1) Scroll RecyclerView to position
+   * 2) Perform an action on the view at position
+   * Usage : TODO


Some thoughts:

We probably don't care about documenting usage at this level (that can be introspected by seeing where this method is being used, instead)

The documentation shouldn't discuss how the method works, just what it's promising to do

Between this & the method above, I don't understand when to pick one versus the other. How does one make that decision? These KDocs should clearly explain when it's appropriate to use each one so that it can become clearer to others when trying to decide which to use when scrolling in a recycler view.

Noted. KDoc for latest changes implemented with these changes.

srikanthkb · 2021-01-12T12:20:44Z

@rt4914 @anandwana001
As an alternative, I used the actionOnItemAtPosition view action instead of scrollToPosition view action, in the private method scrollToPosition in TopicLessonsFragmentTest.kt

oppia-android/app/src/sharedTest/java/org/oppia/android/app/topic/lessons/TopicLessonsFragmentTest.kt

Line 384 in 9895d51

scrollToPosition<RecyclerView.ViewHolder>(

With this one line change, I was able to run both the Espresso tests and Roboelectric tests successfully for TopicLessonsFragmentTest.kt, the screenshots are attached for reference.

Espresso tests

Roboelectric tests

Do we still need the helper class to provide different scrolling actions for the RecyclerView ?

rt4914 · 2021-01-12T16:05:40Z

@rt4914 @anandwana001
As an alternative, I used the actionOnItemAtPosition view action instead of scrollToPosition view action, in the private method scrollToPosition in TopicLessonsFragmentTest.kt

oppia-android/app/src/sharedTest/java/org/oppia/android/app/topic/lessons/TopicLessonsFragmentTest.kt

Line 384 in 9895d51

scrollToPosition<RecyclerView.ViewHolder>(

With this one line change, I was able to run both the Espresso tests and Roboelectric tests successfully for TopicLessonsFragmentTest.kt, the screenshots are attached for reference.

Espresso tests

Roboelectric tests

Do we still need the helper class to provide different scrolling actions for the RecyclerView ?

@Srikanth-Kb I think we should have only 1 function which works for all because at this point both of these functions looks almost same to me.

* Replace scrollToPosition() RecyclerView action with actionOnItemAtPosition() * Successfully supports both Espresso and Roboelectric test cases in TopicLessonsFragment.kt

anandwana001

left my thoughts on replacing thing.

anandwana001 · 2021-01-13T06:48:46Z

app/src/sharedTest/java/org/oppia/android/app/topic/lessons/TopicLessonsFragmentTest.kt

@@ -381,8 +378,9 @@ class TopicLessonsFragmentTest {

  private fun scrollToPosition(position: Int) {
    onView(withId(R.id.story_summary_recycler_view)).perform(
-      scrollToPosition<RecyclerView.ViewHolder>(
-        position
+      actionOnItemAtPosition<RecyclerView.ViewHolder>(


There are many other files in the whole codebase where we are using scrollToPosition<RecyclerView.ViewHolder>(position).

So, as for this file only, if we bring consistency by using actionOnItemAtPosition<RecyclerView.ViewHolder>() everywhere, it's also good, but we do need a helper class which is something single point where we can look for when to use which scrolling way.

anandwana001 · 2021-01-13T06:49:58Z

Should we use actionOnItemAtPosition everywhere in the codebase? WDYT?

* Created a new helper class including methods for all of required RecyclerView actions for the codebase * Added required gradle dependencies in the testing module

srikanthkb · 2021-01-15T16:44:38Z

Requesting @BenHenning , @anandwana001 for any thoughts on this implementation

BenHenning

Thanks @Srikanth-Kb. Some thoughts:

When following up to a review, make sure to respond to all comments before sending the PR back to the reviewer
The implementation of the helper class looks good (aside from my style nits), so I think that you can go ahead and broadly move the rest of the codebase over to this implementation
Note that the migration over to this implementation ought to happen in a later PR. I suggest splitting this up such that:
a. We introduce a new utility with its own dedicated tests to verify correctness
b. We migrate all positional uses over to the new utility
c. We migrate all view type uses to the new utility

This results in 3 PRs to keep things a bit smaller and self-contained. It also means that if something breaks and we need to revert the PR, we won't need to revert the entire batch of changes.

BenHenning · 2021-01-16T01:08:06Z

testing/src/main/java/org/oppia/android/testing/RecyclerViewScrollingActions.kt

+   * Use this method for all testing scenarios when scrolling to a RecyclerView at a particular
+   * position
+   */
+  fun scrollToPosition(recyclerViewId: Int, position: Int) {


Here & below: recyclerViewId should be annotated with LayoutRes (https://developer.android.com/reference/androidx/annotation/LayoutRes) to explicitly specify that this represents a layout resource ID.

BenHenning · 2021-01-16T01:09:23Z

testing/src/main/java/org/oppia/android/testing/RecyclerViewScrollingActions.kt

+  }
+
+  /**
+   * Performs scrolling action to [RecyclerView] of a particular ViewType


Suggested change

* Performs scrolling action to [RecyclerView] of a particular ViewType

* Performs scrolling action to [RecyclerView] of a particular ViewType designed by a view holder via [baseMatcher].

Some additional context (may need wrapping). Try to keep this & other KDocs as proper paragraphs, too (e.g. don't line break unless it's for wrapping or you want to communicate separate ideas (in which case there should be a blank documentation line, e.g. just '*' between the paragraphs).

Ditto for both in above KDocs.

Noted. Updated the change in commit.

BenHenning · 2021-01-16T01:10:28Z

testing/build.gradle

      'androidx.test.espresso:espresso-core:3.2.0',
+      'androidx.test.espresso:espresso-contrib:3.2.0',
+      'androidx.test.espresso:espresso-intents:3.1.0',


Why is the intents library needed?

intents library is not required in the testing module.
Updated the change in new commit.

BenHenning · 2021-01-16T01:11:14Z

app/src/sharedTest/java/org/oppia/android/app/topic/lessons/TopicLessonsFragmentTest.kt

-        actionOnItemAtPosition<RecyclerView.ViewHolder>(2, scrollTo())
+      recyclerViewScrollingActions.scrollToPosition(
+        R.id.story_summary_recycler_view,
+        2


Suggested change

2

position = 2

When using literals, prefer to add named arguments to provide context to readers on the value. Ditto elsewhere in this PR.

Noted. Done.
Also created #2515 as a good-first-issue for such changes elsewhere in the codebase.

This change will be handled in an independent PR, as planned, migration for positional use case will be handled independently for the entire codebase.

BenHenning · 2021-01-16T01:12:10Z

app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt

@@ -253,7 +257,10 @@ class StateFragmentTest {
    launchForExploration(TEST_EXPLORATION_ID_2).use {
      startPlayingExploration()

-      scrollToViewType(CONTINUE_INTERACTION)


I think let's invert this refactor--scrollToViewType can use RecyclerViewScrollingActions' scrollToViewType (the current syntax is much cleaner so we actually lose that by refactoring at the test level).

This change will be handled in a separate PR, as migration will be handled independently.

The migration of the actual tests will be handled independently.

* Updated KDocs with line wrapping, better context. * Updated annotation @LayoutRes for recyclerViewId

srikanthkb · 2021-02-18T13:28:01Z

Note that the migration over to this implementation ought to happen in a later PR. I suggest splitting this up such that:
a. We introduce a new utility with its own dedicated tests to verify correctness
b. We migrate all positional uses over to the new utility
c. We migrate all view type uses to the new utility

This results in 3 PRs to keep things a bit smaller and self-contained. It also means that if something breaks and we need to revert the PR, we won't need to revert the entire batch of changes.

Hi @BenHenning ,
Regarding the first PR, when we are writing dedicated tests to verify the correctness of the utility methods in RecyclerViewScrollingActions.kt
a. Do we have to create a RecyclerViewScrollingActionsTestActivity, RecyclerViewTestAdapter, RecyclerViewTestViewHolder, required test layouts (xml) for activity, viewholder etc and fake data (used only for testing) ?
Is this line of thinking right ?
b. Can you provide some inputs on how we should test the scrollToViewType() method, given that its argument is of type BaseMatcher<RecyclerView.ViewHolder> ?

@anandwana001 Please provide your suggestions as well.

srikanthkb · 2021-03-01T07:15:57Z

Here's some information regarding writing tests for the RecyclerView scrolling actions:

We need to create a dedicated TestActivity, TestFragment, TestFragment presenter, layouts, RecyclerViewAdapter, Viewholders (of two types) and fake data. Use BinderAdapter tests for reference.
For the first method: scrollToPosition() , we can use a RecyclerView with fake data (preferably text) and verify if the text is displayed correctly at that position.
For the second method: scrollToViewType, we have to create a CustomMatcher which extends the BaseMatcher class, and use at least two viewholder types to verify the data displayed on different view types.

srikanthkb added 4 commits January 8, 2021 15:59

Update Kdocs for the public methods in the file

6970f58

(TODO) Improve clarity on the names, Kdocs, usage of the methods described in the new file.

Merge remote-tracking branch 'upstream/develop' into scrolling-action…

cc047e1

…s-testing-module

Merge 'upstream/develop'

a55bff3

into scrolling-actions-testing-module

Update KDocs for the public helper methods

49ba036

rt4914 assigned rt4914, BenHenning and anandwana001 Jan 11, 2021

BenHenning requested changes Jan 12, 2021

View reviewed changes

BenHenning assigned srikanthkb and unassigned BenHenning Jan 12, 2021

Update public method names

2ff6ad3

rt4914 removed their assignment Jan 12, 2021

srikanthkb added 3 commits January 13, 2021 11:05

Use actionOnItemAtPosition()

0a53294

* Replace scrollToPosition() RecyclerView action with actionOnItemAtPosition() * Successfully supports both Espresso and Roboelectric test cases in TopicLessonsFragment.kt

Remove unused gradle dependencies

2061f10

Merge remote-tracking branch 'upstream/develop'

9644374

srikanthkb changed the title ~~Fix #2209: Introduce scrolling actions testing module~~ Fix #2209: Replace scrollToPosition() with actionOnItemAtPosition() [RecyclerViewActions] Jan 13, 2021

anandwana001 suggested changes Jan 13, 2021

View reviewed changes

anandwana001 removed their assignment Jan 13, 2021

srikanthkb added 3 commits January 15, 2021 20:57

Merge remote-tracking branch

b1b0817

Created helper class for scrolling actions

775da5d

* Created a new helper class including methods for all of required RecyclerView actions for the codebase * Added required gradle dependencies in the testing module

Updated local changes for Ktlint

77477f1

srikanthkb changed the title ~~Fix #2209: Replace scrollToPosition() with actionOnItemAtPosition() [RecyclerViewActions]~~ Fix #2209: Create a Helper class to provide for all [RecyclerViewActions] Jan 15, 2021

srikanthkb assigned BenHenning and anandwana001 Jan 15, 2021

srikanthkb changed the title ~~Fix #2209: Create a Helper class to provide for all [RecyclerViewActions]~~ Fix #2209: Create a Helper class to provide methos for [RecyclerViewActions] scrolling actions Jan 15, 2021

BenHenning requested changes Jan 16, 2021

View reviewed changes

BenHenning removed their assignment Jan 16, 2021

anandwana001 removed their assignment Jan 19, 2021

srikanthkb added 5 commits January 19, 2021 16:30

Update StateFragmentTest.kt - nit change

640b947

Removed intents library dependency

d38595b

Revert changes in Test files

15ff6da

The migration of the actual tests will be handled independently.

Update RecyclerViewScrollingActions

68e102e

* Updated KDocs with line wrapping, better context. * Updated annotation @LayoutRes for recyclerViewId

nit change

d8c6a1c

BenHenning added the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label Feb 24, 2021

Merge branch 'develop' into scrolling-actions-testing-module

8aa16a7

BenHenning removed the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label Feb 24, 2021

srikanthkb assigned srikanthkb and unassigned srikanthkb Mar 1, 2021

anandwana001 closed this Mar 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2209: Create a Helper class to provide methos for [RecyclerViewActions] scrolling actions #2451

Fix #2209: Create a Helper class to provide methos for [RecyclerViewActions] scrolling actions #2451

srikanthkb commented Jan 11, 2021 •

edited

Loading

srikanthkb commented Jan 11, 2021

BenHenning left a comment

BenHenning Jan 12, 2021

srikanthkb Jan 18, 2021

BenHenning Jan 12, 2021

srikanthkb Jan 18, 2021

srikanthkb commented Jan 12, 2021

rt4914 commented Jan 12, 2021

anandwana001 left a comment

anandwana001 Jan 13, 2021

srikanthkb Jan 15, 2021

anandwana001 commented Jan 13, 2021

srikanthkb commented Jan 15, 2021 •

edited

Loading

BenHenning left a comment

BenHenning Jan 16, 2021

srikanthkb Jan 19, 2021

BenHenning Jan 16, 2021

srikanthkb Jan 20, 2021

BenHenning Jan 16, 2021

srikanthkb Jan 19, 2021

BenHenning Jan 16, 2021

srikanthkb Jan 19, 2021

srikanthkb Jan 19, 2021

BenHenning Jan 16, 2021

srikanthkb Jan 19, 2021

srikanthkb Jan 19, 2021

srikanthkb commented Feb 18, 2021

srikanthkb commented Mar 1, 2021

-/**
- * Provides the different scrolling options for RecyclerView
- *
- * This is needed because few Roboelectric tests require different scrolling actions for the tests
- * to be successful.
- * See https://github.com/oppia/oppia-android/issues/2209 for more information.
- */
+/**
+ * Provides different scrolling actions for [RecyclerView]s.
+ *
+ * This is needed because different testing scenarios in Robolectric tests
+ * potentially require different scrolling actions for the tests to be successful.
+ * See #2209 for more information.
+ */

	* Performs scrolling action to [RecyclerView] of a particular ViewType
	* Performs scrolling action to [RecyclerView] of a particular ViewType designed by a view holder via [baseMatcher].

Fix #2209: Create a Helper class to provide methos for [RecyclerViewActions] scrolling actions #2451

Fix #2209: Create a Helper class to provide methos for [RecyclerViewActions] scrolling actions #2451

Conversation

srikanthkb commented Jan 11, 2021 • edited Loading

Explanation

Checklist

srikanthkb commented Jan 11, 2021

BenHenning left a 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

Choose a reason for hiding this comment

srikanthkb commented Jan 12, 2021

rt4914 commented Jan 12, 2021

anandwana001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandwana001 commented Jan 13, 2021

srikanthkb commented Jan 15, 2021 • edited Loading

BenHenning left a 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

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

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

srikanthkb commented Feb 18, 2021

srikanthkb commented Mar 1, 2021

srikanthkb commented Jan 11, 2021 •

edited

Loading

srikanthkb commented Jan 15, 2021 •

edited

Loading