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

Generate Screenshots in CI #715

Merged
merged 10 commits into from
Nov 25, 2023
Merged

Conversation

lcreid
Copy link
Contributor

@lcreid lcreid commented Nov 4, 2023

Generate screenshots in CI, and add a commit if any screenshots change. Add some documentation that explains how to deal with screenshots changing.

There are a couple of examples (50, 51) that appear to be failing. I've added #714 for them.

I put the sequence of steps to commit the changes in a Rake task, thinking it would allow us to run the identical steps locally, if needed. Also, it's a bit easier to handle the long commit message in Ruby, at least for me. Does that make sense?

The git push when screenshots change triggers another CI run. To avoid that, I added [skip ci] to the commit message, but I fear that it would prevent CI runs when, for example, one rebases on the upstream main. The last commit will still have [skip ci] and so won't run. So I took it out and we'll simply live with double CI. Okay?

Fixes: #711

lcreid and others added 3 commits November 4, 2023 09:24
Run step if failure

Set working directory

Quote message

Debuggin

Check first, man

With name and email

Changed in CI
Please review changes in these files carefully, as they were
automatically generated during CI.

Revert "Changed in CI"

This reverts commit 1bf997c.

Add documentation and cleanups

Fix quoting
Please review the changes in the files in this commit
carefully, as they were automatically generated during CI.
Run `git pull` to bring the changes into your local branch.
Then, if you do not want the changes, run `git revert HEAD`.
@lcreid lcreid self-assigned this Nov 4, 2023
@lcreid lcreid requested a review from donv November 4, 2023 16:29
lcreid and others added 6 commits November 4, 2023 09:56
This reverts commit e4f9d01.
Please review the changes in the files in this commit
carefully, as they were automatically generated during CI.
Run `git pull` to bring the changes into your local branch.
Then, if you do not want the changes, run `git revert HEAD`.
@lcreid lcreid mentioned this pull request Nov 4, 2023
@lcreid
Copy link
Contributor Author

lcreid commented Nov 9, 2023

@donv This might affect your workflow more than mine. What do you think?

if: failure()
working-directory: demo
run: bundle exec rake commit

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will automatically commit all changed files in the demo directory after a failure? That does not seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't think of a better way to communicate the changed screenshots back into a place where the dev could review the screenshots and see if their code needed to be fixed. GitHub's diff for the screenshots is actually pretty slick. I admit that the revert step may be a nuisance, or people may forget it. But we should still catch changes when reviewing the PR.

I could make the workflow more specific, so we only save the screenshot changes when it was the screenshot diff that failed.

I'm open for other suggestions about how to generate and capture the screenshots when we create new ones, or the change is intended.

git pull # to bring the additional commit to your local branch.
git revert HEAD # to remove the changes.
```

Copy link
Collaborator

Choose a reason for hiding this comment

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

The workflow probably needs to differentiate between master commits and branches for pull requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really good point! Thanks!

@lcreid lcreid requested a review from donv November 10, 2023 01:11
@lcreid
Copy link
Contributor Author

lcreid commented Nov 25, 2023

I'll merge this and we can discuss better solutions later.

@lcreid lcreid merged commit 8a7274e into bootstrap-ruby:main Nov 25, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate System Test Baseline via GitHub Actions
2 participants