chore(percy): added automated targeted Percy build on PRs and releases #152
Conversation
|
There was a problem hiding this comment.
LGTM overall, couple of comments and some questions just so I understand this better.
My main concern is how are we extracting the stories from the body. I see its being passed through /Percy Stories/ but my concern could this be a security issue?
Could we maybe look through the changed scss files and pass it in there?
Or we can create a depdency map (we have one already since some components depend on others)
| # Extract target stories from PR body | ||
| - name: Extract target stories | ||
| id: extract_stories | ||
| env: | ||
| PR_BODY: ${{ github.event.pull_request.body }} | ||
| run: | | ||
| # Use the PR_BODY environment variable securely | ||
| if [[ "$PR_BODY" == *"package: skin"* ]]; then | ||
| STORIES=$(echo "$PR_BODY" | awk '/Percy Stories/{getline; print}') | ||
| if [ -z "$STORIES" ]; then | ||
| echo "No Percy Stories found in PR body." | ||
| exit 0 | ||
| fi | ||
| echo "stories=$STORIES" >> $GITHUB_ENV | ||
| else | ||
| echo "No relevant PR found for package: skin." | ||
| exit 0 | ||
| fi | ||
| working-directory: ./packages/skin |
There was a problem hiding this comment.
Is this section needed for the baesline? I assume when a PR merges it just gets a new baseline story right?
There was a problem hiding this comment.
Is this section needed for the baesline? I assume when a PR merges it just gets a new baseline story right?
I think there may be some confusion around how Percy works and the current process.
Doing baselines is currently a manual process. I've been running full baseline builds from master after releases to create new baselines. If Percy had the ability to automatically create new baselines by itself when we did releases, we would have definitely been doing that all of this time or at least thought about it. AFAIK, Percy has no awareness of repositories. It doesn't check for merges itself and run builds; don't think it's capable of doing that.
With our recent trunk based development, without automating this targeted build on releases, we would have to do that manually and since our releases are going to be smaller and more frequent, full builds on each release are going to be too much. Bottom line is we'd have to do this same thing manually - targeted Percy runs on each release.
This change basically automates that process. This runs targeted baseline builds to create baselines only on the components touched for the PR, which are only the ones that were touched by the PR. This streamlines visual regression with very little effort.
There was a problem hiding this comment.
So here's my issue with the way this is
If I create a PR, merge it, and it runs, then it updates baselines for that PR. If that PR is missing those lines OR they are incorrect, it will only update a certain set of baselines.
The other issue is if we merge two PRs at the same time, only the latest one will run and update only those.
IMO, I think we should maybe run this once a day and update all snapshots. If that seems to be too much we can either run it once a week, or have a way to trigger it github manually when we want to update.
There was a problem hiding this comment.
If that PR is missing those lines OR they are incorrect, it will only update a certain set of baselines.
If this happens, then something terribly wrong has happened with the PR itself, because the whole point of the visual regression is to ensure we're covering all the changes to the appropriate stories. If those are not mentioned in the PR, that's already a failure at the first level. This simply creates baselines for the very first stories that were included, which is what should happen. That's typically all we care about. IMO, it's not a big deal to ensure the PR has the correct stories mentioned in the body.
The other issue is if we merge two PRs at the same time, only the latest one will run and update only those.
When would we merge two PRs at virtually the same time? If we have to concern ourselves with this possibility, then no automation will ever work.
IMO, I think we should maybe run this once a day and update all snapshots.
I don't this is a realistic or practical solution. The whole point is to automate visual regression as a whole. It's not practical to have someone run automation every single day. Also, if we did that, we would deplete the quota we have extremely quickly.
If that seems to be too much we can either run it once a week, or have a way to trigger it github manually when we want to update.
Once a week is better and may not deplete our quota completely. However, if there is more than one change at all touching components that are common in changesets during a single week, the diffs are going to be compared against obsolete baselines. This won't work IMO.
| # Debug: Print extracted stories | ||
| - name: Debug extracted stories | ||
| run: | | ||
| STORIES=$(echo "$PR_BODY" | awk '/Percy Stories/{getline; print}') |
There was a problem hiding this comment.
Where is /Percy Stories/? Is that a url or directory?
Im mainly looking to see how this parses info so that if the PR_BODY has garbage it doesn't get hacked.
There was a problem hiding this comment.
/Percy Stories/ is just the regex for the string it's looking for in the PR body. If garbage is included, I believe Percy run should just fail.
| # Install dependencies | ||
| - name: Install dependencies | ||
| run: npm install | ||
| working-directory: ./packages/skin |
There was a problem hiding this comment.
I believe all the dev depdencies are in the root. Might want to move that to the root.
There was a problem hiding this comment.
I thought about that before going in this direction. What benefits would that have vs. this option? I'm fine doing that, but it's a fairly large change... trying to make sure the benefits outweigh the cost.
There was a problem hiding this comment.
Im just worried that some dev deps might not be added
There was a problem hiding this comment.
Yeah, I think this CI issue is due to no dev deps being installed. Should probably remove working-directory
Error: Cannot find module 'glob'
Require stack:
- /home/runner/work/evo-web/evo-web/packages/skin/scripts/generate-bundle.js
- /home/runner/work/evo-web/evo-web/packages/skin/scripts/index.js
at Module._resolveFilename (node:internal/modules/cjs/loader:1212:15)
at Module._load (node:internal/modules/cjs/loader:1043:27)
at Module.require (node:internal/modules/cjs/loader:1298:19)
at require (node:internal/modules/helpers:182:18)
at Object.<anonymous> (/home/runner/work/evo-web/evo-web/packages/skin/scripts/generate-bundle.js:3:14)
at Module._compile (node:internal/modules/cjs/loader:1529:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1613:10)
at Module.load (node:internal/modules/cjs/loader:1275:[32](https://github.com/eBay/evo-web/actions/runs/15423370047/job/43404096987#step:7:33))
at Module._load (node:internal/modules/cjs/loader:1096:12)
at Module.require (node:internal/modules/cjs/loader:1298:19) {
code: 'MODULE_NOT_FOUND',
requireStack: [
'/home/runner/work/evo-web/evo-web/packages/skin/scripts/generate-bundle.js',
'/home/runner/work/evo-web/evo-web/packages/skin/scripts/index.js'
]
}
There was a problem hiding this comment.
Isn't it just missing glob? Aren't all the other dependencies in /package/skin? Are you saying move everything under skin to the root as a dependency?
Agreed. I like this approach. It gets us one big step closer to the end goal of a fully automated solution (from what I read, there are still some complexities to figure out before we can get to that end goal). |
This PR automates targeted Percy visual regression runs for specific stories on PRs as well as running the new baselines for those specific stories after PRs are merged to main.
Current Approach with Explicit Story Definitions
Here's the current process built into the scripts:
Percy Stories
(replace this with the Percy stories, like so: Toggle Button,Button,Filter Button)
Thoughts? I'm happy to change the process if a simpler one can be offered. The main hurdle here were the scripts to get these going, and based on how the testing pans out, I think I may have solved that piece. Not having to run Percy locally and automating runs should improve workflow.
A Potential Secondary Approach Using Implied Stories from Files Changed
With this approach, the system will find the files that are changed, find a way to map the files changed to their respective story names (this is what Percy needs), and then use that list for the targeted build.
The difficulty with this approach is either assuming file names of the changes will follow the same exact pattern as the Story names, or (if not assuming it), it would need to have a file name:story name mapping somewhere. It would be more automated with less manual effort, but possibly more difficult to pull off.