Skip to content

Compare screenshots with main on PRs#13248

Merged
mockersf merged 9 commits intobevyengine:mainfrom
mockersf:compare-screenshots-in-ci
Oct 14, 2024
Merged

Compare screenshots with main on PRs#13248
mockersf merged 9 commits intobevyengine:mainfrom
mockersf:compare-screenshots-in-ci

Conversation

@mockersf
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

superdump 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.

@mockersf
Copy link
Copy Markdown
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
Copy Markdown
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. :)

@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
Copy Markdown
Member

alice-i-cecile commented Jun 3, 2024

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

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jun 10, 2024
@DJMcNab DJMcNab mentioned this pull request Jun 10, 2024
Closed
3 tasks
@mockersf mockersf mentioned this pull request Jul 3, 2024
Merged
@mockersf
Copy link
Copy Markdown
Member Author

mockersf commented Oct 13, 2024 •
edited
Loading

this should be ready to be merged, but can't be unless we (temporarily) remove the job run-examples-macos-metal from the required checks: I needed to move it to a pull_request_target trigger for it to be able to access secrets, which means it runs from its definition on the base branch of the PR, and this job in this workflow doesn't exist yet there

@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Oct 13, 2024

I'm on board with that plan: please feel free to do that and merge.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 13, 2024
@mockersf mockersf added this pull request to the merge queue Oct 14, 2024
Merged via the queue into bevyengine:main with commit 82d6f56 Oct 14, 2024
@mockersf mockersf mentioned this pull request Oct 14, 2024
Merged
@mockersf
Copy link
Copy Markdown
Member Author

mockersf commented Oct 14, 2024 •
edited
Loading

removed the required check, merged the pr, added the required check back, notice the screenshot upload failed 😭
#15894 should fix it

github-merge-queue bot pushed a commit that referenced this pull request Oct 14, 2024
@mockersf
Fix screenshot comparison (#15894)
bd912c2 
# Objective

- After merging #13248 the new upload job fails

## Solution

- Fix the file path
- Instead of a pull_request_target workflow, keep the examples in the
pull_request workflow and add another job that will run once its all
completed on a `workflow_run` event to upload screenshots

## Testing

- Tested in a ubuntu docker container, running the exact same script
- Manual result:
https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D/run/5/compare/2
- The CI on this job will still fail as its using the job from main
@mockersf mockersf mentioned this pull request Oct 15, 2024
Open
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

+1 more reviewer

@BD103 BD103 BD103 approved these changes

Reviewers whose approvals may not affect merge requirements

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.

5 participants

@mockersf @hymm @superdump @alice-i-cecile @BD103