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.

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 #935: Optimised test cases for HDPI devices #942

Closed
wants to merge 9 commits into from

Conversation

NullByte08
Copy link
Contributor

@NullByte08 NullByte08 commented Apr 3, 2020

Explanation

Fix #935

All the test cases except for player package now run and pass successfully on HDPI devices too as required by the issue. I used the Nexus S emulator.

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.

@NullByte08 NullByte08 requested a review from rt4914 April 3, 2020 22:18
@NullByte08
Copy link
Contributor Author

@rt4914 I have just added the comments so as to bring them to your attention. Otherwise, all the test are completely compatible now with HDPI devices.

@NullByte08 NullByte08 closed this Apr 7, 2020
@NullByte08 NullByte08 reopened this Apr 7, 2020
@rt4914
Copy link
Contributor

rt4914 commented Apr 7, 2020

@NullByte08 I ran all test cases at once. And here are some details regarding to that.
Screenshot 2020-04-07 at 11 11 46 AM

AdminstratorControlsActivityTest -> Tests are failing because the UI is not visible on screen. Mainly you will need to add scrollTo functionality in the failing test cases.
player package test cases you can avoid.
TopicRevisionFragmentTest -> These test cases will need to be updated. For reference you can see HomeActivityTest in which we are checking span count in similar way. Try to use this information to fix the test cases if it does not workout, do inform me and I can give you more hints.
ProfileEditActivityTest -> For this the functionality in code is incorrect and the test case has been written in the correct way.
Follow these steps:

  1. ProfileChooser Screen -> Administrator Controls -> Enter Pin
  2. Click on Edit Profiles
  3. Click on Sean's profile
  4. Now in toolbar you will notice that the name is Sean now do orientation change in mobile and you will see that toolbar shows Oppia.
    You should fix this functionality to fix the test case itself.

This issue is sort of a mini project as it was known that it will have multiple changes.

Thanks a lot for working on this as it is an important issue.

@NullByte08
Copy link
Contributor Author

NullByte08 commented Apr 8, 2020

@rt4914 Please review. I have run all the test files individually and all at once also. All tests are successful.

@rt4914
Copy link
Contributor

rt4914 commented Apr 9, 2020

@NullByte08 Please don'e forget to assign it to correct reviewer, because the assign section was containing your name, I though that you are still working on this.
Read this: https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section

@NullByte08 NullByte08 assigned rt4914 and unassigned NullByte08 Apr 9, 2020
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

There are still some test cases which are failing and we never use SystemClock.sleep(1000).

Actually the best approach to finish this PR soon is to split this PR into 3-4 different PRs and making sure that 4-5 packages are fixed in each PR.

@@ -86,4 +86,8 @@ class ProfileEditActivityPresenter @Inject constructor(
private fun getProfileEditViewModel(): ProfileEditViewModel {
return viewModelProvider.getForActivity(activity, ProfileEditViewModel::class.java)
}

fun handleOnRestoreSavedInstanceState(){
activity.title=editViewModel.profileName
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: activity.title = editViewModel.profileName

@@ -161,6 +161,8 @@ class HomeActivityTest {
getApplicationDependencies()
oppiaClock.setCurrentTimeMs(MORNING_TIMESTAMP)
launch<HomeActivity>(createHomeActivityIntent(internalProfileId)).use {
onView(withId(R.id.home_recycler_view)).perform(scrollToPosition<RecyclerView.ViewHolder>(0))

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this blank line

@@ -117,7 +120,8 @@ class PinPasswordActivityTest {
userId
)
).use {
onView(withId(R.id.input_pin)).perform(typeText("123"))
onView(withId(R.id.input_pin)).perform(typeText("123"), closeSoftKeyboard())
SystemClock.sleep(1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

We never use this because this sleep can actually hide the actual issue that might be happening in PR.

@@ -283,6 +289,7 @@ class PinPasswordActivityTest {
onView(withText(context.getString(R.string.admin_settings_submit))).perform(click())
onView(withText(context.getString(R.string.pin_password_close))).perform(click())
onView(withId(R.id.input_pin)).perform(typeText("321"))
SystemClock.sleep(1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto. Remove this.

@rt4914 rt4914 assigned NullByte08 and unassigned rt4914 Apr 9, 2020
@rt4914
Copy link
Contributor

rt4914 commented Apr 15, 2020

This is finished via #960 #963 #965

@rt4914 rt4914 closed this Apr 15, 2020
@NullByte08 NullByte08 deleted the hdpi-compatible-testing branch April 15, 2020 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimise app module test cases
2 participants