-
Notifications
You must be signed in to change notification settings - Fork 105
Document steps for updating prod screenshots in tests #469
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
Document steps for updating prod screenshots in tests #469
Conversation
|
@seanlip PTAL, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mon4our for doing this! Left a few notes, PTAL.
Acceptance-Tests.md
Outdated
| On the other hand, if the screenshot fails locally (in desktop environment), the screenshot `teachPage-diff.png` will be generated and stored inside a new folder `diff-snapshots` under `logged-out-user/dev-desktop-screenshots`. | ||
|
|
||
| ### Adding Prod screenshots for Acceptance Tests | ||
| On making changes that affect a user journey tested through the acceptance tests or introduce a new feature, a contributor needs to update/add screenshots supporting their changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On --> When
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Acceptance-Tests.md
Outdated
|
|
||
| ### Adding Prod screenshots for Acceptance Tests | ||
| On making changes that affect a user journey tested through the acceptance tests or introduce a new feature, a contributor needs to update/add screenshots supporting their changes. | ||
| It has been noted that screenshots obtained by running the Acceptance tests locally using the prod flag do not match those captured by the CI. To overcome this, you can commit any image in the prod screenshot folder and follow these steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop the first sentence, replace with: "These screenshots should be obtained from the CI (not local) run, so that the environment matches future runs." Update the second sentence to follow on from that.
In the second sentence, what do you mean by "commit any image in the prod screenshot folder"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Acceptance-Tests.md
Outdated
| ### Adding Prod screenshots for Acceptance Tests | ||
| On making changes that affect a user journey tested through the acceptance tests or introduce a new feature, a contributor needs to update/add screenshots supporting their changes. | ||
| It has been noted that screenshots obtained by running the Acceptance tests locally using the prod flag do not match those captured by the CI. To overcome this, you can commit any image in the prod screenshot folder and follow these steps: | ||
| 1. Go to the failing run and click on the 'uploading diff screenshots as an artifact' section: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalize "uploading"
"click on the" --> "open the"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
| 1. Go to the failing run and click on the 'uploading diff screenshots as an artifact' section: | ||
| <img width="349" height="35" alt="image" src="https://github.com/user-attachments/assets/827d8557-3346-443a-aaf8-1fd0168a3f24" /> | ||
|
|
||
| 2. Then download the artifact from the artifact download url: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Highlight that URL in the image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Acceptance-Tests.md
Outdated
| 2. Then download the artifact from the artifact download url: | ||
| <img width="1221" height="301" alt="image" src="https://github.com/user-attachments/assets/c2caa55f-b4f9-40cc-a47a-7ddc2de9a479" /> | ||
|
|
||
| 3. Extract the contents of the artifact. There will be a folder with an image. The image will be like this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be like this --> look like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Acceptance-Tests.md
Outdated
|
|
||
| (Its curriculum-admin in this case) | ||
|
|
||
| 14. Navigate to the spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unclear, please specify what you mean by "spec". How do they figure out which spec to navigate to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Acceptance-Tests.md
Outdated
| 15. Check if the failure was for desktop or mobile based on the screenshot or the subfolder in the artifact: | ||
| <img width="102" height="127" alt="image" src="https://github.com/user-attachments/assets/c1b902ee-5ccb-4218-a247-d4e19da9d3ed" /> | ||
|
|
||
| 16. Go to the prod-desktop-screenshots/prod-mobile-screenshots folder depending on your failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use "or" instead of "/" to avoid ambiguity, since a slash is a folder name separator too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Acceptance-Tests.md
Outdated
|
|
||
| 16. Go to the prod-desktop-screenshots/prod-mobile-screenshots folder depending on your failure | ||
|
|
||
| 17. Paste the image we split. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to the step number where the splitting action was taken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Acceptance-Tests.md
Outdated
|
|
||
| 17. Paste the image we split. | ||
|
|
||
| 18. Check the name of the image from the artifact. It will be {screenshot_name}-diff.png |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can combine steps 18-21 into something along the lines of "replace the old screenshot with the new one"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Acceptance-Tests.md
Outdated
|
|
||
| 20. Rename the pasted image to {screenshot_name}-snap.png | ||
|
|
||
| 21. Check that the correct image got replaced, commit and push your changes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Commit and push your changes" should be a separate step
Maybe at the end you can also ask them to do a self-review of the PR to verify that the correct images are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
|
@mon4our Just to check, any reason this isn't wrapped up yet? It seems like it'd be quite useful to have on the wiki. |
Updated instructions for adding production screenshots in acceptance tests to clarify the process and improve readability.
|
@seanlip Made the requested changes. PTAL, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks @mon4our!
Updated the acceptance tests wiki with steps on how to update screenshots