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

add percy integration #124

Open
wants to merge 39 commits into
base: develop
Choose a base branch
from
Open

add percy integration #124

wants to merge 39 commits into from

Conversation

dpatil-magento
Copy link
Collaborator

Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):

Fix #

Test URLs:
Before: https://develop--aem-boilerplate-commerce--hlxsites.hlx.live/
After: https://percy-demo-new--aem-boilerplate-commerce--hlxsites.hlx.live/

Copy link

aem-code-sync bot commented Oct 7, 2024

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link

aem-code-sync bot commented Oct 7, 2024

Page Scores Audits Google
📱 / PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
🖥️ / PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Comment on lines 28 to 30
Cypress.on('uncaught:exception', (err, runnable) => {
return false;
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is intent of this callback? Doesn't seem to do anything :D

cy.visit('');
cy.get('.nav-drop')
.contains('Catalog')
.click();
// Randomly empty click is triggered
Copy link
Collaborator

Choose a reason for hiding this comment

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

wdym randomly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/hlxsites/aem-boilerplate-commerce/actions/runs/11219937102/job/31187005036 guest checkout failed twice. I could see this on my local where cypress was clicking on product link too fast that empty click happened so, cypress was expecting PDP page but app is still on home page. After adding this wait, i have not reproduced it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the format for comments like `// X because Y

// adding a wait because product link is clicked too quickly

But more importantly, you could probably avoid this by waiting for some specific element that exists on pdp to appear, rather than an arbitrary wait.

Comment on lines 82 to 86
cy
.viewport('iphone-x')
.percySnapshot('Auth Create Account', { width: 375 })
.viewport(1280, 1024)
.percySnapshot('Auth Create Account', { width: 1280 });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, maybe we should wrap this into a util function likefunction takeSnapshot(reason:string, widths: number[])?

I see you repeating this block a bit.

command: npm run cypress:percy
env:
# pass the Percy Token
PERCY_TOKEN: ${{ secrets.PERCY_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this is something that can only be run on CICD? Will it gracefully fail if we run cypress locally?

Copy link

Choose a reason for hiding this comment

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

Only the GitHub action uses percy. Locally you run cypress in your own browser via the cypres:run command.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are test lines which include calls to Percy - just making sure these won't fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Dev told me that these percy commands are just skipped and do not throw if you do not have the percy token. 👍🏼

Copy link

@herzog31 herzog31 left a comment

Choose a reason for hiding this comment

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

btw adding random waits is a bad idea. If you have to wait for something to be interactive, this usually indicates a bug in the implementation. Otherwise it usually helps explicitly checking for visibility of the element you want to interact with.

@dpatil-magento
Copy link
Collaborator Author

Thanks for feedback @sirugh @herzog31 , I was more focused on getting integration, its working now. Will clean this pr and request for re-review.

@aem-code-sync aem-code-sync bot temporarily deployed to percy-demo-new October 17, 2024 16:05 Inactive
@dpatil-magento
Copy link
Collaborator Author

dpatil-magento commented Oct 17, 2024

@sirugh @herzog31 This is ready for final review. DONT MERGE it yet, we need first team members to get access to Percy so they can easily view and accept UI changes or fix if neeed.

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