Add abstraction logic for simplifying writing visual tests #685
Conversation
…or custom describe blocks
There was a problem hiding this comment.
Thank you @KshitijThareja! We have achieved a good level of abstraction. But Im little bit concerned about the maintainability of the code in the future with this new way of writing describe blocks. Apart from that just left a couple of notes here and there. Thank you! Good work so far! 🎉
jest.conf/testUtils.js
Outdated
| } | ||
| } | ||
|
|
||
| export function describeUnit(name, fn) { |
There was a problem hiding this comment.
As far as possible, we should leave the unit tests intact, modifying them in this way would mean that we would have to:
- migrate all the tests we have right now.
- educate all devs who write tests and get them used to this new way of writing "describes"
- and reviewers should be more attentive so that we do not miss any.
So this makes the maintainability of the code a little more complex, makes bugs harder to find (It wouldn't be unusual to inadvertently end up with a describeVisual inside a describeUnit that never runs and we don't realize it). And above all because "describes" is a fairly standardized method already. Then we should look for another way that only changes the way of writing tests for the new functionality we are introducing, and avoid causing breaking changes with previous versions as much as possible.
I would suggest that having the [Visual] filter achieves that since with it we would not be introducing breaking changes for the unit tests, we would not have to do migrations, and it is also more flexible than the describe blocks, because we could have the flexibility to apply it as well to tests that are within other descriptions that are not necessarily visual. For example we could have
describe('KButton', () => {
describe('user interactions', () => {
it('some test testing user interactions');
it('some test testing user interactions');
it('[Visual] hover state');
});
});Or if we have multiple visual testing about user interactions:
describe('KButton', () => {
describe('user interactions', () => {
it('some test testing user interactions');
it('some test testing user interactions');
it('[Visual] hover state');
});
});And we have the flexibility of grouping these tests together with other unit tests to have them semantically together, describes inside other unit tests describes, or tests within other unit tests describes. The only restriction would be that we shouldnt have an unit test inside a visual block, but semantically it's a little more obvious to see it.
Let us know what you think!
There was a problem hiding this comment.
If we consider the additional overhead of migrating all the tests, you are right. I had concerns about that too, but since the idea was okayed in the meetings, I chose to go along with it.
I'll shift this to use the [Visual] tags, but we'd have to see a lot of tests marked as skipped in the console output. One way to handle that would be to use a custom reporter. Or we can just use verbose=false with the test commands to reduce the output verbosity in that case.
Let me know what you'd prefer!
There was a problem hiding this comment.
Oh, Im sorry, a thousand apologies if we okayed you in that direction, there was probably some misunderstanding on my side :').
Regarding the test outputs. For now I think there would be no problems in seeing the skipped tests, I was testing it and I see that Jest does report all the non-skipped test suites grouped at the end, and if there is an error it always reports them all at the end too, so they still have visibility. However if it becomes a problem I think we could consider the custom reporter option. Would love to know @bjester's opinion on that too.
There was a problem hiding this comment.
Oh no problem, I should have understood the requirements clearly.
For now I'll update the PR with the required changes. We can discuss and work on this later if the need arises.
|
Hi @AlexVelezLl 👋 |
package.json
Outdated
| "test:percy": "PERCY_LOGLEVEL=error npx percy exec -v -- jest --config jest.conf/visual.index.js -i ./lib/buttons-and-links/__tests__/KButton.spec.js", | ||
| "test": "jest --config=jest.conf/index.js", | ||
| "test:percy": "PERCY_LOGLEVEL=info npx percy exec -v -- jest --config jest.conf/visual.index.js -i ./lib/buttons-and-links/__tests__/KButton.spec.js -t '\\[Visual\\]'", | ||
| "test": "jest --config=jest.conf/index.js -t '^(?!.*\\[Visual\\])'", |
There was a problem hiding this comment.
To avoid complex regex patterns, we can also use the --testPathIgnorePatterns option, so we just replicate the regex of the previous script just changing the option flag -t with --testPathIgnorePatterns https://jestjs.io/docs/cli#--testpathignorepatternsregexarray
There was a problem hiding this comment.
Sure. Will try it out :)
There was a problem hiding this comment.
Just read that the flag --testPathIgnorePatterns is used to ignore specific test files based on their path and not the tests within it. Maybe if we had something like --testNameIgnorePatterns it would have been easier.
There was a problem hiding this comment.
Ooooh it's true, I didn't realize, it seems that jest doesn't have an ignore name pattern. Well, we could keep the regex then
There was a problem hiding this comment.
Just tested the regex and it seems to be ok 🎉
There was a problem hiding this comment.
Thank you @KshitijThareja!! LGTM! Code looks good, the new way of writing visual tests seems quite clean and easy to use and we have not required a large migration from the previous tests, and there do not seem to be any regressions 🎉. On my side it is already ok and ready to merge. Will left the final approve and merge to @bjester in case he has any comments. Thank you!
|
Hello @AlexVelezLl !! |
There was a problem hiding this comment.
Thank you @KshitijThareja!! This is looking soo good :D. Code changes make sense and just tested it and it is working as expected. Its ready to go!!
| @@ -10,6 +10,7 @@ const moduleNameMapper = { | |||
| module.exports = { | |||
| rootDir: path.resolve(__dirname, '..'), | |||
| moduleFileExtensions: ['js', 'json', 'vue'], | |||
| testNamePattern: '^(?!.*\\[Visual\\])', | |||
There was a problem hiding this comment.
Wooo, thats great, I researched if we had something like this for jest.config but I found nothing in the docs, but its great that this is working 🎉
AlexVelezLl
merged commit 9e45f45
into
learningequality:gsoc/visual-testing
Description
This PR aims to abstract the logic for various functions used for component rendering, taking snapshots for visual tests and separating the two types of tests so that they can run independently.
Issue addressed
#684
Changelog
[#PR no]: PR link
At a high level, how did you implement this?
This was done by creating new supporting functions in the
testUtils.jsto provide the necessary functionalities which were earlier written directly as a part of tests.Testing checklist
Reviewer guidance
After review
CHANGELOG.mdComments