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.

Sign up for GitHub

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 #2209: Create a Helper class to provide methos for [RecyclerViewActions] scrolling actions #2451

Closed
wants to merge 18 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a9c115f
Initial commit for RecyclerView scrolling actions
srikanthkb Jan 8, 2021
6970f58
Update Kdocs for the public methods in the file
srikanthkb Jan 11, 2021
cc047e1
Merge remote-tracking branch 'upstream/develop' into scrolling-action…
srikanthkb Jan 11, 2021
a55bff3
Merge 'upstream/develop'
srikanthkb Jan 11, 2021
49ba036
Update KDocs for the public helper methods
srikanthkb Jan 11, 2021
2ff6ad3
Update public method names
srikanthkb Jan 12, 2021
0a53294
Use actionOnItemAtPosition()
srikanthkb Jan 13, 2021
2061f10
Remove unused gradle dependencies
srikanthkb Jan 13, 2021
9644374
Merge remote-tracking branch 'upstream/develop'
srikanthkb Jan 13, 2021
b1b0817
Merge remote-tracking branch
srikanthkb Jan 15, 2021
775da5d
Created helper class for scrolling actions
srikanthkb Jan 15, 2021
77477f1
Updated local changes for Ktlint
srikanthkb Jan 15, 2021
640b947
Update StateFragmentTest.kt - nit change
srikanthkb Jan 19, 2021
d38595b
Removed intents library dependency
srikanthkb Jan 19, 2021
15ff6da
Revert changes in Test files
srikanthkb Jan 19, 2021
68e102e
Update RecyclerViewScrollingActions
srikanthkb Jan 19, 2021
d8c6a1c
nit change
srikanthkb Feb 18, 2021
8aa16a7
Merge branch 'develop' into scrolling-actions-testing-module
BenHenning Feb 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions app/src/sharedTest/java/org/oppia/android/app/topic/lessons/TopicLessonsFragmentTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@ import androidx.test.core.app.ActivityScenario.launch
import androidx.test.core.app.ApplicationProvider
import androidx.test.espresso.Espresso.onView
import androidx.test.espresso.action.ViewActions.click
import androidx.test.espresso.action.ViewActions.scrollTo
import androidx.test.espresso.assertion.ViewAssertions.doesNotExist
import androidx.test.espresso.assertion.ViewAssertions.matches
import androidx.test.espresso.contrib.RecyclerViewActions.actionOnItemAtPosition
import androidx.test.espresso.contrib.RecyclerViewActions.scrollToPosition
import androidx.test.espresso.intent.Intents
import androidx.test.espresso.intent.Intents.intended
Expand Down Expand Up @@ -72,6 +70,7 @@ import org.oppia.android.domain.topic.RATIOS_EXPLORATION_ID_0
import org.oppia.android.domain.topic.RATIOS_STORY_ID_0
import org.oppia.android.domain.topic.RATIOS_TOPIC_ID
import org.oppia.android.domain.topic.StoryProgressTestHelper
import org.oppia.android.testing.RecyclerViewScrollingActions
import org.oppia.android.testing.RobolectricModule
import org.oppia.android.testing.TestAccessibilityModule
import org.oppia.android.testing.TestCoroutineDispatchers
Expand Down Expand Up @@ -104,6 +103,9 @@ class TopicLessonsFragmentTest {
@Inject
lateinit var storyProgressTestHelper: StoryProgressTestHelper

@Inject
lateinit var recyclerViewScrollingActions: RecyclerViewScrollingActions

private val internalProfileId = 0

private lateinit var profileId: ProfileId
Expand Down Expand Up @@ -281,8 +283,9 @@ class TopicLessonsFragmentTest {
clickLessonTab()
scrollToPosition(position = 1)
clickStoryItem(position = 1, targetViewId = R.id.chapter_list_drop_down_icon)
onView(withId(R.id.story_summary_recycler_view)).perform(
actionOnItemAtPosition<RecyclerView.ViewHolder>(2, scrollTo())
recyclerViewScrollingActions.scrollToPositionWithCompleteDisplayForAction(
R.id.story_summary_recycler_view,
2
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

)
clickStoryItem(position = 2, targetViewId = R.id.chapter_list_drop_down_icon)
scrollToPosition(position = 1)
Expand Down
3 changes: 3 additions & 0 deletions testing/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ dependencies {
implementation(
'androidx.annotation:annotation:1.1.0',
'androidx.lifecycle:lifecycle-livedata-ktx:2.2.0-alpha03',
'androidx.recyclerview:recyclerview:1.0.0',
'androidx.test.espresso:espresso-core:3.2.0',
'androidx.test.espresso:espresso-contrib:3.2.0',
'androidx.test.espresso:espresso-intents:3.1.0',
Copy link
Member

Choose a reason for hiding this comment

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

Why is the intents library needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

'androidx.test:runner:1.2.0',
'com.google.dagger:dagger:2.24',
'org.jetbrains.kotlinx:kotlinx-coroutines-test:1.2.2',
Expand Down
50 changes: 50 additions & 0 deletions testing/src/main/java/org/oppia/android/testing/RecyclerViewScrollingActions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package org.oppia.android.testing

import androidx.recyclerview.widget.RecyclerView
import androidx.test.espresso.Espresso.onView
import androidx.test.espresso.action.ViewActions.scrollTo
import androidx.test.espresso.contrib.RecyclerViewActions.actionOnItemAtPosition
import androidx.test.espresso.contrib.RecyclerViewActions.scrollToPosition
import androidx.test.espresso.matcher.ViewMatchers.withId
import javax.inject.Inject

/**
* 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.
*/
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. Updated the changes.

class RecyclerViewScrollingActions @Inject constructor(
val testCoroutineDispatchers: TestCoroutineDispatchers
) {
BenHenning marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns a [ViewAction] which scrolls RecyclerView to a position
* Usage : TODO
*/
fun scrollToPosition(viewId: Int, position: Int) {
onView(withId(viewId)).perform(
scrollToPosition<RecyclerView.ViewHolder>(
position
)
)
testCoroutineDispatchers.runCurrent()
}

/**
* Performs a [ViewAction] on a view at position.
* 1) Scroll RecyclerView to position
* 2) Perform an action on the view at position
* Usage : TODO
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. KDoc for latest changes implemented with these changes.

*/
fun scrollToPositionWithCompleteDisplayForAction(viewId: Int, position: Int) {
onView(withId(viewId)).perform(
actionOnItemAtPosition<RecyclerView.ViewHolder>(
position,
scrollTo()
)
)
testCoroutineDispatchers.runCurrent()
}
}