-
Notifications
You must be signed in to change notification settings - Fork 148
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
[QA] Component library tests #872
Conversation
for more information, see https://pre-commit.ci
View the example dashboards of the current commit live on PyCafe ☕ 🚀Updated on: 2024-11-25 15:29:21 UTC Link: vizro-core/examples/dev/ Link: vizro-core/examples/scratch_dev |
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.
Generally looks good 👍 I have lots of questions and comments just to make sure we get the initial structure and naming right. I'm not so familiar with testing images either so except lots of questions!
In general, as we move code into our main repo I'm going to apply a slightly higher standard of review compared to the vizro-qa repo so there might be a lot of comments! Very happy to discuss anything that's not clear though 🙂
…onent_library_tests
for more information, see https://pre-commit.ci
@antonymilne , thanks for the very useful review comments! |
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 looks really great, thank you!
I've added a few more minor suggestions. Most important thing is I don't think assert_image_equal
quite makes sense yet and we should probably break it into two functions. See what you think.
Once you've made any changes based on this round of review let's get @maxschulz-COL to have a final look through and then it's good to merge 💯
…sey/vizro into qa/component_library_tests � Conflicts: � vizro-core/tests/e2e/test_component_library.py � vizro-core/tests/tests_utils/e2e_asserts.py
for more information, see https://pre-commit.ci
@antonymilne , finished with your latest suggestions |
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.
Looks good, just made a small suggestion but otherwise @maxschulz-COL please could you do a quick check through and then feel free to merge!
…sey/vizro into qa/component_library_tests
…onent_library_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.
Looks good to me, thanks also for A to make such a thorough review before. I didn't write down any comment, but I think the e2e_asserts.py
would benefit from a few inline code comments about what you are doing. Since not everyone will be super familiar with cv2
etc, it is sometime hard to guess what even the overall strategy is. Maybe a few comments guide a reader on what the code is doing might be helpful?
Left comments about All your comment are now addressed, thanks! |
Description
Moved component library tests from vizro-qa
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":