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

Compare screenshots with main on PRs #13248

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Compare screenshots with main on PRs #13248

wants to merge 5 commits into from
+119 −2

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented May 5, 2024

Objective

  • Compare screenshots for a few examples between PRs and main

Solution

  • Send screenshots taken to a screenshot comparison service
  • Not completely sure every thing will work at once, but it shouldn't break anything at least
  • it needs a secret to work, I'll add it if enough people agree with this PR
  • this PR doesn't change anything on the screenshot selection (load_gltf and breakout currently), this will need rendering folks input and can happen later

@mockersf mockersf added A-Build-System Related to build systems or continuous integration C-Testing A change that impacts how we test Bevy or how users test their apps labels May 5, 2024
@hymm
Copy link
Contributor

hymm commented May 5, 2024

What happens when there's a failure? does ci just fail or are you able to see the diffs somewhere?

@mockersf
Copy link
Member Author

mockersf commented May 5, 2024

What happens when there's a failure? does ci just fail or are you able to see the diffs somewhere?

it should log an url like https://pixel-eagle.vleue.com/project/b25a040a-a980-4602-b90c-d480ab84076d/run/1335/compare/1327 that will let you see the issues

@mockersf mockersf force-pushed the compare-screenshots-in-ci branch from df365f0 to c587b8e Compare May 5, 2024 22:32
@mockersf
Copy link
Member Author

mockersf commented May 5, 2024

job is currently failing because the secret is not set

superdump
superdump approved these changes May 7, 2024
View reviewed changes
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Yes please

@superdump
Copy link
Contributor

Maybe we should put the flight helmet into the lighting scene. Or just put together a test scene which has a whole bunch of features enabled all at once to catch differences.

@mockersf
Copy link
Member Author

mockersf commented May 7, 2024

Maybe we should put the flight helmet into the lighting scene. Or just put together a test scene which has a whole bunch of features enabled all at once to catch differences.

Since #11904, the flight helmet makes rendering crash in CI on Windows

@mockersf mockersf closed this May 17, 2024
@mockersf mockersf deleted the compare-screenshots-in-ci branch May 17, 2024 21:15
@mockersf mockersf restored the compare-screenshots-in-ci branch May 17, 2024 21:15
@mockersf mockersf reopened this May 17, 2024
BD103
BD103 approved these changes May 26, 2024
View reviewed changes
Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

This will be really nice for rendering PRs, since we can catch regressions before they merge. (Such as the MacOS thing that happened a few days ago.)

I have a few nits, but they're non-blocking. :)

.github/workflows/send-screenshots-to-pixeleagle.yml

- name: Send to Pixel Eagle
run: |
project="B04F67C0-C054-4A6F-92EC-F599FEC2FD1D"
Copy link
Member

Choose a reason for hiding this comment

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

Can this be set using the env property? I feel like it signals better that this is a hardcoded constant, and may be changed in the future.

- name: Send to Pixel Eagle
  env:
    project: B04F67C0-C054-4A6F-92EC-F599FEC2FD1D
  run: ...

.github/workflows/send-screenshots-to-pixeleagle.yml
jobs:
send-to-pixel-eagle:
name: Send screenshots to Pixel Eagle
# runs on macos because it has the updated curl that will accept the --json flag, and the script is probably not portable to windows
Copy link
Member

Choose a reason for hiding this comment

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

This can be changed back to ubuntu-latest once Ubuntu 24 becomes the new default. (Installed software)

I won't block on it, though, because who knows when that will be ready. :)

.github/workflows/validation-jobs.yml
@@ -68,7 +68,7 @@ jobs:
run: ANDROID_NDK_ROOT=$ANDROID_NDK_LATEST_HOME cargo apk build --package bevy_mobile_example

run-examples-linux-vulkan:
if: ${{ github.event_name == 'merge_group' }}
if: ${{ github.event_name != 'pull_request' }}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment on why this is different, since all of the other jobs in this file uses == 'merge_group'?

.github/workflows/validation-jobs.yml
run-examples-on-windows-dx12:
if: ${{ github.event_name == 'merge_group' }}
if: ${{ github.event_name != 'pull_request' }}
Copy link
Member

Choose a reason for hiding this comment

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

Same thing as above, please!

@hymm hymm added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 28, 2024
@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Jun 3, 2024
@alice-i-cecile
Copy link
Member

@mockersf I'm happy to merge this: looks like it needs you to add a secret first?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@superdump superdump superdump approved these changes

@BD103 BD103 BD103 approved these changes

Assignees
No one assigned
Labels
A-Build-System Related to build systems or continuous integration C-Testing A change that impacts how we test Bevy or how users test their apps S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mockersf @hymm @superdump @alice-i-cecile @BD103