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

Send comment PR for failed screenshot tests. #616

Merged
merged 4 commits into from Sep 21, 2022
Merged

Send comment PR for failed screenshot tests. #616

merged 4 commits into from Sep 21, 2022
+44 −5

Conversation

eneim
Copy link
Contributor

@eneim eneim commented Sep 19, 2022 •
edited

Issue

Overview (Required)

  • With this PR, a failed screenshot test will automatically push a comment to the same PR with ALL failed screenshots.

  • Idea of this PR:

    • Create a new branch called the "companion" branch. This branch is used to save the failed screenshots.
    • Failed screenshots are uploaded to that branch, the URLs will be used in the PR comment.
    • Note: for now the branch is not deleted automatically. Let's have a TODO for that 🙇 .

  • Currently the comment includes all failed screenshot test. It can be quite big if the failed cases are big (see the demo below). We can setup further filtering rules next.

Links

@eneim eneim marked this pull request as draft September 19, 2022 12:25
@eneim
Copy link
Contributor Author

eneim commented Sep 19, 2022

Convert to draft to fix some naming in the yaml file.

@eneim eneim mentioned this pull request Sep 19, 2022
Closed
@eneim eneim force-pushed the snapshot_error_to_pr_comment branch from 9a07562 to 35b7d19 Compare September 19, 2022 12:32
@eneim eneim marked this pull request as ready for review September 19, 2022 12:32
@eneim eneim force-pushed the snapshot_error_to_pr_comment branch from 35b7d19 to 251d8a8 Compare September 19, 2022 12:33
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 19, 2022 13:13 Inactive
@takahirom
Copy link
Member

I'm not sure if I understood.
Unfortunately, We are using GitHub pages for Zipline.
Probably GitHub page is only one page per repository.
https://github.com/DroidKaigi/conference-app-2022/blob/main/.github/workflows/DeployZipline.yml#L50
https://droidkaigi.github.io/conference-app-2022/manifest.zipline.json

@eneim
Copy link
Contributor Author

eneim commented Sep 19, 2022

As per this comment: #616 (comment), let me convert this PR to draft to work on the "companion branch" approach.

@eneim eneim marked this pull request as draft September 19, 2022 13:57
@eneim eneim force-pushed the snapshot_error_to_pr_comment branch from 251d8a8 to 99f0344 Compare September 20, 2022 12:29
@eneim eneim marked this pull request as ready for review September 20, 2022 12:32
@eneim
Copy link
Contributor Author

eneim commented Sep 20, 2022

Updated: 50530e7

Now this PR will create a new companion branch to store the screenshot result and access images from there to show the failed test in the comment.

@eneim eneim force-pushed the snapshot_error_to_pr_comment branch from 50530e7 to 05166ae Compare September 20, 2022 12:34
@eneim
Copy link
Contributor Author

eneim commented Sep 20, 2022

(I notice that the screenshots in the failure folder include the original screenshots too, we can filter them out somehow 🤔 ).

@takahirom
Copy link
Member

👀

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 21, 2022 00:01 Inactive
@takahirom takahirom mentioned this pull request Sep 21, 2022
Closed
takahirom
takahirom reviewed Sep 21, 2022
View reviewed changes
.github/workflows/ScreenShotTest.yml
run: |
echo "There are differences in Compose previews:" > report.md
echo >> report.md # A blank line.
echo "$ALL_SCREENSHOTS" | jq -r '.[]' | while read -r image; do
Copy link
Member

Choose a reason for hiding this comment

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

It works well!
Can we grep "delta"?

#640

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see, the diffed image names start with "delta". We can filter it to only include the failed tests. Let me quickly try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: eneim#1 (comment) will bring the change here in a few seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: e9b694c

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 21, 2022 10:25 Inactive
takahirom
takahirom approved these changes Sep 21, 2022
View reviewed changes
Copy link
Member

@takahirom takahirom left a comment

Choose a reason for hiding this comment

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

Thank you for making our dreams come true!

@takahirom takahirom merged commit 48b5ece into DroidKaigi:main Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@takahirom takahirom takahirom approved these changes

Assignees
No one assigned
Labels
awesome
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can you suggest any good way to post Compose preview differences in the PR?
2 participants
@eneim @takahirom