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

[UXIT-1471] Optimise Percy snapshots [skip percy] #599

Merged
merged 19 commits into from
Sep 6, 2024

Conversation

barbaraperic
Copy link
Collaborator

@barbaraperic barbaraperic commented Sep 2, 2024

📝 Description

This PR includes a few options to optimise executing Percy Snapshots - see Key Changes. ❗️See part with including husky.

  • Type: Refactor

🛠️ Key Changes

  • updates on github action - don't run tests if PR is in draft
  • updates on github action - if you include "[skip percy]" in your PR title, only Cypress tests will run (without Percy). Not an ideal solution, adds an extra load to think about. I was looking into Inquirer - we could use that library to implement a terminal prompt that could ask "Was there a visual change" when you run "git commit ...", and if the answer is positive that would add a condition to github actions to include Percy (otherwise it wouldn't include it in Cypress tests). I was playing around tho, but couldn't make it work. Could give it another go if you all think it would be a better solution.
  • I included husky - it's a tool that basically runs commands pre-commit. I thought it would be useful to run npm run lint before you push to the branch. Let me know if you want to keep it, or I can remove it.
  • only runs percy on deskop viewport - that reduces snapshots from 24 per commit to 12 per commit.

📸 Screenshots

Screenshot 2024-09-02 at 15 53 04

🔖 Resources

Copy link

vercel bot commented Sep 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
filecoin-foundation-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 6, 2024 9:49am

@mirhamasala mirhamasala changed the title disable tests on draft and run percy on deskopt vp [UXIT-1471] Disable tests on draft and run percy on deskopt vp Sep 2, 2024
Copy link

@barbaraperic barbaraperic marked this pull request as ready for review September 2, 2024 13:32
@barbaraperic barbaraperic changed the title [UXIT-1471] Disable tests on draft and run percy on deskopt vp [UXIT-1471] Optimise Percy snapshots Sep 2, 2024
@barbaraperic barbaraperic changed the title [UXIT-1471] Optimise Percy snapshots [UXIT-1471] Optimise Percy snapshots [skip percy] Sep 2, 2024
Copy link
Collaborator

@CharlyMartin CharlyMartin left a comment

Choose a reason for hiding this comment

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

Thanks @barbaraperic for looking into this!

don't run tests if PR is in draft

That's great!! It will for sure reduce our usage.

if you include "[skip percy]" in your PR title, only Cypress tests will run (without Percy)

Not an ideal solution as you pointed out, I'm sure I will forget it 😅 Inquirer could be an improvement I think, but I don't know whether it's complicated to set up.

The optimal solution would be to only run Percy on pages that changed, but I don't know how to do that. Maybe it will be possible when we setup NX or Turborepo.

I included husky

I did a test commit locally, and I don't think the linter ran. Did I miss something?

CleanShot 2024-09-04 at 10 35 25@2x

only runs percy on deskop viewport

I feel like we might miss on important information by removing small screens from Percy?

Let me know what you think :)

@mirhamasala
Copy link
Collaborator

mirhamasala commented Sep 4, 2024

@barbaraperic - I agree with @CharlyMartin on:

I feel like we might miss on important information by removing small screens from Percy?

and yes, let's wait until we setup our monorepo structure

The optimal solution would be to only run Percy on pages that changed, but I don't know how do that. Maybe it will be possible when we setup NX or Turborepo.

🙏🏼

@barbaraperic
Copy link
Collaborator Author

only runs percy on deskop viewport

I feel like we might miss on important information by removing small screens from Percy?

What if we keep just desktop screenshots for all browsers and keep both desktop and mobile screenshot for only 1 (most used) browser? Total screenshots per build would drop from 24 to 18 (saved 6). Which is 25%.

Doesn't seem a lot, but, for example, if you look at the dashboard, in one day we used 384 screenshots, with 25% saving it would have been 288 (96 saved).

Thoughts?

Screenshot 2024-09-04 at 13 17 18

@barbaraperic
Copy link
Collaborator Author

I included husky

I did a test commit locally, and I don't think the linter ran. Did I miss something?

CleanShot 2024-09-04 at 10 35 25@2x

Hm not sure what could be the issue. You ran npm install?

Copy link
Collaborator

@CharlyMartin CharlyMartin left a comment

Choose a reason for hiding this comment

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

Looks good to me 👏

FYI, I did a test commit and the linter ran with this message:

CleanShot 2024-09-06 at 11 39 53@2x

Copy link
Collaborator

@mirhamasala mirhamasala left a comment

Choose a reason for hiding this comment

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

Yay! LGTM! ✨

@barbaraperic barbaraperic merged commit 1ad7dcc into main Sep 6, 2024
4 checks passed
@barbaraperic barbaraperic deleted the bp/percy-snapshots branch September 6, 2024 12:54
@barbaraperic
Copy link
Collaborator Author

Looks good to me 👏

FYI, I did a test commit and the linter ran with this message:

CleanShot 2024-09-06 at 11 39 53@2x

Thanks Charly! I'll open a ticket to look into it 👍

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.

3 participants