Skip to content
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] e2e screenshots tests #1025

Merged
merged 54 commits into from
Mar 4, 2025
Merged

[QA] e2e screenshots tests #1025

merged 54 commits into from
Mar 4, 2025

Conversation

l0uden
Copy link
Contributor

@l0uden l0uden commented Feb 18, 2025

Description

In this PR I've moved all of the screenshot tests from vizro-qa repo to vizro.
Instead of self-written comparison logic I've used pixelmatch.
Rewrote all of the tests to use dash_br fixture, which helped to use dash testing methods including one that checks if callbacks were finished. It helped to make pages load more predictable.
Pixelmatch threshold helped to skip flaky states of tiny differences between two images.

Screenshots

Example of how differences will look like in case of tests failure

  1. File structure in downloaded archive:
    !image

  2. Here I changed the color of the navigation bar
    main_navbar_kpi_indicators_page[navbar_accordions].png
    main_navbar_kpi_indicators_page navbar_accordions

  3. The original screenshot
    main_navbar_kpi_indicators_page[navbar_accordions]_old.png
    main_navbar_kpi_indicators_page navbar_accordions _old

  4. Difference heatmap:
    main_navbar_kpi_indicators_page[navbar_accordions]_difference_from_main.png
    main_navbar_kpi_indicators_page navbar_accordions _difference_from_main

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

Copy link
Contributor

github-actions bot commented Feb 18, 2025

View the example dashboards of the current commit live on PyCafe ☕ 🚀

Updated on: 2025-03-04 09:40:52 UTC
Commit: 802ad88

Compare the examples using the commit's wheel file vs the latest released version:

vizro-core/examples/scratch_dev

View with commit's wheel vs View with latest release

vizro-core/examples/dev/

View with commit's wheel vs View with latest release

vizro-core/examples/visual-vocabulary/

View with commit's wheel vs View with latest release

vizro-ai/examples/dashboard_ui/

View with commit's wheel vs View with latest release

Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @l0uden,

I'm super excited for this to get merged soon! 🚀 I skimmed through the code and left some comments and requests. It's a big PR, so I'll take another pass after the next iteration. Overall, this looks awesome and having more stable tests is such a huge win! 🚀

Here are a few things I noticed:

1. To make these tests easier to understand, we should simplify the configuration wherever possible. This means:

  • Remove id from models if it's not used or needed by the tests.
  • Skip any configuration that's already the default, like theme="vizro_dark".
  • Leave out any setup that's not relevant to the test. For example, if you're testing table interactions, remove all the tabs/containers and create a separate test for those. I found some tests confusing because they seemed overly complex for what they were named (like "table_interaction"), but then I realized they were actually testing multiple things so that one test is probably more like "test_table_interaction_within_tabs_containers_and_check_tables_look_well_inside_containers" 😄 So, better to split these up and keep them simple.

2. For all the screenshot tests and functions, can you add a docstring or comments explaining what the function does (especially the bigger ones) and which element you're clicking on? The classNames are a bit cryptic, and I had to keep checking the element tree to understand. Some descriptive comments would really help! 🙏

3. Can you include a before/after screenshot in your PR description and explain the threshold a bit more? Like the one you sent me on Slack, so others can see how even a 1px difference is detected, what it looks like, and how the threshold affects it. And of course, make the threshold configurable. :)

Thanks!

Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really happy looking at the tests migration 🎉
I left just a few comments, and other that, and Li's comments, this PR looks really good! 🏅

@l0uden
Copy link
Contributor Author

l0uden commented Feb 28, 2025

Huge thanks @huong-li-nguyen and @petar-qb for your reviews!

I've tried to fix and explain every one of them.

Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-approving considering the last comments (tidying CSS).

If you could do one final check @l0uden , whether you can simplify the configurations, that would be great! Considering the things I've mentioned:

  • Removing ID when not required (filters and actions will automatically affect all components on screen so no need to add an id there, unless you specifically target only one component on that screen)
  • ...

@petar-qb
Copy link
Contributor

petar-qb commented Mar 1, 2025

I'll take the final look on Monday morning, and I promise there won't be many (if at all) comments from my side 😃

Congrats on this work A! 🚀

Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few comments, but other than that, the PR looks ready to merge 🚀 Great Alexey!

@l0uden
Copy link
Contributor Author

l0uden commented Mar 3, 2025

Pre-approving considering the last comments (tidying CSS).

If you could do one final check @l0uden , whether you can simplify the configurations, that would be great! Considering the things I've mentioned:

  • Removing ID when not required (filters and actions will automatically affect all components on screen so no need to add an id there, unless you specifically target only one component on that screen)
  • ...

Cleared the CSS and deleted unused settings in dashboards. 'ids' are in use by tests

@l0uden l0uden requested a review from petar-qb March 3, 2025 16:02
Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great now!

Thanks for handling data_frame parameter test case so well! bb9b560 🚀 🚀

@l0uden l0uden merged commit d62b5e5 into main Mar 4, 2025
36 checks passed
@l0uden l0uden deleted the qa/e2e_screenshot_tests branch March 4, 2025 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants