-
Notifications
You must be signed in to change notification settings - Fork 87
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
Percy and jest-puppeteer environment setup for visual testing #670
Percy and jest-puppeteer environment setup for visual testing #670
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.
Woooo this is super exciting! 🎉 🎉 Thank you @KshitijThareja!!! This is looking super good. I just left some minor comments/questions, but this is looking great! 😃
.eslintrc.js
Outdated
// Remove linting errors for the globals defined in the jest-puppeteer package | ||
esLintConfig.env = { | ||
...esLintConfig.env, | ||
'jest': true, |
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.
I dont think we should add this here, because this lint config is for the whole package, including the development environment where we are not running jest, and I think this could cause unwanted behaviours. Probably we could set these settings in a specific file for tests?
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.
Hi @KshitijThareja! why do we need this property here in the esLintConfig env?
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.
I had followed troubleshooting guidelines as specified here: Jest-Puppeteer
This was basically to recognize jest's global variables and not flag their use while running linting tests.
But on taking a closer look now, I can see that this property has already been included in the main esLint config import (kolibri-tools/.eslintrc
). I'll make the required changes.
Hi @bjester @AlexVelezLl :) For this test, I rendered the I'd love to hear your thoughts and suggestions on 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.
Thank you @KshitijThareja! This looks good, I ran the tests, and I didn't find any problems in Percy's dashboard. I just left some minor concerns, let me know what you think!
lib/KImg/__tests__/KImg.spec.js
Outdated
expect(error.mock.calls[0][0].message).toBe( | ||
'Missing required prop - provide altText or indicate isDecorative' | ||
); | ||
expect(() => |
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.
Hi @KshitijThareja! I think we can let this file without modifications. In this case it seems it is the expected behaviour to pass an error listener to catch these errors. See #645 (comment).
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.
Sure. I'll make the changes. I was currently working on finding a way to use concurrently
as discussed, so was a bit busy with that. I'll get this resolved by today at the earliest.
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.
Thank you @KshitijThareja!!! 🎉 This looks good. I have run some tests and it works fine, so the setup is working as expected. Follow up about concurrently will happen in #676.
Failed checks are because of node version update and missing changelog, but we dont need to add it in this PR as is still a feature WIP. So nothing else to not from my side, but will let the final approve and merge to @bjester 👐.
One last thing @KshitijThareja: please separate the visual tests from the existing |
905e770
into
learningequality:gsoc/visual-testing
…sting Percy and jest-puppeteer environment setup for visual testing
Description
This PR introduces the environment setup and configuration for introducing visual testing to the KDS repository.
Steps to test
yarn install
in the root directory of your local copy of KDS.yarn test:visual
to execute visual tests.Implementation notes
At a high level, how did you implement this?
To implement this, several new packages like
jest-puppeteer
,@percy/cli
, etc were added to the project's dependecies and several changes were made to the current testing environment to allow for visual tests to be run along with the unit tests.Does this introduce any tech-debt items?
--
Testing checklist
Reviewer guidance
After review
CHANGELOG.md
Comments