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 part of #3245 [A11y] Enabling AccessibilityChecks for PinPasswordActivityTest #4208

Merged
merged 1 commit into from
Feb 21, 2022

Conversation

Rohit1173
Copy link
Contributor

@Rohit1173 Rohit1173 commented Feb 20, 2022

Explanation

Fix part of #3245 : Enable Accessibility Checks
Removed the @DisableAccessibilityChecks Annotation from the PinPasswordActivityTest

There were 27 tests with @DisableAccessibilityChecks Annotation in PinPasswordActivityTest
So running these tests after removing it caused an Accessibility error and it said to remove ContentDescription of TextInputeditText inpin_password_activity.xml

I have checked this using Accessibility scanner app too and it also showed the same result

There were 2 tests fun testPinPassword_pinView_hasContentDescription() and
fun testPinPassword_configChange_pinView_hasContentDescription() which check the ContentDescription of the TextinputEditText . So I have to remove these tests
Screenshots after running the tests

Device: Pixel 3a API 28
after tests mob

Device: Pixel C API 28
after test tab

The only test which was failing was
fun testPinPassword_withUser_forgot_inputAdminPinAndNullPin_imeAction_errorIsDisplayed() and it was failing even before removing ContentDescription and @DisableAccessibilityChecks Annotation and not because of the changes made and the same test passes sometimes while running separately
image

UI CHANGES

BEFORE AFTER
pin before mob pin after mob
pin before mob land pin after mob land
pin before tab pin after tab
pin before tab land pin after tab land

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

@yash10019coder
Copy link
Contributor

@Rohit1173 it seems like a flaky test so I have restarted the ci.

@Rohit1173
Copy link
Contributor Author

@Rohit1173 it seems like a flaky test so I have restarted the ci.

Thanks, @yash10019coder

@Rohit1173
Copy link
Contributor Author

@rt4914 @anandwana001 PTAL

@ayush0402
Copy link
Contributor

I had also filed an issue about that failing test at #3771. If it is something that needs to addressed we can re-open that issue.

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.

LGTM, Thanks @Rohit1173

@rt4914 rt4914 merged commit e35ae05 into oppia:develop Feb 21, 2022
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.

None yet

5 participants