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.

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

Compare screenshots with main on PRs #13248

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

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants