Fix #138: Topic train fragment Low-fi UI (Part 4) #204

rt4914 · 2019-10-03T10:25:35Z

Explanation

Replicated from #197

Part 1: #200
Part 2: #202
Part 3: #203
Part 4: Contains test case for TopicTrainFragment

Mock: https://xd.adobe.com/spec/e2239cf4-9cde-4c08-5296-25316c1f0a14-9412/screen/f403add1-079a-4202-9975-efe04c076290/Home-Page-1-Start-Learning-Pre-Scroll-6

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 assigned to an appropriate reviewer.

rt4914 · 2019-10-03T10:36:12Z

@veena14cs @nikitamarysolomanpvt PTAL

veena14cs

LGTM

rt4914 · 2019-10-03T12:49:21Z

LGTM

Thanks @veena14cs

app/src/sharedTest/java/org/oppia/app/topic/train/TopicTrainFragmentTest.kt

nikitamarysolomanpvt

Please include KDoc for test cases

app/src/sharedTest/java/org/oppia/app/topic/train/TopicTrainFragmentTest.kt

BenHenning · 2019-10-04T01:16:47Z

app/src/sharedTest/java/org/oppia/app/topic/train/TopicTrainFragmentTest.kt

-  fun testTopicTrainFragment_loadFragment_textIsDisplayed() {
-    ActivityScenario.launch(TopicActivity::class.java).use {
-      onView(withId(R.id.dummy_text_view)).check(matches(withText("This is dummy TextView for testing")))
+  fun testTopicTrainFragment_loadFragment_selectSkills_submitButtonIsActivated() {


Other cases worth testing:

Verifying that when no skills are selected, the button is not clickable

Verifying that the button's clickable state becomes disabled after selecting & deselecting skills

Verifying that clicking the submit button properly forwards the list of skills to an outgoing activity

Verifying that the list of selected skills stay selected upon a configuration change

Verifying that the submit button activation status stays correct for both enabled/disabled cases upon a configuration change

Added all these working test-cases

rt4914 · 2019-10-10T18:42:20Z

@BenHenning PTAL at updated test cases.

rt4914 · 2019-10-11T06:25:43Z

@BenHenning Regarding RecyclerViewMatcher -> I have removed that file and implemented those methods inside BindableTestAdapter.

I was getting stuck at two points.

I wanted to perform click on Checkbox inside the item which I was not unable do with the existing BindableTestAdapter.
I wanted the information of Checkbox inside the item too.

So, I have added two methods inside BindableTestAdapter which solves the issue that i am facing.

…fi-part-4

BenHenning

Thanks @rt4914. I ran out of time to deep dive this PR, but could you move the test utilities you're using in BindableAdapterTest to a shared location for all tests to use? Prefer not having one test depend on another.

app/src/main/java/org/oppia/app/topic/questionplayer/QuestionPlayerActivity.kt

BenHenning · 2019-10-15T05:19:15Z

Please reassign to me once the requested refactor is completed.

rt4914 · 2019-10-15T07:34:04Z

Thanks @rt4914. I ran out of time to deep dive this PR, but could you move the test utilities you're using in BindableAdapterTest to a shared location for all tests to use? Prefer not having one test depend on another.

@BenHenning I am actually confused what you mean here?

My interpretation: I should move the atPosition and atPositionView functions that I have introduced in BindableTestAdapter to a separate file.

If this is the case, I had actually done that only earlier by introducing the RecyclerViewMatcher file.
Reference Code:

oppia-android/app/src/sharedTest/java/org/oppia/app/recyclerview/RecyclerViewMatcher.kt

Line 11 in f863f6e

class RecyclerViewMatcher {

…fi-part-4

rt4914 · 2019-10-18T05:40:36Z

@BenHenning
As discussed in one of our meetings, for recyclerview item testing we can either introduce androidx.test.espresso:espresso-contrib:3.1.0 library or we can write our own custom recyclerview test code.

I tried using androidx.test.espresso:espresso-contrib:3.1.0 but it only provides code to click on an specific item in recyclerview. There is no mechanism to check the content of an item.

Because of these reasons, I have created one new file which we can use for recyclerview testing, i.e., RecyclerViewMatcher, also, I have changed your BindableAdapterTest cases to use this RecyclerViewMatcher file.

rt4914 · 2019-10-18T05:42:36Z

@BenHenning This PR is ready for your review.

BenHenning

Thanks @rt4914! If the tests are passing, this looks reasonable to me. I found one potential issue with some of the new tests around configuration changes, but other than that the PR LGTM. Feel free to submit once the comments are resolved.

app/src/sharedTest/java/org/oppia/app/topic/train/TopicTrainFragmentTest.kt

BenHenning · 2019-10-18T07:34:41Z

app/src/sharedTest/java/org/oppia/app/topic/train/TopicTrainFragmentTest.kt

+  fun testTopicTrainFragment_loadFragment_selectSkills_configurationChange_skillsAreSelected() {
+    activityTestRule.launchActivity(null)
+    onView(atPosition(R.id.skill_recycler_view,0)).perform(click())
+    activityTestRule.activity.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE


I don't believe this is sufficient for a configuration change. Suggest instead: ActivityScenario.recreate()

Ditto elsewhere for configuration changes.

This means I have add this or do I have to remove the current line and replace this one?

Using both and merge code for now, because the test-cases passes in all possible combinations, using current code only, using your code only and using both.

rt4914 · 2019-10-18T08:56:45Z

Merging this code after making all suggested changes.

Rajat Talesra added 4 commits October 3, 2019 15:20

All xml files for topic-train except layout folder

8cdfb33

Skill item related files

afd8196

TopicTrainFragment implementation

87977ab

Topic Train Test Case

456d129

rt4914 requested review from BenHenning, nikitamarysolomanpvt and veena14cs October 3, 2019 10:36

rt4914 assigned veena14cs and nikitamarysolomanpvt Oct 3, 2019

veena14cs approved these changes Oct 3, 2019

View reviewed changes

veena14cs assigned rt4914 and unassigned veena14cs Oct 3, 2019

rt4914 assigned BenHenning and unassigned rt4914 Oct 3, 2019

nikitamarysolomanpvt reviewed Oct 3, 2019

View reviewed changes

app/src/sharedTest/java/org/oppia/app/topic/train/TopicTrainFragmentTest.kt Show resolved Hide resolved

nikitamarysolomanpvt reviewed Oct 3, 2019

View reviewed changes

rt4914 mentioned this pull request Oct 3, 2019

Project introduction: initial app module #1

Merged

BenHenning requested changes Oct 4, 2019

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Oct 4, 2019

Introduce QuestionPlayer Activity/Fragment

d21f52b

rt4914 changed the title ~~Fix (#138): Topic train fragment Low-fi UI (Part 4)~~ Fix #138: Topic train fragment Low-fi UI (Part 4) Oct 4, 2019

Rajat Talesra added 2 commits October 5, 2019 15:11

Domain layer code integration

e39fa93

Resolve merge conflict develop

9ff0ff5

Rajat Talesra added 2 commits October 11, 2019 00:09

Updated RecyclerViewMatcher

b7fb1e4

Nit change

9e81bb5

rt4914 assigned BenHenning and unassigned rt4914 Oct 10, 2019

Test added

5274077

Merge remote-tracking branch 'upstream/develop' into topic-train-low-…

ba680a3

…fi-part-4

rt4914 mentioned this pull request Oct 12, 2019

Fix #153/#154 : MultipleChoice/ItemSelection Interaction View #225

Closed

2 tasks

BenHenning requested changes Oct 15, 2019

View reviewed changes

app/src/main/java/org/oppia/app/topic/questionplayer/QuestionPlayerActivity.kt Outdated Show resolved Hide resolved

BenHenning assigned rt4914 and unassigned BenHenning Oct 15, 2019

Rajat Talesra added 3 commits October 15, 2019 13:06

Merge remote-tracking branch 'upstream/develop' into topic-train-low-…

3d03e47

…fi-part-4

Nit changes

1b446ff

Nit changes

2e9a334

rt4914 assigned BenHenning and unassigned rt4914 Oct 15, 2019

Rajat Talesra added 2 commits October 18, 2019 09:45

Merge remote-tracking branch 'upstream/develop' into topic-train-low-…

79b11ff

…fi-part-4

Test cases

5b9d0dd

BenHenning approved these changes Oct 18, 2019

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Oct 18, 2019

Test case updated

8a5f22f

rt4914 merged commit 117ec95 into develop Oct 18, 2019

rt4914 deleted the topic-train-low-fi-part-4 branch October 18, 2019 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #138: Topic train fragment Low-fi UI (Part 4) #204

Fix #138: Topic train fragment Low-fi UI (Part 4) #204

rt4914 commented Oct 3, 2019

rt4914 commented Oct 3, 2019

veena14cs left a comment

rt4914 commented Oct 3, 2019

nikitamarysolomanpvt left a comment

BenHenning Oct 4, 2019

rt4914 Oct 7, 2019

rt4914 commented Oct 10, 2019

rt4914 commented Oct 11, 2019

BenHenning left a comment

BenHenning commented Oct 15, 2019

rt4914 commented Oct 15, 2019

rt4914 commented Oct 18, 2019

rt4914 commented Oct 18, 2019

BenHenning left a comment

BenHenning Oct 18, 2019

rt4914 Oct 18, 2019

rt4914 Oct 18, 2019

rt4914 commented Oct 18, 2019

Fix #138: Topic train fragment Low-fi UI (Part 4) #204

Fix #138: Topic train fragment Low-fi UI (Part 4) #204

Conversation

rt4914 commented Oct 3, 2019

Explanation

Checklist

rt4914 commented Oct 3, 2019

veena14cs left a comment

Choose a reason for hiding this comment

rt4914 commented Oct 3, 2019

nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

BenHenning Oct 4, 2019

Choose a reason for hiding this comment

rt4914 Oct 7, 2019

Choose a reason for hiding this comment

rt4914 commented Oct 10, 2019

rt4914 commented Oct 11, 2019

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Oct 15, 2019

rt4914 commented Oct 15, 2019

rt4914 commented Oct 18, 2019

rt4914 commented Oct 18, 2019

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Oct 18, 2019

Choose a reason for hiding this comment

rt4914 Oct 18, 2019

Choose a reason for hiding this comment

rt4914 Oct 18, 2019

Choose a reason for hiding this comment

rt4914 commented Oct 18, 2019