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

Jump to bottom

Fixes #3351 Merge accessibility test rule in oppia test rule #4025

Merged
merged 30 commits into from
Jan 25, 2022
Merged

Fixes #3351 Merge accessibility test rule in oppia test rule #4025

merged 30 commits into from
Jan 25, 2022

Conversation

vrajdesai78
Copy link
Contributor

@vrajdesai78 vrajdesai78 commented Dec 5, 2021 •
edited
Loading

Explanation

Fixes #3351 merged Accessibility TestRule in OppiaTestRule.

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

Hi @vrajdesai78 please add the Fixes #issue number so that PR and Issue can be linked.

@vrajdesai78 vrajdesai78 changed the title Fix #3351 Merge accessibility test rule in oppia test rule Fixes #3351 Merge accessibility test rule in oppia test rule Dec 5, 2021
@vrajdesai78
Copy link
Contributor Author

Hi @vrajdesai78 please add the Fixes #issue number so that PR and Issue can be linked.

Thanks for mentioning it

@yash10019coder
Copy link
Contributor

Hi @vrajdesai78 please add the Fixes #issue number so that PR and Issue can be linked.

Thanks for mentioning it

you have to add it in the PR description also as no linked issue appears.

rt4914
rt4914 suggested changes Dec 7, 2021
View reviewed changes
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.

Have a look at these failing tests: https://github.com/oppia/oppia-android/runs/4447735732?check_suite_focus=true

Basically the AccessibilityTestRule file was getting used in a lot of places so please remove it from all those files and instead use OppiaTestRule in that file. But in case OppiaTestRule is already available in the file then do not add another one.

@oppiabot
Copy link

oppiabot bot commented Dec 14, 2021

Hi @vrajdesai78, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Dec 14, 2021
@vrajdesai78
Copy link
Contributor Author

I am working on this

@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Dec 15, 2021
@BenHenning
Copy link
Sponsor Member

Hi. As of today, some main reviewers have taken time off for the next few weeks, so it may take a little while before we can look at this PR. We appreciate your patience while some of our team members recharge. We'll be fully returning on 4 January 2021.

@BenHenning BenHenning assigned vrajdesai78 and unassigned BenHenning Jan 19, 2022
@vrajdesai78
Copy link
Contributor Author

Test case with DisableAccessibilityChecks annotation
image

Test case after removing DisableAccessibilityChecks annotation
image

@vrajdesai78
Copy link
Contributor Author

@BenHenning PTAL.

@oppiabot oppiabot bot assigned BenHenning and unassigned vrajdesai78 Jan 19, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 19, 2022

Unassigning @vrajdesai78 since a re-review was requested. @vrajdesai78, please make sure you have addressed all review comments. Thanks!

anandwana001
anandwana001 approved these changes Jan 19, 2022
View reviewed changes
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

My codeowner files LGTM.

@oppiabot
Copy link

oppiabot bot commented Jan 19, 2022

Unassigning @anandwana001 since they have already approved the PR.

BenHenning
BenHenning reviewed Jan 21, 2022
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @vrajdesai78. Followed up on the last remaining comment--PTAL.

testing/src/main/java/org/oppia/android/testing/OppiaTestRule.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned vrajdesai78 and unassigned BenHenning Jan 21, 2022
@vrajdesai78
Copy link
Contributor Author

vrajdesai78 commented Jan 21, 2022 •
edited
Loading

@BenHenning As you said I have tried to place this condition inside the first case of when image but after removing DisableAccessibilitychecks test cases are passing instead of failing. So, I have said that after placing the condition in first case of when it's not working as after removing DisableAccessibilityChecks annotation test cases should fail but it's passing.

@BenHenning PTAL. Thanks

@oppiabot oppiabot bot assigned BenHenning and unassigned vrajdesai78 Jan 21, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 21, 2022

Unassigning @vrajdesai78 since a re-review was requested. @vrajdesai78, please make sure you have addressed all review comments. Thanks!

@BenHenning BenHenning assigned vrajdesai78 and unassigned BenHenning Jan 22, 2022
@vrajdesai78
Copy link
Contributor Author

@BenHenning PTAL. Thanks

@oppiabot oppiabot bot assigned BenHenning and unassigned vrajdesai78 Jan 22, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 22, 2022

Unassigning @vrajdesai78 since a re-review was requested. @vrajdesai78, please make sure you have addressed all review comments. Thanks!

BenHenning
BenHenning approved these changes Jan 25, 2022
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @vrajdesai78. This LGTM!

@BenHenning BenHenning merged commit 55d0669 into oppia:develop Jan 25, 2022
bhaktideshmukh pushed a commit to bhaktideshmukh/oppia-android that referenced this pull request Jan 25, 2022
@vrajdesai78 @bhaktideshmukh
Fixes oppia#3351 Merge accessibility test rule in oppia test rule (op…
6819445 
…pia#4025)

* Tests added in OppiaTestRule from AccessbilityTestRule

* Optimized code in OppiaTestRule

* Optimized imports in OppiaTestRule

* AccessbilityTestRule file deleted

* Revert "AccessbilityTestRule file deleted"

This reverts commit e6df4eb.

* AccessbilityTestRule replaced with OppiaTestRule in some files

* OppiaTestRule added in remaining files

* AccessbilityTestRule file deleted

* klint issue fixed for some files

* Revert "AccessbilityTestRule file deleted"

This reverts commit 8f9532f.

* klint fixed for single activity to check

* klint fixed for some files

* klint issue fixed

* Imports optimized

* Removed blank line from HomeActivityTest

* AccessibilityTestRule File Deleted

* Fixed tests passing on removing DisableAccessibilityTest

* Revert "AccessibilityTestRule File Deleted"

This reverts commit 1566b0d.

* Revert "Fixed tests passing on removing DisableAccessibilityTest"

This reverts commit fe3b0f8.

* AccessibilityTest File Deleted

* Fixed error in OppiaTestRule

* Fixed klint issue

* Changed variables in OppiaTestRule

* Fixed klint issue in OppiaTestRule

* Klint space issue fixed

* AccessibilityTestRule placed inside when condition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

@BenHenning BenHenning BenHenning approved these changes

@anandwana001 anandwana001 anandwana001 approved these changes

Assignees

@BenHenning BenHenning

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge OppiaTestRule and AccessibilityTestRule
5 participants
@vrajdesai78 @yash10019coder @BenHenning @rt4914 @anandwana001