-
Notifications
You must be signed in to change notification settings - Fork 531
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
Intoduce screenshot-testing framework #1815
Labels
enhancement
End user-perceivable enhancements.
Impact: Medium
Moderate perceived user impact (non-blocking bugs and general improvements).
Issue: Needs Break-down
Indicates that an issue is too large and should be broken into smaller chunks.
Issue: Needs Clarification
Indicates that an issue needs more detail in order to be able to be acted upon.
Work: High
It's not clear what the solution is.
Z-ibt
Temporary label for Ben to keep track of issues he's triaged.
Comments
This was referenced Feb 24, 2021
Closed
6 tasks
Broppia
added
issue_type_infrastructure
Impact: Low
Low perceived user impact (e.g. edge cases).
labels
Jul 29, 2022
BenHenning
added
Impact: Medium
Moderate perceived user impact (non-blocking bugs and general improvements).
Issue: Needs Clarification
Indicates that an issue needs more detail in order to be able to be acted upon.
Issue: Needs Break-down
Indicates that an issue is too large and should be broken into smaller chunks.
Z-ibt
Temporary label for Ben to keep track of issues he's triaged.
issue_user_developer
and removed
Impact: Low
Low perceived user impact (e.g. edge cases).
labels
Sep 15, 2022
BenHenning
moved this to Needs Triage
in [Team] Core Learner and Mastery flows & UI Frontend - Android
Sep 15, 2022
seanlip
moved this from Needs Triage
to Todo
in [Team] Core Learner and Mastery flows & UI Frontend - Android
Dec 19, 2022
seanlip
added
enhancement
End user-perceivable enhancements.
and removed
issue_type_infrastructure
labels
Mar 28, 2023
seanlip
removed this from [Team] Core Learner and Mastery flows & UI Frontend - Android
Mar 29, 2023
github-project-automation
bot
moved this to Needs Triage
in [Team] Developer Workflow & Infrastructure - Android
Mar 29, 2023
6 tasks
seanlip
pushed a commit
that referenced
this issue
Jun 9, 2023
) ## Explanation Fixes #5022 Fixes #5030 Fixes #5031 First, it was found during investigation that some images aren't showing up in Arabic or Brazilian Portuguese translations of lessons due to their image control character (that indicates to Android an image needs to be rendered) being replaced by empty reading text. This has been addressed with a more robust content description check and three new tests to detect this case and addresses #5022 and #5030. This was determined due to an excellent investigation & finding by @adhiamboperes. Second, some specific images wouldn't render in Arabic specifically. This was due to a bug in AndroidSVG (our dependency library used for rendering SVG files). Per https://github.com/oppia/androidsvg/blob/4bc1d26412f0fb9fd4ef263fa93f6a64f4d4dbcf/androidsvg/src/main/java/com/caverock/androidsvg/utils/CSSFontVariationSettings.java#L73 AndroidSVG uses ``DecimalFormat`` to format certain CSS font settings (particularly in the case for dynamic fonts), and AndroidSVG isn't following the conventional advice of https://developer.android.com/reference/java/util/Locale (under "Be wary of the default locale") and https://docs.oracle.com/javase/8/docs/api/java/text/DecimalFormat.html (specific advice to create formatters for specific locales). In summary, AndroidSVG was formatting certain CSS parameters to floats without accounting for the fact that the default locale may change the numeral system when formatting those decimals. A patch to Oppia's fork of AndroidSVG fixes this problem (oppia/androidsvg@1265eb1). Note that this fix will also be included in the later AndroidSVG changes that the app will later be moved to. Additionally, these are unfortunately not changes that can be easily tested without finding specific SVGs to include in tests, and either scanning logcat or relying on screenshot testing. A more suitable testing strategy would be end-to-end tests vis-a-vis #3839. Third, bullets weren't rendering correctly in Arabic views of lessons (#5031). This was due to two issues: 1. The previous logic was always assuming a rightward shift to center the bullet which doesn't work in RTL. 2. The previous logic was using the canvas width to invert X coordinates rather than the clipped space. This wasn't caught previously because the main tested content for bullets was a test revision card whose content spans the width of the screen (which corresponds to the drawing area of the canvas). However, lessons use a more constrained area due to content margins, so the bullets were being placed off screen in the margin whitespace that was being clipped. See the following screenshots for a demonstration of the new content description tests failing without the fix, and passing with it: | Without fix | With fix | |--------|--------| | ![image](https://github.com/oppia/oppia-android/assets/12983742/bef30eb2-a966-426c-8902-8ee23850d94a) | ![image](https://github.com/oppia/oppia-android/assets/12983742/4a69feae-13de-47ad-acb5-c28f4ee72dc3) | A few caveats with the bullet fixes: - The FAQ bullets will still not render correctly in Arabic (technically in all non-English languages) due to #5034 which requires fixes in the translation strings. They're actually missing the necessary markup elements needed to show their bullet lists correctly. - The FAQ style fix can only be tested via basic measurement verification, but fortunately the existing test did require an update and changed in the expected way (the calculated x position of the bullets in the test have moved left 18 pixels). This isn't nearly as good as a visual test, but that will require #1815 to be finished, first. **Note** that this PR will need to be cherry-picked into the 0.11 release branch. ## Essential Checklist - [x] 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: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only Demonstrations of fixes for #5022 (note that, for the first Arabic screenshots, the lesson image isn't actually localized in the content; also, the Naija image does load without the PR fixes but it can be a bit delayed): | Before (without fixes) | After (with the fixes of this PR) | |--------|--------| | ![image](https://github.com/oppia/oppia-android/assets/12983742/6039721d-37e0-4d63-9c70-47cf60548e51) | ![image](https://github.com/oppia/oppia-android/assets/12983742/7375e028-0d00-4aee-9c14-11cbde10d536) | | ![image](https://github.com/oppia/oppia-android/assets/12983742/8fdc7a59-4f49-4ce0-b166-68e3c69377f4) | ![image](https://github.com/oppia/oppia-android/assets/12983742/90f80b21-308f-49df-b977-2454ed0a7cef) | | ![image](https://github.com/oppia/oppia-android/assets/12983742/b5a912b1-7583-40d8-9632-1a7e5562c1e1) | ![image](https://github.com/oppia/oppia-android/assets/12983742/04a6e2b5-0e71-4e30-8b47-b1c99e16d3cf) | Demonstrations of fixes for #5030 (only some images actually hit this bug): | Before (without fixes) | After (with the fixes of this PR) | |--------|--------| | ![image](https://github.com/oppia/oppia-android/assets/12983742/95a989d6-88d0-4c85-ade8-c86356a0986a) | ![image](https://github.com/oppia/oppia-android/assets/12983742/905dc36b-de9a-45c1-8ee3-1c202d2602c4) | | ![image](https://github.com/oppia/oppia-android/assets/12983742/2cab94b6-c13c-48e5-b53c-7c2826c6f413) | ![image](https://github.com/oppia/oppia-android/assets/12983742/b5f4d2f9-dafb-476c-8763-eca84f407431) | | ![image](https://github.com/oppia/oppia-android/assets/12983742/0c292e78-a00a-45cb-abb9-9bd141dfc7ff) | ![image](https://github.com/oppia/oppia-android/assets/12983742/6d67652f-b15a-421e-8b87-8ed4365d19f3) | Demonstrations of fixes for #5022, #5030, and #5031 (since Arabic lessons have conflated failures for images, and some of these images also demonstrate the bullet list issue): | Before (without fixes) | After (with the fixes of this PR) | |--------|--------| | ![image](https://github.com/oppia/oppia-android/assets/12983742/78a6d788-3968-4441-afcf-f7ea9cde270c) | ![image](https://github.com/oppia/oppia-android/assets/12983742/08b02727-a6fe-43d9-9631-acdd9f03fe02) | | ![image](https://github.com/oppia/oppia-android/assets/12983742/8e4943e6-6277-4e97-80f8-a57e384c5354) | ![image](https://github.com/oppia/oppia-android/assets/12983742/d292e871-7ebc-425f-9127-49edbad92ff7) | | ![image](https://github.com/oppia/oppia-android/assets/12983742/315d65a9-10ff-43e8-b472-322df5f4ef37) | ![image](https://github.com/oppia/oppia-android/assets/12983742/b62e0226-aec4-4a49-acc4-f6f83c1c7509) | --------- Co-authored-by: Adhiambo Peres <[email protected]>
adhiamboperes
added a commit
to adhiamboperes/oppia-android
that referenced
this issue
Jun 9, 2023
oppia#5040) ## Explanation Fixes #5022 Fixes #5030 Fixes oppia#5031 First, it was found during investigation that some images aren't showing up in Arabic or Brazilian Portuguese translations of lessons due to their image control character (that indicates to Android an image needs to be rendered) being replaced by empty reading text. This has been addressed with a more robust content description check and three new tests to detect this case and addresses #5022 and #5030. This was determined due to an excellent investigation & finding by @adhiamboperes. Second, some specific images wouldn't render in Arabic specifically. This was due to a bug in AndroidSVG (our dependency library used for rendering SVG files). Per https://github.com/oppia/androidsvg/blob/4bc1d26412f0fb9fd4ef263fa93f6a64f4d4dbcf/androidsvg/src/main/java/com/caverock/androidsvg/utils/CSSFontVariationSettings.java#L73 AndroidSVG uses ``DecimalFormat`` to format certain CSS font settings (particularly in the case for dynamic fonts), and AndroidSVG isn't following the conventional advice of https://developer.android.com/reference/java/util/Locale (under "Be wary of the default locale") and https://docs.oracle.com/javase/8/docs/api/java/text/DecimalFormat.html (specific advice to create formatters for specific locales). In summary, AndroidSVG was formatting certain CSS parameters to floats without accounting for the fact that the default locale may change the numeral system when formatting those decimals. A patch to Oppia's fork of AndroidSVG fixes this problem (oppia/androidsvg@1265eb1). Note that this fix will also be included in the later AndroidSVG changes that the app will later be moved to. Additionally, these are unfortunately not changes that can be easily tested without finding specific SVGs to include in tests, and either scanning logcat or relying on screenshot testing. A more suitable testing strategy would be end-to-end tests vis-a-vis oppia#3839. Third, bullets weren't rendering correctly in Arabic views of lessons (oppia#5031). This was due to two issues: 1. The previous logic was always assuming a rightward shift to center the bullet which doesn't work in RTL. 2. The previous logic was using the canvas width to invert X coordinates rather than the clipped space. This wasn't caught previously because the main tested content for bullets was a test revision card whose content spans the width of the screen (which corresponds to the drawing area of the canvas). However, lessons use a more constrained area due to content margins, so the bullets were being placed off screen in the margin whitespace that was being clipped. See the following screenshots for a demonstration of the new content description tests failing without the fix, and passing with it: | Without fix | With fix | |--------|--------| | ![image](https://github.com/oppia/oppia-android/assets/12983742/bef30eb2-a966-426c-8902-8ee23850d94a) | ![image](https://github.com/oppia/oppia-android/assets/12983742/4a69feae-13de-47ad-acb5-c28f4ee72dc3) | A few caveats with the bullet fixes: - The FAQ bullets will still not render correctly in Arabic (technically in all non-English languages) due to oppia#5034 which requires fixes in the translation strings. They're actually missing the necessary markup elements needed to show their bullet lists correctly. - The FAQ style fix can only be tested via basic measurement verification, but fortunately the existing test did require an update and changed in the expected way (the calculated x position of the bullets in the test have moved left 18 pixels). This isn't nearly as good as a visual test, but that will require oppia#1815 to be finished, first. **Note** that this PR will need to be cherry-picked into the 0.11 release branch. ## Essential Checklist - [x] 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: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only Demonstrations of fixes for #5022 (note that, for the first Arabic screenshots, the lesson image isn't actually localized in the content; also, the Naija image does load without the PR fixes but it can be a bit delayed): | Before (without fixes) | After (with the fixes of this PR) | |--------|--------| | ![image](https://github.com/oppia/oppia-android/assets/12983742/6039721d-37e0-4d63-9c70-47cf60548e51) | ![image](https://github.com/oppia/oppia-android/assets/12983742/7375e028-0d00-4aee-9c14-11cbde10d536) | | ![image](https://github.com/oppia/oppia-android/assets/12983742/8fdc7a59-4f49-4ce0-b166-68e3c69377f4) | ![image](https://github.com/oppia/oppia-android/assets/12983742/90f80b21-308f-49df-b977-2454ed0a7cef) | | ![image](https://github.com/oppia/oppia-android/assets/12983742/b5a912b1-7583-40d8-9632-1a7e5562c1e1) | ![image](https://github.com/oppia/oppia-android/assets/12983742/04a6e2b5-0e71-4e30-8b47-b1c99e16d3cf) | Demonstrations of fixes for #5030 (only some images actually hit this bug): | Before (without fixes) | After (with the fixes of this PR) | |--------|--------| | ![image](https://github.com/oppia/oppia-android/assets/12983742/95a989d6-88d0-4c85-ade8-c86356a0986a) | ![image](https://github.com/oppia/oppia-android/assets/12983742/905dc36b-de9a-45c1-8ee3-1c202d2602c4) | | ![image](https://github.com/oppia/oppia-android/assets/12983742/2cab94b6-c13c-48e5-b53c-7c2826c6f413) | ![image](https://github.com/oppia/oppia-android/assets/12983742/b5f4d2f9-dafb-476c-8763-eca84f407431) | | ![image](https://github.com/oppia/oppia-android/assets/12983742/0c292e78-a00a-45cb-abb9-9bd141dfc7ff) | ![image](https://github.com/oppia/oppia-android/assets/12983742/6d67652f-b15a-421e-8b87-8ed4365d19f3) | Demonstrations of fixes for #5022, #5030, and oppia#5031 (since Arabic lessons have conflated failures for images, and some of these images also demonstrate the bullet list issue): | Before (without fixes) | After (with the fixes of this PR) | |--------|--------| | ![image](https://github.com/oppia/oppia-android/assets/12983742/78a6d788-3968-4441-afcf-f7ea9cde270c) | ![image](https://github.com/oppia/oppia-android/assets/12983742/08b02727-a6fe-43d9-9631-acdd9f03fe02) | | ![image](https://github.com/oppia/oppia-android/assets/12983742/8e4943e6-6277-4e97-80f8-a57e384c5354) | ![image](https://github.com/oppia/oppia-android/assets/12983742/d292e871-7ebc-425f-9127-49edbad92ff7) | | ![image](https://github.com/oppia/oppia-android/assets/12983742/315d65a9-10ff-43e8-b472-322df5f4ef37) | ![image](https://github.com/oppia/oppia-android/assets/12983742/b62e0226-aec4-4a49-acc4-f6f83c1c7509) | --------- Co-authored-by: Adhiambo Peres <[email protected]>
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
enhancement
End user-perceivable enhancements.
Impact: Medium
Moderate perceived user impact (non-blocking bugs and general improvements).
Issue: Needs Break-down
Indicates that an issue is too large and should be broken into smaller chunks.
Issue: Needs Clarification
Indicates that an issue needs more detail in order to be able to be acted upon.
Work: High
It's not clear what the solution is.
Z-ibt
Temporary label for Ben to keep track of issues he's triaged.
Screenshot-testing
Screenshot testing is a kind of testing that is used to detect unintentional visual changes or regressions in the UI.
This can be done by comparing two screenshots together, one is the reference (the golden) screenshot, and the other is the screenshot of the app after some code change.
Sub-Tasks
The screenshot testing process can be divided into the following tasks:
Record
It is the process of capturing the golden/reference screenshots so that they can be used in later verifications.
Verify
The actual testing in which a new screenshot is taken to be tested, then it is compared with the reference screenshot that was taken during the “record” process. If the two screenshots are identical (they are pixel-perfect), the test passes, if not, the test fails.
Update
It is the process of updating the current golden screenshots. It is used when we intentionally change the UI and want to take a new screenshot of that UI so that it becomes the new reference.
The text was updated successfully, but these errors were encountered: