-
Notifications
You must be signed in to change notification settings - Fork 49
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
Adding Visual Testing: Cypress + Percy #419
Conversation
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.
This is great! Can you please also provide some steps how to set it up locally?
@@ -0,0 +1,37 @@ | |||
/// <reference types="cypress" /> |
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 need this file at all?
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.
It turns out we do need this file. Without it Cypress can't resolve /commands
. See here
|
||
it('Basic Annotation', () => { | ||
cy.contains('Basic Annotations').click() | ||
cy.wait(3000) |
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.
Is there a specific reason to wait 3000 ms?
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.
Is there a specific reason to wait 3000 ms?
Some page has animations. This way we wait till everything is fully rendered before taking the screenshot.
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.
Should we add duration
as a query string argument to the pages? I think this way you can speed up the testing significantly.
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.
Should we add
duration
as a query string argument to the pages? I think this way you can speed up the testing significantly.
What do you mean? Add something like ?duration=3000
to the url?
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.
More like ?duration=0
to render charts without animation
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.
More like
?duration=0
to render charts without animation
That's a good idea.
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.
More like
?duration=0
to render charts without animation
See a POC here
cy.percySnapshot('Stacked Bar with Labels', { scope: '.exampleViewer' }) | ||
}) | ||
}) | ||
|
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.
We can try generating this file automatically based on the example files. Technically they have the required information, like the description text.
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.
We can try generating this file automatically based on the example files. Technically they have the required information, like the description text.
I suppose we could, but some page require additional settings. For example the API Endpoints Tree
requires a bigger minHeight, we'd have to manually set things like that for specific test cases. Do you have a workaround for cases like this?
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.
You can have a map of additional settings in the testing file to apply on the go if needed. I can explain more if you want, just let me know.
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 you mean to have an array or load a separate file which contains
testingArray = [
{name: 'Single Container', duration:3000, additionalParams: {width: [1300]}},
{name: 'Sankey', duration:3000}, ...
]
And iterate through the array inside the unovis.cy.ts
?
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.
Take a look at unovis/packages/dev/src/examples/index.ts
, I was thinking about something similar to generate this test file automatically. You'll need to "compile" it using Webpack but I think it will be a reasonable trade-off.
So the script will:
- Batch import all the
index.tsx
files in the examples folder; - You'll be able to get the required information from the imports (titles, ...);
- You can define additional testing configuration in the
unovis.cy.ts
file and match the corresponding examples by name or path. Or you can define it in theindex.tsx
files of the examples themsevles.
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.
@rokotyan I will give it a try. Is this PR getting too big? Should we have a separate PR for this and the duration
params?
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.
Up to you. I would do it here, but I don't see any problem with merging this PR first and having another one
Running this locally is a little tricky. You'd have to have a Percy account and a token to be able to do that. See this. It is possible to have a free personal account, which is what I did when I was testing this out. You need to create a project and it will generate a PERCY_TOKEN. You will then set it as part of your local env. |
82eeb5e
to
9d567db
Compare
9d567db
to
86f9ed0
Compare
6f2198b
to
7780999
Compare
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.
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.
Nice! I think I would try doing it in packages/dev/src/components/ExampleViewer/index.tsx
and passing as a prop, or defining a dedicated React hook that can be imported to avoid code duplication.
0c8331e
to
2ead8ea
Compare
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.
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.
@reb-dev the latest push should fully implement the duration
props for all dev files. I put the urls in a separate file, we can fully implement the autogen for testing urls in a separate PR.
29a2648
to
feddac8
Compare
@lee00678 dont enable percy run on merge to master. it will eat up the screenshot quota.
|
Core | Test: Add random seed generator for dev data
…ercy integration via browserstack
feddac8
to
fd7304c
Compare
It has a weekly run. Removed the trigger for merge to main. |
I'm setting the trigger for visual testing to happen when merge to main for now. I think we could also have Percy runs on a weekly basis just in case.
main