-
Notifications
You must be signed in to change notification settings - Fork 25
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
CI : Add visual regression testing workflow based on Playwright and Argos. #511
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for permitio-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@gemanor @filipermit Here's the PR and can you approve workflow? It had passed for me locally and lets see here in upstream |
Let me connect the Argos app to the repository, so we can run it on a fork |
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, left some comments.
You also need to add the following to the .gitignore
so it will make it available for local testing:
# Playwright
node_modules/
/test-results/
/playwright-report/
/blob-report/
/playwright/.cache/
screenshots
|
||
- name: Upload screenshots to Argos | ||
env: | ||
ARGOS_TOKEN: ${{ secrets.ARGOS_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to remove it, see if we can run the workflow without it
run: yarn playwright install --with-deps chromium | ||
|
||
- name: Build the website | ||
run: yarn docusaurus build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why yarn and not npm/npx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho npm
would better suit our case here.
source: https://www.syncfusion.com/blogs/post/pnpm-vs-npm-vs-yarn
iframe, | ||
.avatar__photo, | ||
img[src$='.gif'], | ||
.DocSearch-Button-Keys > kbd, | ||
[class*='playgroundPreview'] { | ||
visibility: hidden; | ||
} | ||
|
||
.theme-last-updated, | ||
.docusaurus-mermaid-container { | ||
display: none; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please implement the recommendation from the comment in the issue (for video, for example)
iframe, | |
.avatar__photo, | |
img[src$='.gif'], | |
.DocSearch-Button-Keys > kbd, | |
[class*='playgroundPreview'] { | |
visibility: hidden; | |
} | |
.theme-last-updated, | |
.docusaurus-mermaid-container { | |
display: none; | |
} | |
/* Iframes can load lazily */ | |
iframe, | |
/* Avatars can be flaky due to using external sources: GitHub/Unavatar */ | |
.avatar__photo, | |
/* Gifs load lazily and are animated */ | |
img[src$='.gif'], | |
/* Algolia keyboard shortcuts appear with a little delay */ | |
.DocSearch-Button-Keys>kbd, | |
/* The live playground preview can often display dates/counters */ | |
[class*='playgroundPreview'] { | |
visibility: hidden; | |
} | |
/* Different docs last-update dates can alter layout */ | |
.theme-last-updated, | |
/* Mermaid diagrams are rendered client-side and produce layout shifts */ | |
.docusaurus-mermaid-container { | |
display: none; | |
} | |
/* Videos are laggy and the loading spinner is shown as change on Argos */ | |
video { | |
visibility: hidden; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this from docs/utils
since it's only used for tests.
It's only 17 lines, so I'd say it's ok to leave it at screenshot.spec.ts
Alternatively, you can create a tests
folder to put screenshot.css
, screenshot.spec.ts
and this helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a specific reason for using typescript here?
I haven't seen ts anywhere else in the codebase, so it's safer to assume we should keep it JavaScript-only (as it does not offer any advantage other than code completion—which can be achieved via JSDocs—for a simple implementation like this).
I know technically playwright
handles the TS overhead (transpilation, etc), but I'm afraid this will confuse future testers; should they be done in JS or TS? If they're done outside of playwright scope (e.g unit/integration tests) we'd need to add tsc
and typescript
to the repo.
- name: Use Node.js | ||
uses: actions/setup-node@v3 | ||
with: | ||
node-version: current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current
v23 will be gone in April and can introduce bugs that bjork the pipeline.
personally I'd go with v22
/22
, but if you find a lts
option for this, that would be fantastic too.
No description provided.